Skip to content

fix: Token allowance flow update #6619

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
Merged
seaona merged 7 commits into from
Jun 26, 2023
Merged

fix: Token allowance flow update #6619

seaona merged 7 commits into from
Jun 26, 2023

Conversation

@blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Jun 15, 2023 •
edited
Loading

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

Screenshots/Recordings

Screen.Recording.2023-06-15.at.20.15.31.mov

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

Issue

Progresses https://github.com/MetaMask/MetaMask-planning/issues/685

Checklist

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

@blackdevelopa blackdevelopa requested a review from a team as a code owner June 15, 2023 19:18
@github-actions
Copy link
Contributor

github-actions bot commented Jun 15, 2023

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.

@blackdevelopa blackdevelopa self-assigned this Jun 15, 2023
@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-confirmations-secure-ux-PR PR from the confirmations team labels Jun 15, 2023
jpuri
jpuri previously approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

🚢

digiwand
digiwand reviewed Jun 16, 2023
View reviewed changes
digiwand
digiwand previously approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

no blockers. minor comments above

@digiwand digiwand added the DO-NOT-MERGE Pull requests that should not be merged label Jun 16, 2023
@blackdevelopa blackdevelopa dismissed stale reviews from digiwand and jpuri via e245dfe June 16, 2023 19:28
@blackdevelopa blackdevelopa requested review from digiwand and jpuri June 19, 2023 09:09
@bschorchit
Copy link

bschorchit commented Jun 20, 2023

Updated copy changes:

  1. Copy for top right link should beUse site suggestion instead of Use third party suggestion
    Screenshot 2023-06-20 at 13 26 29

  2. Copy for when amount is higher than balance should be: This allows the third party to spend all your token balance until it reaches the cap or you revoke the spending cap. If this is not intended, consider setting a lower spending cap. Learn more
    Screenshot 2023-06-20 at 13 27 23

  3. Copy for when there's an input error or for when field is empty should be: Only enter a number that you're comfortable with the third party spending now or in the future. You can always increase the spending cap later. Learn more as suggested by Mariona here fix: Disable next button if input is invalid for token approval #6634 (comment)
    Screenshot 2023-06-20 at 13 32 47

@blackdevelopa blackdevelopa force-pushed the 685/token-allowance-flow branch from e245dfe to a8434da Compare June 20, 2023 17:46
digiwand
digiwand reviewed Jun 20, 2023
View reviewed changes
digiwand
digiwand approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

🙌

@blackdevelopa blackdevelopa 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 Jun 21, 2023
@blackdevelopa blackdevelopa changed the base branch from main to release/7.2.0 June 21, 2023 15:39
@seaona seaona removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Jun 22, 2023
@seaona
Copy link
Member

seaona commented Jun 22, 2023

The PR looks good @blackdevelopa . I can see how the new Approve flow has been updated with the specs above.
I have encountered one issue: whenever the user sets a custom value, if clicks View transaction details the value is changed back to the default dapp value. This same behaviour also happens if you click Edit - you will see that the value is again the default by the dapp instead of the one the user filled.

custom-approve-value.mp4

Since this is a pretty big PR, maybe the PR could be merged and we could fix that in a separate issue.
What do you think @blackdevelopa @bschorchit ?

@bschorchit
Copy link

bschorchit commented Jun 22, 2023

I agree with fixing in a different PR ✅

@seaona seaona added QA Passed QA testing has been completed and passed and removed DO-NOT-MERGE Pull requests that should not be merged labels Jun 23, 2023
@blackdevelopa blackdevelopa force-pushed the 685/token-allowance-flow branch from 082a6f9 to 3f319e5 Compare June 23, 2023 17:23
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

44.6% 44.6% Coverage
0.0% 0.0% Duplication

@seaona seaona added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA Passed QA testing has been completed and passed labels Jun 26, 2023
@seaona
Copy link
Member

seaona commented Jun 26, 2023

@blackdevelopa from QA rebase looks good. I think we need x2 confirmations from @jpuri and @digiwand that the last changes also look good from dev side before merging 🙏 thank you!!

@blackdevelopa blackdevelopa requested review from digiwand and jpuri June 26, 2023 08:25
@seaona seaona added QA Passed QA testing has been completed and passed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jun 26, 2023
@seaona seaona merged commit ae3c789 into release/7.2.0 Jun 26, 2023
@seaona seaona deleted the 685/token-allowance-flow branch June 26, 2023 13:25
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jpuri jpuri jpuri approved these changes

+1 more reviewer

@digiwand digiwand digiwand approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@blackdevelopa blackdevelopa

Labels

QA Passed QA testing has been completed and passed team-confirmations-secure-ux-PR PR from the confirmations team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@blackdevelopa @bschorchit @seaona @jpuri @digiwand