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

[IMPROVEMENT] Address Verification #4418

Merged
merged 9 commits into from
Jun 9, 2022
Merged

Conversation

gantunesr
Copy link
Member

Description

Improve address verification in SendFlow,

  1. Allow user to input an address with all lower or upper case characters.
  2. If the user inputs a mixed case address, it should be the correct checksum. Else, the address should be flagged as invalid and not auto-checksum the address.
  • Example of incorrect checksum: 0x87187657b35f461d0ceEc338d9B8e944A193afe2
  • Example of correct checksum: 0x87187657b35F461D0Ceec338d9b8E944a193aFE2

Checklist

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

Screenshots/Recordings

Bug Fix

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/288

@gantunesr gantunesr 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) Mobile QA board labels May 30, 2022
@gantunesr gantunesr requested a review from a team as a code owner May 30, 2022 05:04
Copy link
Contributor

@Fatxx Fatxx left a comment

Choose a reason for hiding this comment

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

Should we also add the same validation in Views/Send/index.js screen?

@gantunesr
Copy link
Member Author

@Fatxx I don't see any validation logic in Views/Send/index.js and we should stop using it in favor of SendFlow

@gantunesr gantunesr removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jun 1, 2022
@cortisiko cortisiko 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 Jun 6, 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.

I tested these Incorrect check sum addressed:

0xa54D3c09E34aC96807c1CC397404bF2B98DC4eFb
0x87187657b35f461d0ceEc338d9B8e944A193afe2

on the send flow and also on the add contact view. I also generated QR codes for the above addresses and tested with them. See QR codes here and here. I tested upper cased addresses and lower cased addresses.

I tested scanning a keystone address QR code and I was able to successfully send to that address.

Finally I did sanity checks on:
scanning a request QR code. The correct address received the funds
Scanning an extension QR code for a couple of my accounts. I had no issues

QA passed! 🌮 🌮

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Jun 9, 2022
@gantunesr gantunesr merged commit c53718c into main Jun 9, 2022
@gantunesr gantunesr deleted the improv/address-verification branch June 9, 2022 21:55
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2022
@tommasini tommasini added the release-5.3.0 All PRs that will be included in 5.3.0 release label Jun 14, 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.3.0 All PRs that will be included in 5.3.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants