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.

Sign up for GitHub

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

Jump to bottom

[FIX] ENS Resolves to Wrong Address when DeepLink #5656

Merged
merged 5 commits into from
Feb 8, 2023
Merged

[FIX] ENS Resolves to Wrong Address when DeepLink #5656

merged 5 commits into from
Feb 8, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Feb 1, 2023 •
edited
Loading

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
Approving an ERC20 using deep link resolves incorrect ENS. See issue

Screenshots/Recordings
https://recordit.co/NEnSKMPdrB

Issue

Progresses #5648

Checklist

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

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 marked this pull request as ready for review February 1, 2023 12:15
@blackdevelopa blackdevelopa requested a review from a team as a code owner February 1, 2023 12:15
@blackdevelopa blackdevelopa changed the title ens-fix [FIX] ENS Resolves to Wrong Address when DeepLink Feb 1, 2023
@blackdevelopa blackdevelopa self-assigned this Feb 1, 2023
@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 Feb 1, 2023
tommasini
tommasini requested changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

It would be nice to have the validation logic covering both scenarios on one util folder, wdyt?

app/core/DeeplinkManager.js Outdated Show resolved Hide resolved
tommasini
tommasini approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

Just good to have:
Could we have test cases when:

  • Address is not valid
  • ENS is not valid

jpuri
jpuri approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

LGTM

@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 Feb 2, 2023
@seaona
Copy link
Contributor

seaona commented Feb 8, 2023 •
edited
Loading

Functionality Tested:

  • ERC20 & ERC721 Approve: ENS is resolving to the correct address, when the Primary Address is set on ENS
  • ERC20 & ERC721 Approve: MM is giving an error when the ENS does not have a Primary Address set (i.e. evan.eth) and you cannot proceed
ens-approve.mp4

Extra Tests for general ENS flows:

  • On a regular Send, the ENS address never resolves, for the ones that don't have the Primary Address set
  • On a Deeplink Send, an error is thrown, for the ENS that don't have the Primary Address set
deeplink-send.mp4

jpcloureiro
jpcloureiro approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@jpcloureiro jpcloureiro left a comment

Choose a reason for hiding this comment

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

Code looks good to me ✅
Left one suggestion

app/components/Views/Send/index.js Outdated Show resolved Hide resolved
app/util/address/index.js Show resolved Hide resolved
@seaona seaona merged commit f32bef7 into main Feb 8, 2023
@seaona seaona deleted the ens-fix branch February 8, 2023 11:48
@seaona seaona added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Feb 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2023
@blackdevelopa blackdevelopa added the release-5.14.1 Issue or pull request that will be included in release 5.14.1 label Feb 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tommasini tommasini tommasini approved these changes

@jpuri jpuri jpuri approved these changes

@jpcloureiro jpcloureiro jpcloureiro approved these changes

Assignees

@blackdevelopa blackdevelopa

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@blackdevelopa @seaona @jpcloureiro @jpuri @tommasini @hilvmason