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

refactor: generate nonces using nonce tracker #7689

Merged
merged 10 commits into from
Nov 24, 2023

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Nov 5, 2023

Description

Use NonceTracker package

When sending transactions, the nonce value is currently generated using an eth_getTransactionCount query to determine the next nonce according to the network, plus a caching layer in the NetworkController which intercepts this query result to accommodate any pending transactions sent from the wallet.

This PR replaces all of the above and generates the nonce value using only the NonceTracker package, which internally continues to check the network nonce, and also factor pending transactions but by explicitly checking them, rather than relying on a cached value.

This same package is used in the extension.

Following the previous bug concerning incorrect nonce values, the NonceTracker usage now explicitly ignores incoming transfer transactions using the isTransfer property.

Synchronise custom nonce logic

The NonceTracker is now also used to propose nonces when Customize transaction nonce is enabled, and the related UI logic now utilises the TransactionController to ensure this logic remains synchronised in future.

Generate submit history

Finally, as a means to protect the user following any nonce issues in future, a submitHistory array is now automatically populated whenever a transaction is successfully submitted.

This records the following data per transaction in order to support debugging efforts even after Reset account has been used:

  • Chain ID
  • Transaction Hash
  • Network Type
  • Network URL
  • Origin
  • Raw Transaction
  • Time
  • Transaction Parameters

This history is currently limited to the last 100 submitted transactions.

A migration has been added to generate this information from existing wallets using the transaction metadata.

Related issues

Fixes: #7673

Manual testing steps

Full transaction regression.

Additional focus on all nonce edge cases including:

  • Presence of incoming transfer transaction with higher nonces.
  • Presence of standard incoming transactions.
  • Using Reset Account after sending a transaction with a higher nonce.
  • Cancelling a transaction with a high nonce then sending a normal transaction.
  • Sending multiple pending transactions.
  • Switching networks during pending transactions and after confirmed transactions.
  • Using custom nonce values.

Verify the correct submitHistory is now added to state logs after the following scenarios:

  • Use of alternate networks.
  • Use of Reset account button.
  • Upgrade from previous versions using existing transaction state.

Screenshots/Recordings

Before

After

[Pending]

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@matthewwalsh0 matthewwalsh0 requested a review from a team as a code owner November 5, 2023 13:56
@matthewwalsh0 matthewwalsh0 added the team-confirmations-system DEPRECATED: please use "team-confirmations" label instead label Nov 5, 2023
Copy link
Contributor

github-actions bot commented Nov 5, 2023

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8d1c2ac7-245b-46ce-9daf-31ef015e13b4
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0163b8b) 37.39% compared to head (c100488) 37.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7689      +/-   ##
==========================================
+ Coverage   37.39%   37.44%   +0.05%     
==========================================
  Files        1052     1052              
  Lines       28175    28193      +18     
  Branches     2517     2517              
==========================================
+ Hits        10536    10557      +21     
+ Misses      17040    17037       -3     
  Partials      599      599              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OGPoyraz
OGPoyraz previously approved these changes Nov 6, 2023
vinistevam
vinistevam previously approved these changes Nov 6, 2023
@sleepytanya
Copy link
Contributor

sleepytanya commented Nov 7, 2023

Dev build iOS:

  1. Customize transaction nonce is disabled
  2. Send approve transaction from the mobile wallet to the extension wallet - nonce 45
  3. Send transferFrom transactions from extension wallet - nonce 65
  4. Customize transaction nonce is enabled
  5. Send approve transaction from the mobile wallet - nonce 70 (stuck transaction)
65-70.mov
  1. Cancel transaction with the high nonce fails, speed up the transaction with high nonce seems to work but tx remains stuck
  2. Send transferFrom transaction from the extension wallet - nonce 66 (confirmed transaction)
70-66.mov
Screenshot 2023-11-07 at 12 07 34 AM

@sleepytanya
Copy link
Contributor

sleepytanya commented Nov 7, 2023

Performing reset account after stuck transaction with high nonce (and subsequent transferFrom transaction) leaves only first transaction (nonce 45) in the mobile incoming transactions history (3 missing txs including successfully confirmed with nonces 65 and 66):
Screenshot 2023-11-07 at 12 16 03 AM

All successful transactions (45,65 and 66) are in the state log:

Screenshot 2023-11-07 at 12 25 13 AM

@sleepytanya
Copy link
Contributor

sleepytanya commented Nov 7, 2023

Token transactions sent using transferFrom to mobile account (67, 68) are not detected in the incoming transaction history:
Screenshot 2023-11-07 at 12 38 02 AM
Regular send transactions detected successfully:
Screenshot 2023-11-07 at 12 54 56 AM

@sleepytanya
Copy link
Contributor

  1. Enable customize tx nonce after reseting the account
  2. Send transferFrom tx with the higher nonce to the mobile account (nonce 68)
  3. Send approve transaction from the mobile account - transactions nonce is 81 (stuck transaction)
  4. Try to send another transaction (regular eth send) from the mobile (to have multiple stuck transactions)
  5. New transaction has the same nonce and replaces previous stuck transaction:
81.mov
Screenshot 2023-11-07 at 1 10 21 AM

@sleepytanya
Copy link
Contributor

All transactions performed with disabled and enabled customize tx nonce were confirmed successfully.
Screenshot 2023-11-08 at 11 57 21 AM
State log file:
fixed.txt

@sleepytanya sleepytanya added the QA Passed A successful QA run through has been done label Nov 8, 2023
@matthewwalsh0 matthewwalsh0 marked this pull request as draft November 13, 2023 11:18
@matthewwalsh0 matthewwalsh0 marked this pull request as ready for review November 13, 2023 13:51
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d28dc0f9-640c-45ac-9edd-e9768a931959
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

Just have a little curious question, since I didn't debug it:

On the nonce-tracker library, we can also have the nonce details when we use the getNonceLock method

Could we have some advantage of what is returned there on the future?

app/util/networks/index.js Show resolved Hide resolved
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/cfb64b67-0220-4b9a-a339-330786d404b9
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@matthewwalsh0 matthewwalsh0 removed the QA Passed A successful QA run through has been done label Nov 14, 2023
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4995f517-7812-4f4e-8d80-9d88a18d19d9
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@sleepytanya
Copy link
Contributor

sleepytanya commented Nov 15, 2023

Scenarios tested:

  1. correct submitHistory is added to theState log after use of alternate networks:
    before_network.txt
    after_network.txt

  2. correct submitHistory is added to the State log after use of 'Delete wallet':
    before_reset.txt
    after_reset.txt

  3. correct submitHistory is added to the State log after migration:
    before_migr.txt

after_migr.txt
4. Stuck multiple pending transaction with custom nonce on and off
5. Performing transaction with higher and lower nonce with custom nonce enabled
6. Normal Eth send with the presence higher/lower nonce transactions with custom nonce disabled and enabled
7. Cancelling/speeding up transactions
8. Switching networks while having submitted/confirmed/queued transactions
9. Perform transferFrom transactions with higher nonce and then perform any transactions with lower nonce (with custom nonce enabled and disabled)
10. Using custom nonce

@hesterbruikman
Copy link
Contributor

@matthewwalsh0 Can you please confirm whether this PR is QA passed?

cc @sethkfman

Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b70e69c6-78bb-4474-870c-18e2db039f6d
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Copy link

sonarcloud bot commented Nov 24, 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 1 Code Smell

97.3% 97.3% Coverage
0.0% 0.0% Duplication

@matthewwalsh0 matthewwalsh0 merged commit 13acaa7 into main Nov 24, 2023
52 checks passed
@matthewwalsh0 matthewwalsh0 deleted the refactor/nonce-tracker branch November 24, 2023 17:17
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2023
@metamaskbot metamaskbot added the release-7.13.0 Issue or pull request that will be included in release 7.13.0 label Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.13.0 Issue or pull request that will be included in release 7.13.0 team-confirmations-system DEPRECATED: please use "team-confirmations" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Reset Account is not clearing the nonce
8 participants