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

[FEATURE] On-ramp: Add buy-crypto deeplink #5743

Merged
merged 8 commits into from
Apr 20, 2023
Merged

[FEATURE] On-ramp: Add buy-crypto deeplink #5743

merged 8 commits into from
Apr 20, 2023

Conversation

wachunei
Copy link
Member

@wachunei wachunei commented Feb 9, 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

This PR adds support for metamask://buy-crypto and https://metamask.app.link/buy-crypto, these will open the buy flow when the current network supports it.

Screenshots/Recordings

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

Issue

Progresses #???

Checklist

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

@wachunei wachunei added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-ramp issues related to Ramp features release-6.2.0 Issue or pull request that will be included in release 6.2.0 labels Feb 9, 2023
@wachunei wachunei requested a review from a team as a code owner February 9, 2023 18:56
@wachunei wachunei force-pushed the feature/onramp-deeplink branch from eb83cd7 to 13518bd Compare February 9, 2023 18:56
@cortisiko cortisiko added needs-ramp-qa Tickets that need feature QA on the ramps flows and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Feb 16, 2023
@lorenzosantos
Copy link

@wachunei, can we configure the https://metamask.app.link/buy-crypto link to open the Pdapp https://portfolio.metamask.io/buy/ if the user is clicking on the desktop?

@vpintorico vpintorico added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-ramp-qa Tickets that need feature QA on the ramps flows labels Mar 7, 2023
@Cal-L
Copy link
Contributor

Cal-L commented Mar 7, 2023

@wachunei Could you provide a video of this feature in action? Is it also intended to work for both tapping on a link as well as scanning a QR code?

@wachunei
Copy link
Member Author

wachunei commented Mar 7, 2023

@Cal-L sadly no, the simulator was not handling any of the deeplinks correctly.

Is it also intended to work for both tapping on a link as well as scanning a QR code?

Yes, these two should work:
imageimage

@worldlyjohn
Copy link

@lorenzosantos might also make sense to trigger an event here with referrer to know if this is being used and where their coming from (assuming user opted-in)

@lorenzosantos
Copy link

@lorenzosantos might also make sense to trigger an event here with referrer to know if this is being used and where their coming from (assuming user opted-in)

@worldlyjohn we're intentionally keeping this PR simple as a POC while we develop a buy intent spec that can handle all the on-ramp edge cases.

We plan to add several properties like token, amount, and referrer in an upcoming release (more details here).

cortisiko
cortisiko previously requested changes Mar 9, 2023
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

swinging this back your way @wachunei

1
When I attempt to deep link to a contract approval: https://metamask.app.link/approve/0x514910771AF9Ca656af840dff83E8264EcF986CA@1/approve?address=0x178e3e6c9f547A00E33150F7104427ea02cfc747&uint256=3e18

I get TypeError: undefined is not can object (evaluating ‘’t.toLowerCase;)
See recording: http://recordit.co/WSXnJIi1ro

2
When I deep link to the onramps flow via the URL SCHEMA deep link, then try to deep link to a dapp, The onramps flow always appear prior to opening the dapp.

See recording:
https://recordit.co/dDdpvRSzUa

To reproduce:
Deep link here metamask://buy-crypto
Tap get started
Then tap on “cancel”
Now kill the app and attempt to deep link here: https://metamask.app.link/app.olympusdao.finance/#/bonds
Notice the app opens to the onramps screen
If you were to tap cancel, then you are taken to the dapp in the browser

@cortisiko cortisiko added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 9, 2023
@wachunei
Copy link
Member Author

wachunei commented Mar 9, 2023

  1. Can you reproduce the error on main? test cases before 45s mark seem OK, for the last one I see no reason for this code to cause it 🤔 .

  2. I see, this seems like the deeplink is not marked as handled, maybe I am missing a handled() call. @andreahaku I'd appreciate if you can review this code change please, I might be missing something 🙏 .

@wachunei wachunei added release-6.2.0 Issue or pull request that will be included in release 6.2.0 release-6.3.0 Issue or pull request that will be included in release 6.3.0 and removed release-6.2.0 Issue or pull request that will be included in release 6.2.0 labels Mar 20, 2023
andreahaku
andreahaku approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@andreahaku andreahaku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can't reproduce the issue as well. Checked the code, tested the deep link and universal link, tried to reproduce the issue following the instruction by @cortisiko and everything work correctly as expected. LGTM

@andreahaku andreahaku added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 30, 2023
@chrisleewilcox chrisleewilcox added team-mobile-client 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 Apr 6, 2023
@sethkfman sethkfman added release-6.5.0 Issue or pull request that will be included in release 6.5.0 needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed release-6.4.0 Issue or pull request that will be included in release 6.4.0 QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 11, 2023
@chrisleewilcox
Copy link
Contributor

@wachunei I am seeing the Your Region screen constantly pop back up if user cancels out of it and never continues pass Your Region.
https://recordit.co/iy7Gfm09zu

@wachunei
Copy link
Member Author

@chrisleewilcox is the browser loading the deeplink again and again? Can you kill the browser from the task manager?

@andreahaku how can we solve the issue? If the above is not the case then the deeplink is not being marked as handled although handled() is being called.

@chrisleewilcox
Copy link
Contributor

@wachunei to clarify, it's seems to be any screen during the payment flow to set region or pay method these screens will pop back up when MMM app is selected/foreground on device. Device browser closed still does not fix problem.
https://recordit.co/aggn1nK3bB

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

chrisleewilcox commented Apr 20, 2023 •
edited
Loading

Issue #1 reported from @cortisiko is resolved.

Issue #2 reported from @cortisiko is resolved.

Payment deeplink device browser tap

Payment deeplink in-app browser tap

Regression:
Send Flow deeplink: PASS
URL deeplink: PASS
QR Scan URL deeplink: PASS

chrisleewilcox
chrisleewilcox approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

LGTM

@chrisleewilcox chrisleewilcox merged commit 9dadefa into main Apr 20, 2023
@chrisleewilcox chrisleewilcox deleted the feature/onramp-deeplink branch April 20, 2023 23:57
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2023
@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NicolasMassart NicolasMassart NicolasMassart left review comments

@andreahaku andreahaku andreahaku approved these changes

@chrisleewilcox chrisleewilcox chrisleewilcox approved these changes

@cortisiko cortisiko Awaiting requested review from cortisiko

Assignees
No one assigned
Labels
QA Passed A successful QA run through has been done release-6.5.0 Issue or pull request that will be included in release 6.5.0 team-mobile-platform team-ramp issues related to Ramp features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.