Add nonce tracker to transactions controller by adonesky1 · Pull Request #1147 · MetaMask/core

adonesky1 · 2023-03-28T20:55:53Z

Resolves: #1070

ADDED
- Adds NonceTracker package to transaction approval flow to manage transaction nonces. This functionality was previously handled by middleware in `web3-provider-engine which is slated to be removed. #123

socket-security · 2023-03-28T20:57:36Z

New dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package	Capability Access	`+/-` Transitive Count	Publisher
nonce-tracker@1.1.0	None	`+5`	rekmarks

adonesky1 · 2023-04-03T17:59:35Z

@metamaskbot publish-preview

github-actions · 2023-04-03T18:04:58Z

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.

{
"@metamask/address-book-controller": "2.0.0-preview.a96d64c",
"@metamask/announcement-controller": "3.0.0-preview.a96d64c",
"@metamask/approval-controller": "2.0.0-preview.a96d64c",
"@metamask/assets-controllers": "5.0.0-preview.a96d64c",
"@metamask/base-controller": "2.0.0-preview.a96d64c",
"@metamask/composable-controller": "2.0.0-preview.a96d64c",
"@metamask/controller-utils": "3.0.0-preview.a96d64c",
"@metamask/ens-controller": "2.0.0-preview.a96d64c",
"@metamask/gas-fee-controller": "4.0.0-preview.a96d64c",
"@metamask/keyring-controller": "4.0.0-preview.a96d64c",
"@metamask/message-manager": "2.0.0-preview.a96d64c",
"@metamask/network-controller": "5.0.0-preview.a96d64c",
"@metamask/notification-controller": "2.0.0-preview.a96d64c",
"@metamask/permission-controller": "3.0.0-preview.a96d64c",
"@metamask/phishing-controller": "3.0.0-preview.a96d64c",
"@metamask/preferences-controller": "2.1.0-preview.a96d64c",
"@metamask/rate-limit-controller": "2.0.0-preview.a96d64c",
"@metamask/subject-metadata-controller": "2.0.0-preview.a96d64c",
"@metamask/transaction-controller": "4.0.0-preview.a96d64c"
}

adonesky1 · 2023-04-03T23:12:12Z

@metamaskbot publish-preview

github-actions · 2023-04-03T23:16:41Z

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.

{
"@metamask/address-book-controller": "2.0.0-preview.1f288cd",
"@metamask/announcement-controller": "3.0.0-preview.1f288cd",
"@metamask/approval-controller": "2.0.0-preview.1f288cd",
"@metamask/assets-controllers": "5.0.1-preview.1f288cd",
"@metamask/base-controller": "2.0.0-preview.1f288cd",
"@metamask/composable-controller": "2.0.0-preview.1f288cd",
"@metamask/controller-utils": "3.1.0-preview.1f288cd",
"@metamask/ens-controller": "3.0.0-preview.1f288cd",
"@metamask/gas-fee-controller": "4.0.1-preview.1f288cd",
"@metamask/keyring-controller": "4.0.0-preview.1f288cd",
"@metamask/message-manager": "2.1.0-preview.1f288cd",
"@metamask/network-controller": "6.0.0-preview.1f288cd",
"@metamask/notification-controller": "2.0.0-preview.1f288cd",
"@metamask/permission-controller": "3.1.0-preview.1f288cd",
"@metamask/phishing-controller": "3.0.0-preview.1f288cd",
"@metamask/preferences-controller": "3.0.0-preview.1f288cd",
"@metamask/rate-limit-controller": "2.0.0-preview.1f288cd",
"@metamask/subject-metadata-controller": "2.0.0-preview.1f288cd",
"@metamask/transaction-controller": "4.0.1-preview.1f288cd"
}

Gudahtt

Great work! Left some minor comments but generally this all looks correct.

adonesky1 · 2023-04-06T18:56:56Z

+ * @param transactions - Array of transactionMeta objects that have been prefiltered.
+ * @returns Array of transactions formatted for the nonce tracker.
+ */
+export function getAndFormatTransactionsForNonceTracker(


I opted to make this a utility function rather than a private method on the TransactionsController just so testing it made more sense... happy to move it to the controller if we think that makes more sense.

Gudahtt · 2023-04-06T22:01:11Z

Looks great! CI is failing due to a drop in test coverage though.

adonesky1 · 2023-04-06T22:16:22Z

CI is failing due to a drop in test coverage though.

Yeah been mainly focusing on validating it works in mobile and still fighting the env. Also not totally sure how best to target these lines:

Gudahtt · 2023-04-06T22:25:24Z

Ah, for those lines I'd suggest we use a real nonce tracker instead of a mock one. We can mock the provider passed into the nonce tracker instead. Then those lines will be tested as part of the tests you've already written

legobeat · 2023-04-10T09:01:56Z

 type BlockTracker = any;

-type BlockTrackerProxy = SwappableProxy<BlockTracker>;
+export type BlockTrackerProxy = SwappableProxy<BlockTracker>;


As this is now an exported type, doesn't it warrant constraining BlockTracker to something more specific than any?

Perhaps! But I don't believe that's appropriately scoped to this PR. As you can see directly above this we are doing the same with Provider:
type Provider = any; export type ProviderProxy = SwappableProxy<Provider>;

How does the type of Provider reduce the scope of this PR?
The impacts of the type being any is different for public and private interfaces. A type which is acceptable as any internally may require constraining before being appropriate for exposure.

How does the type of Provider reduce the scope of this PR?
On that point I was just noting that we're already exporting the ProviderProxy object with a Provider type parameter that is also typed as any.

Currently the BlockTracker is set here, where it is a property on the Provider. I believe there is an intention to follow back here and improve typing soon. cc @Gudahtt @mcmire
If we think it makes sense to add the types for both Provider and BlockTracker in this PR I'm open to that.

Yes, that's right. We're using any at the moment because web3-provider-engine doesn't provide types at all, and we're about to replace it anyway (see #1116). Once the replacement is complete, then we can just use the correct types from @metamask/eth-json-rpc-provider and eth-block-tracker.

adonesky1 · 2023-04-10T19:49:23Z

Ah, for those lines I'd suggest we use a real nonce tracker instead of a mock one. We can mock the provider passed into the nonce tracker instead. Then those lines will be tested as part of the tests you've already written

done here: 9f2662e

@Gudahtt

adonesky1 · 2023-04-10T19:58:30Z

+ let nonceToUse = nonce;
+ // if a nonce already exists on the transactionMeta it means this is a speedup or cancel transaction
+ // so we want to reuse that nonce and hope that it beats the previous attempt to chain. Otherwise use a new locked nonce
+ if (!nonceToUse) {


@Gudahtt I'm realizing that this isn't actually necessary in the current implementation since in this TransactionController we don't actually re-process/re-approve speedup and cancel transactions in this method as we do in the extension TransactionController. Rather we just modify the transaction and re-broadcast it directly in the speedup/cancel method, for instance here.
That said it may make sense to leave this logic to ensure correct nonce management in future uses of this method. And as it is currently there is no issue keeping this logic since nonceToUse should always be undefined on line 680.

Gudahtt

LGTM!

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from 0e048e4 to f503244Compare March 30, 2023 17:49

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from a96d64c to 1f288cdCompare April 3, 2023 22:25

Gudahtt reviewed Apr 5, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

Gudahtt reviewed Apr 5, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

Gudahtt reviewed Apr 5, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

Gudahtt reviewed Apr 5, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

Gudahtt reviewed Apr 5, 2023
View reviewed changes

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from 1f288cd to 58737d6Compare April 5, 2023 16:21

adonesky1 marked this pull request as ready for review April 5, 2023 18:16

adonesky1 requested a review from a team as a code owner April 5, 2023 18:16

adonesky1 requested a review from Gudahtt April 5, 2023 18:16

mcmire reviewed Apr 5, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated
Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated
Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch 3 times, most recently from 29f8fb4 to feda1b9Compare April 6, 2023 17:59

adonesky1 requested a review from mcmire April 6, 2023 17:59

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from feda1b9 to f32cf8bCompare April 6, 2023 18:11

Gudahtt reviewed Apr 6, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from f32cf8b to 28a00daCompare April 6, 2023 18:14

Gudahtt reviewed Apr 6, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

adonesky1 added 2 commits April 6, 2023 13:15

add nonceTracker to TransactionsController
5679f2b

cleanup
4abd631

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from 28a00da to 4abd631Compare April 6, 2023 18:30

address feedback
2138d65

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from 4abd631 to f32cf8bCompare April 6, 2023 18:53

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from f32cf8b to 2138d65Compare April 6, 2023 18:54

adonesky1 commented Apr 6, 2023
View reviewed changes

update comment
c73cf69

Gudahtt reviewed Apr 6, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts

Gudahtt reviewed Apr 6, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.test.ts

legobeat reviewed Apr 10, 2023
View reviewed changes

modify mocking strategy
9f2662e

adonesky1 requested review from Gudahtt and legobeat April 10, 2023 19:31

cleanup
96c406d

adonesky1 commented Apr 10, 2023
View reviewed changes

Gudahtt reviewed Apr 11, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.ts Outdated

Gudahtt reviewed Apr 11, 2023
View reviewed changes

Comment threadpackages/transaction-controller/src/TransactionController.test.ts Outdated

Gudahtt previously approved these changes Apr 11, 2023
View reviewed changes

address feedback
c2961a4

adonesky1 dismissed Gudahtt’s stale review via c2961a4April 11, 2023 15:33

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from a913cf4 to c2961a4Compare April 11, 2023 15:33

Gudahtt approved these changes Apr 11, 2023
View reviewed changes

Merge branch 'main' into add-nonce-tracker-to-transactions-controller
943b0b1

adonesky1 merged commit e64e181 into mainApr 11, 2023

adonesky1 deleted the add-nonce-tracker-to-transactions-controller branch April 11, 2023 15:57

mcmire mentioned this pull request Apr 11, 2023
Investigate function impact of replacing web3-provider-engine #1065
Closed

legobeat mentioned this pull request Apr 25, 2023
Release 49.0.0 #1263
Merged

Uh oh!

Uh oh!

Conversation

adonesky1 commented Mar 28, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-securityBot commented Mar 28, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adonesky1 commented Apr 3, 2023

Uh oh!

github-actionsBot commented Apr 3, 2023

Uh oh!

adonesky1 commented Apr 3, 2023

Uh oh!

github-actionsBot commented Apr 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adonesky1Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Gudahtt commented Apr 6, 2023

Uh oh!

adonesky1 commented Apr 6, 2023

Uh oh!

Gudahtt commented Apr 6, 2023

Uh oh!

legobeatApr 10, 2023

Choose a reason for hiding this comment

Uh oh!

adonesky1Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

legobeatApr 10, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adonesky1Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

adonesky1Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

mcmireApr 11, 2023

Choose a reason for hiding this comment

Uh oh!

adonesky1 commented Apr 10, 2023

Uh oh!

adonesky1Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adonesky1 commented Mar 28, 2023•
edited
Loading

socket-securityBot commented Mar 28, 2023•
edited
Loading

legobeatApr 10, 2023•
edited
Loading