Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle ENS Address Error #5605

Merged
merged 5 commits into from
Jan 26, 2023
Merged

Handle ENS Address Error #5605

merged 5 commits into from
Jan 26, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Jan 25, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
When an ENS name does not have an address registered, we expect to return an error to users. However, on mobile evan.eth currently resolves to 0x0000.....0000. See reported issue.

On mobile, ethjs-ens is used for resolving ens addresses. There's a known issue on said package. From the code, this PR returns when the resolvedAddress is 0x0000000000000000000000000000000000000000

Test Case
tested on mainnet

  • Tap to send eth
  • Enter evan.eth on the receiver (to) input

Screenshots/Recordings
http://recordit.co/4c3dFLDytL

Issue

Progresses #5396

Scenario: Send to unregistered ENS address
Given I am attempting a send transaction
When I enter an ENS name not pointed to an eth address in the TO field
Then I am NOT able to complete a send transaction destined to 0x000...0000

Scenario: Send to 0's manually
Given I am attempting a send transaction
When I manually type/paste '0x0000000000000000000000000000000000000000' in the TO field
Then I am able to complete a send transaction destined to 0x000...0000

Scenario: Send to 0's saved as a contact
Given I have previously saved a contact with destination address '0x0000000000000000000000000000000000000000'
When I attempt a send transaction
And I select this previously saved contact as a destination in the TO field
Then I am able to complete a send transaction destined to 0x000...0000

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@blackdevelopa blackdevelopa requested a review from a team as a code owner January 25, 2023 14:03
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-confirmations-secure-ux-PR PR from the confirmations team labels Jan 25, 2023
@blackdevelopa blackdevelopa self-assigned this Jan 25, 2023
app/constants/transaction.ts Outdated Show resolved Hide resolved
app/util/ENSUtils.js Outdated Show resolved Hide resolved
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jan 25, 2023
@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 25, 2023
Copy link
Contributor

@plasmacorral plasmacorral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks deeplink payment requests to ENS addresses. This functions in production v5.13 build 1042 on my device (Samsung a515f w/Android11).

This can be tested by either tapping on a deeplink or scanning it as a QR code from wallet view.

Video: https://recordit.co/ftn5q2HgMU

Expectation: Should be presented confirmation and have the chance to submit the transaction

Current State: Just returned to wallet view after engaging a payment deeplink to ENS address

Some example deeplinks:

https://metamask.app.link/send/0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48@1/transfer?address=evan.ethereum.eth&uint256=1e4

https://metamask.app.link/send/pay-metarnask.eth@1?value=1e13

@plasmacorral plasmacorral added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Jan 25, 2023
@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jan 26, 2023
@plasmacorral
Copy link
Contributor

plasmacorral commented Jan 26, 2023

EDIT: Looks like this may be an issue on main

Ok...we made a production build to rule out any deeplinking issues in the debug build. While payment deeplinks now work in build 1053 when a user clicks on them, they are still not functioning when scanned from the Qr scanner at wallet view.

Please test with these sample QR codes:
Screenshot 2023-01-26 at 1 01 00 PM
Screenshot 2023-01-26 at 1 00 47 PM

@plasmacorral plasmacorral added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Jan 26, 2023
@plasmacorral
Copy link
Contributor

Tested:

  • Valid ENS deeplink Eth payment
  • Valid ENS deeplink ERC20 payment
  • Invalid ENS deeplink
  • Intentionally sending to 0x000...000 to burn an asset
  • Payment deeplink on testnet without ENS
  • Deeplink to DAPP

Note that scanning a QR to deeplink is broken on main currently, so this was not successfully tested.

@plasmacorral plasmacorral dismissed their stale review January 26, 2023 19:52

Turns out this is an issue on main, not from this branch.

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jan 26, 2023
@plasmacorral plasmacorral merged commit 4ab3206 into main Jan 26, 2023
@plasmacorral plasmacorral deleted the resolve_ens/5396 branch January 26, 2023 20:39
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2023
@cortisiko cortisiko added the release-6.0.0 Issue or pull request that will be included in release 6.0.0 label Jan 30, 2023
@blackdevelopa blackdevelopa added release-5.14.1 Issue or pull request that will be included in release 5.14.1 and removed release-6.0.0 Issue or pull request that will be included in release 6.0.0 labels Feb 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-5.14.1 Issue or pull request that will be included in release 5.14.1 team-confirmations-secure-ux-PR PR from the confirmations team unit test coverage confirmed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants