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 network changes for incoming deeplink and qr code requests. #3650

Merged
merged 13 commits into from
Feb 8, 2022

Conversation

Fatxx
Copy link
Contributor

@Fatxx Fatxx commented Jan 26, 2022

Description

When a user receives an incoming request either by deeplink or QR code scan, and he doesn't have the requested network selected it should now automatically switch to the requested network or shows up an alert if the network isn't available in user preferred networks list.

To Reproduce
Steps to reproduce the behavior

Create a deeplink in browser or QR code payment request from one network
With your device, open app and be on a different network than above request
Scan QR code, or open deeplink from browser
Feel free to use this deeplink which should change network to Rinkeby:

https://metamask.app.link/send/0x1FDb169Ef12954F20A15852980e1F0C122BfC1D6@4?value=1e15

Checklist

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

Issue

Resolves #2557 and #2836

@Fatxx Fatxx requested a review from a team as a code owner January 26, 2022 17:57
@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.

@Fatxx Fatxx linked an issue Jan 26, 2022 that may be closed by this pull request
@cortisiko cortisiko added QA in Progress QA has started on the feature. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jan 26, 2022
@Fatxx Fatxx changed the base branch from develop to main January 26, 2022 19:07
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.

@Fatxx sending this back your way. Below are the list of issues I found:

Issue 1
If I attempt to deep link to a custom network (such as matic or binance) and that network is not added to my wallet I get conflicting behavior. I see the same clipboard alert message when my network is successfully changed and in that message I also see “oops something went wrong please try again”

to reproduce:
ensure you do not have matic added to your list of networks
deep link here

notice the behavior.

Issue 1B

If i were to:
switch to a custom network. In this case i am using binance.
create a request qr-code/deep link by going through the request flow in the app on device A,
On device B i do not have binance (BSC) added to my wallet
scan the QR on device B
on device B once the QR takes you to the send flow, the network work defaults to your current wallet. Shouldn’t we let the user know that they do not have the network(in this case binance) added?

see here

@cortisiko cortisiko 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, 2022
@cortisiko
Copy link
Member

cortisiko commented Jan 30, 2022

Potential Issue 2

There is a bug with the clipboard alert whenever a network changes from a custom network back to main net. Unfortunately I am unable to provide a screenshot/recording because this happens sporadically.

@cortisiko cortisiko mentioned this pull request Feb 2, 2022
3 tasks
@Fatxx
Copy link
Contributor Author

Fatxx commented Feb 4, 2022

@cortisiko you can QA if you like, although it needs another dev review.

@Fatxx Fatxx changed the title Add support for main and test networks on Send requests. Handle network change for incoming deeplink and qr code requests. Feb 4, 2022
@Fatxx Fatxx changed the title Handle network change for incoming deeplink and qr code requests. Handle network changes for incoming deeplink and qr code requests. Feb 4, 2022
@andrepimenta andrepimenta added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Feb 4, 2022
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.

🌮 🌮 🌮

@cortisiko cortisiko removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Feb 8, 2022
@cortisiko cortisiko added the QA Passed A successful QA run through has been done label Feb 8, 2022
@omnat
Copy link
Contributor

omnat commented Feb 8, 2022

@Fatxx did you want my input on copy on this issue? I don't see a suggestion. Am I at the wrong issue?

Copy link
Member

@andrepimenta andrepimenta left a comment

Choose a reason for hiding this comment

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

LGTM!

@Fatxx
Copy link
Contributor Author

Fatxx commented Feb 8, 2022

@Fatxx did you want my input on copy on this issue? I don't see a suggestion. Am I at the wrong issue?

@omnat I tagged you again

@gantunesr gantunesr 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) QA Passed A successful QA run through has been done labels Feb 8, 2022
@cortisiko cortisiko 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, 2022
@gantunesr gantunesr merged commit 0d3d54e into main Feb 8, 2022
@gantunesr gantunesr deleted the fix/requesting-token-doesnt-respect-network branch February 8, 2022 18:28
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERC681 URI doesn't change network on iOS mobile app Requesting token doesn't respect network
5 participants