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

feat: Custom Spend Allowance #5591

Merged
merged 44 commits into from
Jun 2, 2023
Merged

feat: Custom Spend Allowance #5591

merged 44 commits into from
Jun 2, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Jan 24, 2023 •
edited

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
Currently when approving a token transaction, there's an option to set a custom spend limit to be approved. In this PR, that flow is broken into two screens: a custom spend screen to set a spend cap and a review screen to approve transaction. This issue is defined further here (#178) and (#177). This PR also removes the metric for DAPP_APPROVE_SCREEN_EDIT_PERMISSION for token approve screen.

Design

Test Case

  • custom spend allowance should match design flow
  • using metamask testdapp, approve:
  • erc721
  • erc20
  • using deeplink to approve confirmation screen

Screenshots/Recordings
Before
http://recordit.co/P8mSgLguHF

After
http://recordit.co/PVJF0qx9P3
http://recordit.co/LY5Z3Us1Ax

Issue
Progresses #178 and #177

Checklist

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

@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.

@blackdevelopa blackdevelopa self-assigned this Jan 24, 2023
@blackdevelopa blackdevelopa added the team-confirmations-secure-ux-PR PR from the confirmations team label Jan 24, 2023
@blackdevelopa blackdevelopa force-pushed the ft/178-set-spend-cap branch 2 times, most recently from cbbd57a to ac44257 Compare February 16, 2023 22:38
@blackdevelopa blackdevelopa added the needs-design Feature that requires UI/UX design label Feb 21, 2023
@blackdevelopa blackdevelopa marked this pull request as ready for review February 23, 2023 14:32
@blackdevelopa blackdevelopa requested a review from a team as a code owner February 23, 2023 14:32
@blackdevelopa blackdevelopa marked this pull request as draft March 13, 2023 11:25
@blackdevelopa blackdevelopa force-pushed the ft/178-set-spend-cap branch 2 times, most recently from 061d5a1 to aa17390 Compare March 13, 2023 11:49
@blackdevelopa blackdevelopa marked this pull request as ready for review March 14, 2023 08:18
@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed needs-design Feature that requires UI/UX design labels Mar 14, 2023
@blackdevelopa
Copy link
Contributor Author

blackdevelopa commented Mar 14, 2023 •
edited

Hi @holantonela following our conversation on Slack, please see the loading state
http://recordit.co/5w4afYZTlY

cc: @bschorchit

@blackdevelopa blackdevelopa added the design-review Any feature that needs feedback from the design team label Mar 14, 2023
@blackdevelopa
Copy link
Contributor Author

blackdevelopa commented Mar 14, 2023 •
edited

Taking this back to draft temporarily to complete close out small bug fixes

@blackdevelopa blackdevelopa marked this pull request as draft March 14, 2023 12:45
@blackdevelopa blackdevelopa marked this pull request as ready for review March 20, 2023 20:11
@blackdevelopa blackdevelopa removed the design-review Any feature that needs feedback from the design team label Mar 21, 2023
@blackdevelopa blackdevelopa changed the title Set Custom Spend Cap Custom Spend Allowance Mar 21, 2023
@NicolasMassart NicolasMassart dismissed a stale review via 3dd56b2 May 30, 2023 15:05
@sethkfman sethkfman dismissed stale reviews from ghost via 3dd56b2 May 30, 2023 16:38
@blackdevelopa blackdevelopa dismissed a stale review via ad07ba5 May 30, 2023 17:58
@blackdevelopa blackdevelopa requested a review from jpuri May 30, 2023 18:13
@sonarcloud
Copy link

sonarcloud bot commented May 30, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

25.4% 25.4% Coverage
0.0% 0.0% Duplication

@tommasini tommasini dismissed a stale review via ad07ba5 May 30, 2023 19:43
@jpuri jpuri dismissed a stale review via ad07ba5 May 30, 2023 23:03
jpuri
jpuri approved these changes May 31, 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.

LGTM

@sethkfman sethkfman dismissed a stale review via ad07ba5 June 1, 2023 12:24
@NicolasMassart NicolasMassart dismissed a stale review via ad07ba5 June 1, 2023 14:52
@seaona
Copy link
Contributor

seaona commented Jun 1, 2023

@blackdevelopa I see the issues fixed!! Overall looks good 🔥 I don't want to hold more this PR so I would QA Pass it.

There is a small improvement that could be made with load state. Basically, whenever is on a loading state we don't seem to see any indicator that is loading, but we see the Approve button disabled for some seconds - which might create confusion to the user. I would open a separate issue for that, if we agree @blackdevelopa @bschorchit , so we can merge this PR. Also, I will continue QAing this feature together in main, as it's a big change. I will let you know if I find something else.

Amazing job!!

load-state-approve.mp4

@seaona seaona added QA Passed A successful QA run through has been done release-7.1.0 labels Jun 1, 2023
@brianacnguyen brianacnguyen dismissed stale reviews from ghost via ad07ba5 June 1, 2023 18:22
@bschorchit
Copy link

I agree with loading being a separate issue :)

@brianacnguyen brianacnguyen dismissed a stale review via ad07ba5 June 1, 2023 22:16
segun
segun approved these changes Jun 2, 2023
View reviewed changes
Copy link
Contributor

@segun segun left a comment

Choose a reason for hiding this comment

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

LGTM

@seaona seaona merged commit 549be0b into main Jun 2, 2023
14 of 15 checks passed
@seaona seaona deleted the ft/178-set-spend-cap branch June 2, 2023 09:25
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2023
@bschorchit
Copy link

I've changed the label from release-7.1.0 to release 7.2.0 as we want to make this change https://github.com/MetaMask/MetaMask-planning/issues/685 (same changes we're making on extension) before shipping this on mobile

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@segun segun segun approved these changes

@jpuri jpuri jpuri approved these changes

@digiwand digiwand digiwand approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels
QA Passed A successful QA run through has been done release-7.3.0 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.

None yet
6 participants
@blackdevelopa @seaona @bschorchit @jpuri @segun @digiwand