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

Support for ens on deeplink transactions #5191

Merged
merged 20 commits into from
Dec 16, 2022
Merged

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Nov 2, 2022

Description
When reading a deep link with an ENS instead of an address, when it was an ERC20 token it was being resolved to the wrong address on the confirm screen. When it was eth, the transaction would not have a destination address to send the eth.

Proposed Solution
It is implementing support for deep link ERC20 and ETH transactions with ENS. When parsing the recipient from the deep link, we resolve it to an address if it is an ENS.

Screenshots/Recordings

Bug Solution
Wrong UI Right UI

Test Cases
Use https://metamask.github.io/metamask-deeplinks/# to generate the deep links

Case1 (ERC20 tx):

  • The ens resolve in the correct way
  • Couldn't submit the transaction due to this issue

Case2: (ETH tx):

  • The ens resolve in the correct way
  • Transaction submitted with success

Case3: (with the wrong ens or wrong address):

  • Should appear one alert saying the recipient is wrong (Alert message approved 🚀)

Code impact
Low - Will impact the Send component and the deep link (QR Code) transactions

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses #5176

Checklist

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

@tommasini tommasini requested a review from a team as a code owner November 2, 2022 18:07
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

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.

@tommasini tommasini added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Nov 2, 2022
app/components/Views/Send/index.js Outdated Show resolved Hide resolved
app/components/Views/Send/index.js Outdated Show resolved Hide resolved
Copy link
Member

@gantunesr gantunesr 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 left a last question

app/components/Views/Send/index.js Outdated Show resolved Hide resolved
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM. Sending it to QA

@gantunesr gantunesr 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 Nov 8, 2022
@plasmacorral plasmacorral self-assigned this Nov 22, 2022
@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 Nov 29, 2022
@plasmacorral
Copy link
Contributor

Observed on Android 11 device (Samsung a515f) that if the deeplink is to an invalid ENS name (borangefox.eth), that the user is presented with a (seemingly) infinite spinning wheel.

deeplink tested:
https://metamask.app.link/send/0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48@1/transfer?address=borangefox.eth&uint256=1e15

https://recordit.co/pzAAfjaMdS

@plasmacorral
Copy link
Contributor

RE: Small fix on UI of confirm screen

I believe that we should leave the line spacing as-is rather than centering the address in the field. There is an issue in production that if the TO: address is saved as a contact that neither the saved name nor the ENS are presented above the resolved address, like it would if the ENS name was not stored as a contact and was instead entered via the keyboard into the TO: field.

I will open the production issue in another ticket.

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Dec 16, 2022
@plasmacorral
Copy link
Contributor

Validated patch and invalid address with ENS names, as well as dapp and payment deeplinks.

@plasmacorral plasmacorral merged commit 01680ea into main Dec 16, 2022
@plasmacorral plasmacorral deleted the fix/ens-deeplink-issue branch December 16, 2022 01:31
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2022
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.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants