feat: update nonce of existing transaction in batch (#6528)

## Explanation

When dynamically converting an existing transaction to a transaction
batch via the `batchTransactions` property, automatically update the
nonce and re-sign the existing transaction if it is not the first
transaction in the batch.

## References

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

Author: matthewwalsh0

packages/transaction-controller/CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Update nonce of existing transaction if converted to batch via `batchTransactions` but not first transaction ([#6528](https://github.com/MetaMask/core/pull/6528))
 - Bump `@metamask/base-controller` from `^8.2.0` to `^8.3.0` ([#6465](https://github.com/MetaMask/core/pull/6465))
 
 ## [60.2.0]

packages/transaction-controller/src/TransactionController.ts

@@ -1090,6 +1090,7 @@ export class TransactionController extends BaseController<
         transactionMeta: TransactionMeta,
       ) => this.#publishTransaction(ethQuery, transactionMeta) as Promise<Hex>,
       request,
+      signTransaction: this.#signTransaction.bind(this),
       update: this.update.bind(this),
       updateTransaction: this.#updateTransactionInternal.bind(this),
     });

packages/transaction-controller/src/hooks/CollectPublishHook.test.ts

@@ -1,3 +1,5 @@
+import { noop } from 'lodash';
+
 import { CollectPublishHook } from './CollectPublishHook';
 import type { TransactionMeta } from '..';
 import { flushPromises } from '../../../../tests/helpers';
@@ -10,21 +12,43 @@ const ERROR_MESSAGE_MOCK = 'Test error';
 
 const TRANSACTION_META_MOCK = {
   id: '123-456',
+  txParams: {
+    nonce: '0x1',
+  },
+} as TransactionMeta;
+
+const TRANSACTION_META_2_MOCK = {
+  id: '123-457',
+  txParams: {
+    nonce: '0x2',
+  },
 } as TransactionMeta;
 
 describe('CollectPublishHook', () => {
   describe('getHook', () => {
-    it('returns function that resolves ready promise', async () => {
+    it('resolves ready promise', async () => {
       const collectHook = new CollectPublishHook(2);
       const publishHook = collectHook.getHook();
 
-      publishHook(TRANSACTION_META_MOCK, SIGNED_TX_MOCK).catch(() => {
-        // Intentionally empty
-      });
+      publishHook(TRANSACTION_META_MOCK, SIGNED_TX_MOCK).catch(noop);
+      publishHook(TRANSACTION_META_2_MOCK, SIGNED_TX_2_MOCK).catch(noop);
 
-      publishHook(TRANSACTION_META_MOCK, SIGNED_TX_2_MOCK).catch(() => {
-        // Intentionally empty
-      });
+      await flushPromises();
+
+      const result = await collectHook.ready();
+
+      expect(result.signedTransactions).toStrictEqual([
+        SIGNED_TX_MOCK,
+        SIGNED_TX_2_MOCK,
+      ]);
+    });
+
+    it('resolves ready promise with signatures in nonce order', async () => {
+      const collectHook = new CollectPublishHook(2);
+      const publishHook = collectHook.getHook();
+
+      publishHook(TRANSACTION_META_2_MOCK, SIGNED_TX_2_MOCK).catch(noop);
+      publishHook(TRANSACTION_META_MOCK, SIGNED_TX_MOCK).catch(noop);
 
       await flushPromises();
 
@@ -48,10 +72,33 @@ describe('CollectPublishHook', () => {
       );
 
       const publishPromise2 = publishHook(
-        TRANSACTION_META_MOCK,
+        TRANSACTION_META_2_MOCK,
+        SIGNED_TX_2_MOCK,
+      );
+
+      collectHook.success([TRANSACTION_HASH_MOCK, TRANSACTION_HASH_2_MOCK]);
+
+      const result1 = await publishPromise1;
+      const result2 = await publishPromise2;
+
+      expect(result1.transactionHash).toBe(TRANSACTION_HASH_MOCK);
+      expect(result2.transactionHash).toBe(TRANSACTION_HASH_2_MOCK);
+    });
+
+    it('resolves all publish promises in nonce order', async () => {
+      const collectHook = new CollectPublishHook(2);
+      const publishHook = collectHook.getHook();
+
+      const publishPromise2 = publishHook(
+        TRANSACTION_META_2_MOCK,
         SIGNED_TX_2_MOCK,
       );
 
+      const publishPromise1 = publishHook(
+        TRANSACTION_META_MOCK,
+        SIGNED_TX_MOCK,
+      );
+
       collectHook.success([TRANSACTION_HASH_MOCK, TRANSACTION_HASH_2_MOCK]);
 
       const result1 = await publishPromise1;

packages/transaction-controller/src/hooks/CollectPublishHook.ts

@@ -1,5 +1,6 @@
 import type { DeferredPromise, Hex } from '@metamask/utils';
 import { createDeferredPromise, createModuleLogger } from '@metamask/utils';
+import { sortBy } from 'lodash';
 
 import { projectLogger } from '../logger';
 import type { PublishHook, PublishHookResult, TransactionMeta } from '../types';
@@ -15,18 +16,19 @@ export type CollectPublishHookResult = {
  * Used by batch transactions to publish multiple transactions at once.
  */
 export class CollectPublishHook {
-  readonly #publishPromises: DeferredPromise<PublishHookResult>[];
-
-  readonly #signedTransactions: Hex[];
+  #results: {
+    nonce: number;
+    promise: DeferredPromise<PublishHookResult>;
+    signedTransaction: Hex;
+  }[];
 
   readonly #transactionCount: number;
 
   readonly #readyPromise: DeferredPromise<CollectPublishHookResult>;
 
   constructor(transactionCount: number) {
-    this.#publishPromises = [];
     this.#readyPromise = createDeferredPromise();
-    this.#signedTransactions = [];
+    this.#results = [];
     this.#transactionCount = transactionCount;
   }
 
@@ -56,39 +58,54 @@ export class CollectPublishHook {
       throw new Error('Transaction hash count mismatch');
     }
 
-    for (let i = 0; i < this.#publishPromises.length; i++) {
-      const publishPromise = this.#publishPromises[i];
+    for (let i = 0; i < this.#results.length; i++) {
+      const result = this.#results[i];
       const transactionHash = transactionHashes[i];
 
-      publishPromise.resolve({ transactionHash });
+      result.promise.resolve({ transactionHash });
     }
   }
 
   error(error: unknown) {
     log('Error', { error });
 
-    for (const publishPromise of this.#publishPromises) {
-      publishPromise.reject(error);
+    for (const result of this.#results) {
+      result.promise.reject(error);
     }
   }
 
   #hook(
     transactionMeta: TransactionMeta,
     signedTx: string,
   ): Promise<PublishHookResult> {
-    this.#signedTransactions.push(signedTx as Hex);
+    const nonceHex = transactionMeta.txParams.nonce ?? '0x0';
+    const nonceDecimal = parseInt(nonceHex, 16);
 
-    log('Processing transaction', { transactionMeta, signedTx });
+    log('Processing transaction', {
+      nonce: nonceDecimal,
+      signedTx,
+      transactionMeta,
+    });
 
     const publishPromise = createDeferredPromise<PublishHookResult>();
 
-    this.#publishPromises.push(publishPromise);
+    this.#results.push({
+      nonce: nonceDecimal,
+      promise: publishPromise,
+      signedTransaction: signedTx as Hex,
+    });
+
+    this.#results = sortBy(this.#results, (r) => r.nonce);
 
-    if (this.#signedTransactions.length === this.#transactionCount) {
+    if (this.#results.length === this.#transactionCount) {
       log('All transactions signed');
 
+      const signedTransactions = this.#results.map(
+        (result) => result.signedTransaction,
+      );
+
       this.#readyPromise.resolve({
-        signedTransactions: this.#signedTransactions,
+        signedTransactions,
       });
     }