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"
}

packages/transaction-controller/src/TransactionController.ts

Gudahtt

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

packages/transaction-controller/src/TransactionController.ts

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

packages/transaction-controller/src/utils.ts

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

packages/transaction-controller/src/TransactionController.ts

packages/transaction-controller/src/TransactionController.test.ts

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

packages/network-controller/src/NetworkController.ts

 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

packages/transaction-controller/src/TransactionController.ts

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

packages/transaction-controller/src/TransactionController.ts

packages/transaction-controller/src/TransactionController.test.ts

Gudahtt

LGTM!

* add nonceTracker to TransactionsController

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

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

Gudahtt reviewed Apr 5, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 5, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 5, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 5, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 5, 2023

View reviewed changes

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from 1f288cd to 58737d6 Compare 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

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch 3 times, most recently from 29f8fb4 to feda1b9 Compare 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 f32cf8b Compare April 6, 2023 18:11

Gudahtt reviewed Apr 6, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

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

Gudahtt reviewed Apr 6, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

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 4abd631 Compare April 6, 2023 18:30

address feedback

2138d65

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

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

adonesky1 commented Apr 6, 2023

View reviewed changes

update comment

c73cf69

Gudahtt reviewed Apr 6, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.ts Show resolved Hide resolved

Gudahtt reviewed Apr 6, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.test.ts Show resolved Hide resolved

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

packages/transaction-controller/src/TransactionController.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 11, 2023

View reviewed changes

packages/transaction-controller/src/TransactionController.test.ts Show resolved Hide resolved

Gudahtt previously approved these changes Apr 11, 2023

View reviewed changes

address feedback

c2961a4

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

adonesky1 force-pushed the add-nonce-tracker-to-transactions-controller branch from a913cf4 to c2961a4 Compare 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 main Apr 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

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Add nonce tracker to transactions controller (#1147)

25e61d4

* add nonceTracker to TransactionsController

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

Add nonce tracker to transactions controller (#1147)

01a21d5

* add nonceTracker to TransactionsController

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-security bot 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-actions bot commented Apr 3, 2023

Uh oh!

adonesky1 commented Apr 3, 2023

Uh oh!

github-actions bot 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!

adonesky1 Apr 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!

legobeat Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

adonesky1 Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

legobeat Apr 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!

adonesky1 Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

adonesky1 Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

mcmire Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

adonesky1 commented Apr 10, 2023

Uh oh!

adonesky1 Apr 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-security bot commented Mar 28, 2023 •

edited

Loading

legobeat Apr 10, 2023 •

edited

Loading