Check pending txs before submitting cancel (#3800)

## Explanation This PR updates `speedUp` and `cancel` transactions to force checking pending transaction statuses before creating each transaction. ## References * Fixes MetaMask/metamask-extension#22314 ## Changelog ### `@metamask/transaction-controller` - **Added**: Added a call of `PendingTransactionTracker.checkTransactions` before creating speed-up and cancel transactions ## 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 - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Matthew Walsh <[email protected]>
MetaMask · Feb 13, 2024 · 81ac379 · 81ac379
1 parent 58d2bd7
commit 81ac379
Show file tree

Hide file tree

Showing 4 changed files with 244 additions and 2 deletions.
diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts
@@ -625,6 +625,7 @@ describe('TransactionController', () => {
       hub: {
         on: jest.fn(),
       },
+      forceCheckTransaction: jest.fn(),
     } as unknown as jest.Mocked<PendingTransactionTracker>;
 
     incomingTransactionHelperClassMock.mockReturnValue(
@@ -1878,6 +1879,73 @@ describe('TransactionController', () => {
       expect(controller.state.transactions).toHaveLength(1);
     });
 
+    it('should throw error if transaction already confirmed', async () => {
+      const controller = newController();
+
+      controller.state.transactions.push({
+        id: '2',
+        chainId: toHex(5),
+        status: TransactionStatus.submitted,
+        type: TransactionType.cancel,
+        time: 123456789,
+        txParams: {
+          from: ACCOUNT_MOCK,
+        },
+      });
+
+      mockSendRawTransaction.mockImplementationOnce(
+        (_transaction, callback) => {
+          callback(
+            undefined,
+            // eslint-disable-next-line prefer-promise-reject-errors
+            Promise.reject({
+              message: 'nonce too low',
+            }),
+          );
+        },
+      );
+
+      await expect(controller.stopTransaction('2')).rejects.toThrow(
+        'Previous transaction is already confirmed',
+      );
+
+      // Expect cancel transaction to be submitted - it will fail
+      expect(mockSendRawTransaction).toHaveBeenCalledTimes(1);
+      expect(controller.state.transactions).toHaveLength(1);
+    });
+
+    it('should throw error if publish transaction fails', async () => {
+      const errorMock = new Error('Another reason');
+      const controller = newController();
+
+      controller.state.transactions.push({
+        id: '2',
+        chainId: toHex(5),
+        status: TransactionStatus.submitted,
+        type: TransactionType.cancel,
+        time: 123456789,
+        txParams: {
+          from: ACCOUNT_MOCK,
+        },
+      });
+
+      mockSendRawTransaction.mockImplementationOnce(
+        (_transaction, callback) => {
+          callback(
+            undefined,
+            // eslint-disable-next-line prefer-promise-reject-errors
+            Promise.reject(errorMock),
+          );
+        },
+      );
+
+      await expect(controller.stopTransaction('2')).rejects.toThrow(errorMock);
+
+      // Expect cancel transaction to be submitted - it will fail
+      expect(mockSendRawTransaction).toHaveBeenCalledTimes(1);
+      expect(controller.state.transactions).toHaveLength(1);
+    });
+
     it('submits a cancel transaction', async () => {
       const simpleSendTransactionId =
         'simpleeb1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d';
@@ -2098,6 +2166,75 @@ describe('TransactionController', () => {
       expect(controller.state.transactions).toHaveLength(1);
     });
 
+    it('should throw error if transaction already confirmed', async () => {
+      const controller = newController();
+
+      controller.state.transactions.push({
+        id: '2',
+        chainId: toHex(5),
+        status: TransactionStatus.submitted,
+        type: TransactionType.retry,
+        time: 123456789,
+        txParams: {
+          from: ACCOUNT_MOCK,
+        },
+      });
+
+      mockSendRawTransaction.mockImplementationOnce(
+        (_transaction, callback) => {
+          callback(
+            undefined,
+            // eslint-disable-next-line prefer-promise-reject-errors
+            Promise.reject({
+              message: 'nonce too low',
+            }),
+          );
+        },
+      );
+
+      await expect(controller.speedUpTransaction('2')).rejects.toThrow(
+        'Previous transaction is already confirmed',
+      );
+
+      // Expect speedup transaction to be submitted - it will fail
+      expect(mockSendRawTransaction).toHaveBeenCalledTimes(1);
+      expect(controller.state.transactions).toHaveLength(1);
+    });
+
+    it('should throw error if publish transaction fails', async () => {
+      const controller = newController();
+      const errorMock = new Error('Another reason');
+
+      controller.state.transactions.push({
+        id: '2',
+        chainId: toHex(5),
+        status: TransactionStatus.submitted,
+        type: TransactionType.retry,
+        time: 123456789,
+        txParams: {
+          from: ACCOUNT_MOCK,
+        },
+      });
+
+      mockSendRawTransaction.mockImplementationOnce(
+        (_transaction, callback) => {
+          callback(
+            undefined,
+            // eslint-disable-next-line prefer-promise-reject-errors
+            Promise.reject(errorMock),
+          );
+        },
+      );
+
+      await expect(controller.speedUpTransaction('2')).rejects.toThrow(
+        errorMock,
+      );
+
+      // Expect speedup transaction to be submitted - it will fail
+      expect(mockSendRawTransaction).toHaveBeenCalledTimes(1);
+      expect(controller.state.transactions).toHaveLength(1);
+    });
+
     it('creates additional transaction with increased gas', async () => {
       const controller = newController({
         network: MOCK_LINEA_MAINNET_NETWORK,

diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts
@@ -876,7 +876,7 @@ export class TransactionController extends BaseControllerV1<
       txParams: newTxParams,
     });
 
-    const hash = await this.publishTransaction(rawTx);
+    const hash = await this.publishTransactionForRetry(rawTx, transactionMeta);
 
     const cancelTransactionMeta: TransactionMeta = {
       actionId,
@@ -1028,7 +1028,7 @@ export class TransactionController extends BaseControllerV1<
 
     log('Submitting speed up transaction', { oldFee, newFee, txParams });
 
-    const hash = await query(this.ethQuery, 'sendRawTransaction', [rawTx]);
+    const hash = await this.publishTransactionForRetry(rawTx, transactionMeta);
 
     const baseTransactionMeta: TransactionMeta = {
       ...transactionMeta,
@@ -2747,4 +2747,38 @@ export class TransactionController extends BaseControllerV1<
       log('Error while updating post transaction balance', error);
     }
   }
+
+  private async publishTransactionForRetry(
+    rawTx: string,
+    transactionMeta: TransactionMeta,
+  ): Promise<string> {
+    try {
+      const hash = await this.publishTransaction(rawTx);
+      return hash;
+    } catch (error: unknown) {
+      if (this.isTransactionAlreadyConfirmedError(error as Error)) {
+        await this.pendingTransactionTracker.forceCheckTransaction(
+          transactionMeta,
+        );
+        throw new Error('Previous transaction is already confirmed');
+      }
+      throw error;
+    }
+  }
+
+  /**
+   * Ensures that error is a nonce issue
+   *
+   * @param error - The error to check
+   * @returns Whether or not the error is a nonce issue
+   */
+  // TODO: Replace `any` with type
+  // Some networks are returning original error in the data field
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  private isTransactionAlreadyConfirmedError(error: any): boolean {
+    return (
+      error?.message?.includes('nonce too low') ||
+      error?.data?.message?.includes('nonce too low')
+    );
+  }
 }
diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts
@@ -822,4 +822,57 @@ describe('PendingTransactionTracker', () => {
       });
     });
   });
+
+  describe('forceCheckTransaction', () => {
+    let tracker: PendingTransactionTracker;
+    let transactionMeta: TransactionMeta;
+
+    beforeEach(() => {
+      tracker = new PendingTransactionTracker(options);
+      transactionMeta = {
+        ...TRANSACTION_SUBMITTED_MOCK,
+        hash: '0x123',
+      } as TransactionMeta;
+    });
+
+    it('should update transaction status to confirmed if receipt status is success', async () => {
+      queryMock.mockResolvedValueOnce(RECEIPT_MOCK);
+      queryMock.mockResolvedValueOnce(BLOCK_MOCK);
+      options.getTransactions.mockReturnValue([]);
+
+      await tracker.forceCheckTransaction(transactionMeta);
+
+      expect(transactionMeta.status).toStrictEqual(TransactionStatus.confirmed);
+      expect(transactionMeta.txReceipt).toStrictEqual(RECEIPT_MOCK);
+      expect(transactionMeta.verifiedOnBlockchain).toBe(true);
+    });
+
+    it('should fail transaction if receipt status is failure', async () => {
+      const receiptMock = { ...RECEIPT_MOCK, status: '0x0' };
+      queryMock.mockResolvedValueOnce(receiptMock);
+      options.getTransactions.mockReturnValue([]);
+
+      const listener = jest.fn();
+      tracker.hub.addListener('transaction-failed', listener);
+
+      await tracker.forceCheckTransaction(transactionMeta);
+
+      expect(listener).toHaveBeenCalledTimes(1);
+      expect(listener).toHaveBeenCalledWith(
+        transactionMeta,
+        new Error('Transaction dropped or replaced'),
+      );
+    });
+
+    it('should not change transaction status if receipt status is neither success nor failure', async () => {
+      const receiptMock = { ...RECEIPT_MOCK, status: '0x2' };
+      queryMock.mockResolvedValueOnce(receiptMock);
+      options.getTransactions.mockReturnValue([]);
+
+      await tracker.forceCheckTransaction(transactionMeta);
+
+      expect(transactionMeta.status).toStrictEqual(TransactionStatus.submitted);
+      expect(transactionMeta.txReceipt).toBeUndefined();
+    });
+  });
 });
diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts
@@ -144,6 +144,24 @@ export class PendingTransactionTracker {
     });
   }
 
+  /**
+   * Force checks the network if the given transaction is confirmed and updates it's status.
+   *
+   * @param txMeta - The transaction to check
+   */
+  async forceCheckTransaction(txMeta: TransactionMeta) {
+    const nonceGlobalLock = await this.#nonceTracker.getGlobalLock();
+
+    try {
+      await this.#checkTransaction(txMeta);
+    } catch (error) {
+      /* istanbul ignore next */
+      log('Failed to check transaction', error);
+    } finally {
+      nonceGlobalLock.releaseLock();
+    }
+  }
+
   #start() {
     if (this.#running) {
       return;