-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8d1c2ac7-245b-46ce-9daf-31ef015e13b4 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
74bc60c
All transactions performed with disabled and enabled |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d28dc0f9-640c-45ac-9edd-e9768a931959 |
There was a problem hiding this 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?
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/cfb64b67-0220-4b9a-a339-330786d404b9 |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4995f517-7812-4f4e-8d80-9d88a18d19d9 |
Scenarios tested:
after_migr.txt |
@matthewwalsh0 Can you please confirm whether this PR is cc @sethkfman |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b70e69c6-78bb-4474-870c-18e2db039f6d |
Kudos, SonarCloud Quality Gate passed! |
Description
Use NonceTracker package
When sending transactions, the
nonce
value is currently generated using aneth_getTransactionCount
query to determine the next nonce according to the network, plus a caching layer in theNetworkController
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 theNonceTracker
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, theNonceTracker
usage now explicitly ignores incoming transfer transactions using theisTransfer
property.Synchronise custom nonce logic
The
NonceTracker
is now also used to propose nonces whenCustomize transaction nonce
is enabled, and the related UI logic now utilises theTransactionController
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: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:
Reset Account
after sending a transaction with a higher nonce.Verify the correct
submitHistory
is now added to state logs after the following scenarios:Reset account
button.Screenshots/Recordings
Before
After
[Pending]
Pre-merge author checklist
Pre-merge reviewer checklist