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: Nonce too low error on Approve ERC20 and ERC721 transactions #6592

Merged
merged 3 commits into from
Jun 15, 2023
Merged

fix: Nonce too low error on Approve ERC20 and ERC721 transactions #6592

merged 3 commits into from
Jun 15, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Jun 13, 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
Approve Nonce issue was previously fixed in Nonce Too Low for Approve Transaction, but Custom Spend Allowance reintroduced the similar bug by repopulating the transaction proposed nonce (already used nonce value) when component mounts.

Screenshots/Recordings

Screen.Recording.2023-06-13.at.17.01.23.mov

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

Issue

Progresses #6537

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 changed the title Nonce too low on approve txn [FIX] Nonce too low error on Approve ERC20 and ERC721 transactions Jun 13, 2023
@blackdevelopa blackdevelopa changed the title [FIX] Nonce too low error on Approve ERC20 and ERC721 transactions fix Nonce too low error on Approve ERC20 and ERC721 transactions Jun 13, 2023
@blackdevelopa blackdevelopa marked this pull request as ready for review June 13, 2023 20:12
@blackdevelopa blackdevelopa requested a review from a team as a code owner June 13, 2023 20:12
@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 13, 2023
@blackdevelopa blackdevelopa self-assigned this Jun 13, 2023
@blackdevelopa blackdevelopa changed the title fix Nonce too low error on Approve ERC20 and ERC721 transactions fix: Nonce too low error on Approve ERC20 and ERC721 transactions Jun 13, 2023
NiranjanaBinoy
NiranjanaBinoy previously approved these changes Jun 13, 2023
View reviewed changes
@digiwand digiwand self-requested a review June 14, 2023 10:07
@sonarcloud
Copy link

sonarcloud bot commented Jun 14, 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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@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 14, 2023
@seaona
Copy link
Contributor

seaona commented Jun 14, 2023

hi @blackdevelopa ! I've been QAing this PR. I've seen the behaviour seems fixed, but, I've encountered several issues, around it, but I confirm they are also reproduceable on main. I cannot test further as keep encountering those issues. So I would say we can merge this PR and we could continue investigating the new issues separately.
Does it sound okay to you @blackdevelopa @digiwand @NiranjanaBinoy ? Pls let me know if otherwise

@seaona seaona 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 Jun 14, 2023
@seaona seaona merged commit 1f3a1a0 into main Jun 15, 2023
15 checks passed
@seaona seaona deleted the fix/6537 branch June 15, 2023 09:15
@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@digiwand digiwand digiwand approved these changes

@NiranjanaBinoy NiranjanaBinoy NiranjanaBinoy approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels
QA Passed A successful QA run through has been done release-7.2.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
4 participants
@blackdevelopa @seaona @digiwand @NiranjanaBinoy