Support custom nonce value (#3579)

Encapsulate all nonce logic in a new file with additional logging.
Update transaction metadata after approval if the approval result includes the value.txMeta property.
Add type to incoming transactions.

Author: matthewwalsh0

packages/transaction-controller/jest.config.js

@@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 88.78,
-      functions: 94.16,
-      lines: 98.07,
-      statements: 98.03,
+      branches: 89.18,
+      functions: 94.18,
+      lines: 98.09,
+      statements: 98.05,
     },
   },
 

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

@@ -213,7 +213,7 @@ function buildMockMessenger({
 
   if (delay) {
     promise = new Promise((res, rej) => {
-      approve = () => res({ resultCallbacks });
+      approve = (value?: any) => res({ resultCallbacks, value });
       reject = rej;
     });
   }
@@ -237,7 +237,7 @@ function buildMockMessenger({
 
   return {
     messenger,
-    approve: approve as unknown as () => void,
+    approve: approve as unknown as (value?: any) => void,
     reject: reject as unknown as (reason: unknown) => void,
   };
 }
@@ -454,7 +454,7 @@ describe('TransactionController', () => {
   let messengerMock: TransactionControllerMessenger;
   let rejectMessengerMock: TransactionControllerMessenger;
   let delayMessengerMock: TransactionControllerMessenger;
-  let approveTransaction: () => void;
+  let approveTransaction: (value?: any) => void;
   let getNonceLockSpy: jest.Mock;
   let incomingTransactionHelperMock: jest.Mocked<IncomingTransactionHelper>;
   let pendingTransactionTrackerMock: jest.Mocked<PendingTransactionTracker>;
@@ -1342,6 +1342,29 @@ describe('TransactionController', () => {
         expect(resultCallbacksMock.error).toHaveBeenCalledTimes(1);
       });
 
+      it('updates transaction if approval result includes updated metadata', async () => {
+        const controller = newController();
+
+        const { result } = await controller.addTransaction({
+          from: ACCOUNT_MOCK,
+          to: ACCOUNT_MOCK,
+        });
+
+        const transaction = controller.state.transactions[0];
+
+        approveTransaction({
+          txMeta: { ...transaction, customNonceValue: '123' },
+        });
+
+        await result;
+
+        expect(controller.state.transactions).toStrictEqual([
+          expect.objectContaining({
+            customNonceValue: '123',
+          }),
+        ]);
+      });
+
       describe('fails', () => {
         /**
          * Test template to assert adding and submitting a transaction fails.

packages/transaction-controller/src/TransactionController.ts

@@ -66,13 +66,16 @@ import {
   addInitialHistorySnapshot,
   updateTransactionHistory,
 } from './utils/history';
+import {
+  getAndFormatTransactionsForNonceTracker,
+  getNextNonce,
+} from './utils/nonce';
 import {
   updatePostTransactionBalance,
   updateSwapsTransaction,
 } from './utils/swaps';
 import { determineTransactionType } from './utils/transaction-type';
 import {
-  getAndFormatTransactionsForNonceTracker,
   getIncreasedPriceFromExisting,
   normalizeTxParams,
   isEIP1559Transaction,
@@ -1882,6 +1885,26 @@ export class TransactionController extends BaseControllerV1<
               resultCallbacks = undefined;
             });
           }
+
+          const approvalValue = acceptResult.value as
+            | {
+                txMeta?: TransactionMeta;
+              }
+            | undefined;
+
+          const updatedTransaction = approvalValue?.txMeta;
+
+          if (updatedTransaction) {
+            log('Updating transaction with approval data', {
+              customNonce: updatedTransaction.customNonceValue,
+              params: updatedTransaction.txParams,
+            });
+
+            this.updateTransaction(
+              updatedTransaction,
+              'TransactionController#processApproval - Updated with approval data',
+            );
+          }
         }
 
         const { isCompleted: isTxCompleted } =
@@ -1951,10 +1974,13 @@ export class TransactionController extends BaseControllerV1<
     const chainId = this.getChainId();
     const index = transactions.findIndex(({ id }) => transactionId === id);
     const transactionMeta = transactions[index];
+
     const {
-      txParams: { nonce, from },
+      txParams: { from },
     } = transactionMeta;
-    let nonceLock;
+
+    let releaseNonceLock: (() => void) | undefined;
+
     try {
       if (!this.sign) {
         releaseLock();
@@ -1974,16 +2000,15 @@ export class TransactionController extends BaseControllerV1<
         return;
       }
 
-      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) {
-        nonceLock = await this.nonceTracker.getNonceLock(from);
-        nonceToUse = addHexPrefix(nonceLock.nextNonce.toString(16));
-      }
+      const [nonce, releaseNonce] = await getNextNonce(
+        transactionMeta,
+        this.nonceTracker,
+      );
+
+      releaseNonceLock = releaseNonce;
 
       transactionMeta.status = TransactionStatus.approved;
-      transactionMeta.txParams.nonce = nonceToUse;
+      transactionMeta.txParams.nonce = nonce;
       transactionMeta.txParams.chainId = chainId;
 
       const baseTxParams = {
@@ -2057,9 +2082,7 @@ export class TransactionController extends BaseControllerV1<
     } finally {
       this.inProcessOfSigning.delete(transactionId);
       // must set transaction to submitted/failed before releasing lock
-      if (nonceLock) {
-        nonceLock.releaseLock();
-      }
+      releaseNonceLock?.();
       releaseLock();
     }
   }

packages/transaction-controller/src/helpers/EtherscanRemoteTransactionSource.test.ts

@@ -1,7 +1,7 @@
 import { v1 as random } from 'uuid';
 
 import { CHAIN_IDS } from '../constants';
-import { TransactionStatus } from '../types';
+import { TransactionStatus, TransactionType } from '../types';
 import type {
   EtherscanTokenTransactionMeta,
   EtherscanTransactionMeta,
@@ -116,6 +116,7 @@ const EXPECTED_NORMALISED_TRANSACTION_BASE = {
     to: ETHERSCAN_TRANSACTION_SUCCESS_MOCK.to,
     value: '0xb1a2bc2ec50000',
   },
+  type: TransactionType.incoming,
   verifiedOnBlockchain: false,
 };
 

packages/transaction-controller/src/helpers/EtherscanRemoteTransactionSource.ts

@@ -10,7 +10,7 @@ import type {
   RemoteTransactionSourceRequest,
   TransactionMeta,
 } from '../types';
-import { TransactionStatus } from '../types';
+import { TransactionStatus, TransactionType } from '../types';
 import {
   fetchEtherscanTokenTransactions,
   fetchEtherscanTransactions,
@@ -177,6 +177,7 @@ export class EtherscanRemoteTransactionSource
         to: txMeta.to,
         value: BNToHex(new BN(txMeta.value)),
       },
+      type: TransactionType.incoming,
       verifiedOnBlockchain: false,
     };
   }

packages/transaction-controller/src/types.ts

@@ -105,6 +105,9 @@ type TransactionMetaBase = {
    */
   custodyStatus?: string;
 
+  /** The optional custom nonce override as a decimal string. */
+  customNonceValue?: string;
+
   /**
    * The custom token amount is the amount set by the user.
    */

packages/transaction-controller/src/utils/nonce.test.ts

@@ -0,0 +1,186 @@
+import type {
+  NonceTracker,
+  Transaction as NonceTrackerTransaction,
+} from 'nonce-tracker';
+
+import type { TransactionMeta } from '../types';
+import { TransactionStatus } from '../types';
+import { getAndFormatTransactionsForNonceTracker, getNextNonce } from './nonce';
+
+const TRANSACTION_META_MOCK: TransactionMeta = {
+  chainId: '0x1',
+  id: 'testId1',
+  status: TransactionStatus.unapproved,
+  time: 1,
+  txParams: {
+    from: '0x1',
+  },
+};
+
+/**
+ * Creates a mock instance of a nonce tracker.
+ * @returns The mock instance.
+ */
+function createNonceTrackerMock(): jest.Mocked<NonceTracker> {
+  return { getNonceLock: jest.fn() } as any;
+}
+
+describe('nonce', () => {
+  describe('getNextNonce', () => {
+    it('returns custom nonce if provided', async () => {
+      const transactionMeta = {
+        ...TRANSACTION_META_MOCK,
+        customNonceValue: '123',
+      };
+
+      const nonceTracker = createNonceTrackerMock();
+
+      const [nonce, releaseLock] = await getNextNonce(
+        transactionMeta,
+        nonceTracker,
+      );
+
+      expect(nonce).toBe('0x7b');
+      expect(releaseLock).toBeUndefined();
+    });
+
+    it('returns existing nonce if provided and no custom nonce', async () => {
+      const transactionMeta = {
+        ...TRANSACTION_META_MOCK,
+        txParams: {
+          ...TRANSACTION_META_MOCK.txParams,
+          nonce: '0x123',
+        },
+      };
+
+      const nonceTracker = createNonceTrackerMock();
+
+      const [nonce, releaseLock] = await getNextNonce(
+        transactionMeta,
+        nonceTracker,
+      );
+
+      expect(nonce).toBe('0x123');
+      expect(releaseLock).toBeUndefined();
+    });
+
+    it('returns next nonce from tracker if no custom nonce and no existing nonce', async () => {
+      const transactionMeta = {
+        ...TRANSACTION_META_MOCK,
+        txParams: {
+          ...TRANSACTION_META_MOCK.txParams,
+        },
+      };
+
+      const nonceTracker = createNonceTrackerMock();
+      const releaseLock = jest.fn();
+
+      nonceTracker.getNonceLock.mockResolvedValueOnce({
+        nextNonce: 456,
+        releaseLock,
+      } as any);
+
+      const [nonce, resultReleaseLock] = await getNextNonce(
+        transactionMeta,
+        nonceTracker,
+      );
+
+      expect(nonce).toBe('0x1c8');
+
+      resultReleaseLock?.();
+
+      expect(releaseLock).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('getAndFormatTransactionsForNonceTracker', () => {
+    it('returns formatted transactions filtered by chain, from, isTransfer, and status', () => {
+      const fromAddress = '0x123';
+      const inputTransactions: TransactionMeta[] = [
+        {
+          id: '1',
+          chainId: '0x1',
+          time: 123456,
+          txParams: {
+            from: fromAddress,
+            gas: '0x100',
+            value: '0x200',
+            nonce: '0x1',
+          },
+          status: TransactionStatus.confirmed,
+        },
+        {
+          id: '2',
+          chainId: '0x1',
+          time: 123457,
+          txParams: {
+            from: '0x124',
+            gas: '0x101',
+            value: '0x201',
+            nonce: '0x2',
+          },
+          status: TransactionStatus.submitted,
+        },
+        {
+          id: '3',
+          chainId: '0x1',
+          time: 123458,
+          txParams: {
+            from: fromAddress,
+            gas: '0x102',
+            value: '0x202',
+            nonce: '0x3',
+          },
+          status: TransactionStatus.approved,
+        },
+        {
+          id: '4',
+          chainId: '0x2',
+          time: 123459,
+          txParams: {
+            from: fromAddress,
+            gas: '0x103',
+            value: '0x203',
+            nonce: '0x4',
+          },
+          status: TransactionStatus.confirmed,
+        },
+        {
+          id: '5',
+          chainId: '0x2',
+          isTransfer: true,
+          time: 123460,
+          txParams: {
+            from: fromAddress,
+            gas: '0x104',
+            value: '0x204',
+            nonce: '0x5',
+          },
+          status: TransactionStatus.confirmed,
+        },
+      ];
+
+      const expectedResult: NonceTrackerTransaction[] = [
+        {
+          status: TransactionStatus.confirmed,
+          history: [{}],
+          txParams: {
+            from: fromAddress,
+            gas: '0x103',
+            value: '0x203',
+            nonce: '0x4',
+          },
+        },
+      ];
+
+      const result = getAndFormatTransactionsForNonceTracker(
+        '0x2',
+        fromAddress,
+        TransactionStatus.confirmed,
+        inputTransactions,
+      );
+
+      expect(result).toStrictEqual(expectedResult);
+    });
+  });
+});