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 QR send #1916

Merged
merged 11 commits into from
Nov 6, 2020
Merged

Fix QR send #1916

merged 11 commits into from
Nov 6, 2020

Conversation

andrepimenta
Copy link
Member

@andrepimenta andrepimenta commented Oct 27, 2020 •
edited
Loading

Description

PR that fixes QR code send flow:

  1. It goes to the right send flow from QR.
  2. It shows a warning when the user inserts a token contract address or smart contract in the “to” field

@andrepimenta andrepimenta requested a review from a team as a code owner October 27, 2020 18:51
@andrepimenta andrepimenta marked this pull request as draft October 27, 2020 19:40
@andrepimenta andrepimenta added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 27, 2020
@andrepimenta andrepimenta marked this pull request as ready for review October 27, 2020 22:31
rickycodes
rickycodes reviewed Oct 28, 2020
View reviewed changes
app/core/DeeplinkManager.js Outdated
urlObj.set('protocol', 'https:');
this.handleBrowserUrl(urlObj.href, browserCallBack);
break;

// Specific to the MetaMask app
// For ex. go to settings
case 'metamask':
onHandled && onHandled();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could consolodate these into a single method? eg:

const handle = () => onHandled && onHandled();

and then just call that?

@ibrahimtaveras00
Copy link
Contributor

All scenarios look good to me 👍

There's just 2 instances where I noticed we don't provide any feedback and just keep you on the send to screen:

  1. when you're on the send to view and you scan a private key QR code
  2. when you're on the send to view and you scan a seed phrase QR code

Do we want to do anything in this regard @andrepimenta @omnat @cjeria

This is what we currently do if a user is on the wallet screen and they scan such things:

  • Private Key

Screen Shot 2020-10-29 at 7 59 00 PM

  • Seed phrase

Screen Shot 2020-10-29 at 7 59 15 PM

Perhaps we send them to wallet with this messaging and continue from there?

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA Passed A successful QA run through has been done labels Oct 30, 2020
rickycodes
rickycodes suggested changes Nov 3, 2020
View reviewed changes
locales/en.json
"cancelled": "Cancelled",
"tokenContractAddressWarning": "WARNING: This address is a token contract address. If you send tokens to this address, you will lose them.",
"smartContractAddressWarning": "WARNING: This address is a smart contract address. Please make sure you understand what this address is for, otherwise you risk losing your funds.",
"continueError": "I understand the risks, continue"
Copy link
Contributor

Choose a reason for hiding this comment

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

es.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed QA'd but questions A QA run through has been done but you need clarification on minor issues you found labels Nov 4, 2020
@omnat
Copy link
Contributor

omnat commented Nov 4, 2020

@andrepimenta @cjeria based off of our discussion in team standup, could we remove the warning when user enters just a contract address (which isn't a token address)?
It's totally my bad for not considering the case where people are actually intending to send tokens to smart contract accounts. I agree with Dan that the user error/mistakes are most of the times when sending to token addresses.

rickycodes
rickycodes reviewed Nov 4, 2020
View reviewed changes
app/core/DeeplinkManager.js
@@ -78,12 +78,15 @@ class DeeplinkManager {
}
}

const handled = () => onHandled?.();
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

rickycodes
rickycodes approved these changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@rickycodes rickycodes left a comment

Choose a reason for hiding this comment

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

👍

@andrepimenta
Copy link
Member Author

@andrepimenta @cjeria based off of our discussion in team standup, could we remove the warning when user enters just a contract address (which isn't a token address)?
It's totally my bad for not considering the case where people are actually intending to send tokens to smart contract accounts. I agree with Dan that the user error/mistakes are most of the times when sending to token addresses.

Done it will show a warning:
Simulator Screen Shot - iPhone 11 Pro - 2020-11-04 at 20 32 15

ibrahimtaveras00
ibrahimtaveras00 approved these changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Update looks good on both OS's 👍

ibrahimtaveras00
ibrahimtaveras00 approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Latest fixes look good 👍

@andrepimenta andrepimenta merged commit 43a5f82 into develop Nov 6, 2020
@andrepimenta andrepimenta deleted the bugfix/qr-send-flow branch November 6, 2020 00:49
@omnat omnat linked an issue Nov 16, 2020 that may be closed by this pull request
When I scan a QR code, whether its a payment request or recipient public address, I want to be redirected with the right values filled in. #1999
Closed
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
@andrepimenta
Fix QR send (#1916)
5baf8de 
* Fix QR send

* Working for more types

* More comments

* Remove unnecessary export

* Warning for smart and token address

* Fixes and new warning info adjustments

* Remove comment

* Add comments

* Show error for erc721 and erc20

* Fix bold style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

@rickycodes rickycodes rickycodes approved these changes

Assignees
No one assigned
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) next release QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
4 participants
@andrepimenta @ibrahimtaveras00 @omnat @rickycodes