Make cancel transaction idempotent (#15675)

app/scripts/controllers/transactions/index.js

@@ -1161,13 +1161,23 @@ export default class TransactionController extends EventEmitter {
    *  params instead of allowing this method to generate them
    * @param options
    * @param options.estimatedBaseFee
+   * @param options.actionId
    * @returns {txMeta}
    */
   async createCancelTransaction(
     originalTxId,
     customGasSettings,
-    { estimatedBaseFee } = {},
+    { estimatedBaseFee, actionId } = {},
   ) {
+    // If transaction is found for same action id, do not create a new cancel transaction.
+    if (actionId) {
+      const existingTxMeta =
+        this.txStateManager.getTransactionWithActionId(actionId);
+      if (existingTxMeta) {
+        return existingTxMeta;
+      }
+    }
+
     const originalTxMeta = this.txStateManager.getTransaction(originalTxId);
     const { txParams } = originalTxMeta;
     const { from, nonce } = txParams;
@@ -1195,6 +1205,7 @@ export default class TransactionController extends EventEmitter {
       loadingDefaults: false,
       status: TRANSACTION_STATUSES.APPROVED,
       type: TRANSACTION_TYPES.CANCEL,
+      actionId,
     });
 
     if (estimatedBaseFee) {
@@ -1293,6 +1304,7 @@ export default class TransactionController extends EventEmitter {
     // we need to keep track of what is currently being signed,
     // So that we do not increment nonce + resubmit something
     // that is already being incremented & signed.
+    const txMeta = this.txStateManager.getTransaction(txId);
     if (this.inProcessOfSigning.has(txId)) {
       return;
     }
@@ -1302,8 +1314,6 @@ export default class TransactionController extends EventEmitter {
       // approve
       this.txStateManager.setTxStatusApproved(txId);
       // get next nonce
-      const txMeta = this.txStateManager.getTransaction(txId);
-
       const fromAddress = txMeta.txParams.from;
       // wait for a nonce
       let { customNonceValue } = txMeta;
@@ -1793,10 +1803,9 @@ export default class TransactionController extends EventEmitter {
         },
       })
       .forEach((txMeta) => {
-        const txSignError = new Error(
-          'Transaction found as "approved" during boot - possibly stuck during signing',
-        );
-        this._failTransaction(txMeta.id, txSignError);
+        // Line below will try to publish transaction which is in
+        // APPROVED state at the time of controller bootup
+        this.approveTransaction(txMeta);
       });
   }
 

app/scripts/controllers/transactions/index.test.js

@@ -467,6 +467,106 @@ describe('Transaction Controller', function () {
     });
   });
 
+  describe('#createCancelTransaction', function () {
+    const selectedAddress = '0x1678a085c290ebd122dc42cba69373b5953b831d';
+    const recipientAddress = '0xc42edfcc21ed14dda456aa0756c153f7985d8813';
+
+    let getSelectedAddress,
+      getPermittedAccounts,
+      getDefaultGasFees,
+      getDefaultGasLimit;
+    beforeEach(function () {
+      const hash =
+        '0x2a5523c6fa98b47b7d9b6c8320179785150b42a16bcff36b398c5062b65657e8';
+      providerResultStub.eth_sendRawTransaction = hash;
+
+      getSelectedAddress = sinon
+        .stub(txController, 'getSelectedAddress')
+        .returns(selectedAddress);
+      getDefaultGasFees = sinon
+        .stub(txController, '_getDefaultGasFees')
+        .returns({});
+      getDefaultGasLimit = sinon
+        .stub(txController, '_getDefaultGasLimit')
+        .returns({});
+      getPermittedAccounts = sinon
+        .stub(txController, 'getPermittedAccounts')
+        .returns([selectedAddress]);
+    });
+
+    afterEach(function () {
+      getSelectedAddress.restore();
+      getPermittedAccounts.restore();
+      getDefaultGasFees.restore();
+      getDefaultGasLimit.restore();
+    });
+
+    it('should add an cancel transaction and return a valid txMeta', async function () {
+      const txMeta = await txController.addUnapprovedTransaction({
+        from: selectedAddress,
+        to: recipientAddress,
+      });
+      await txController.approveTransaction(txMeta.id);
+      const cancelTxMeta = await txController.createCancelTransaction(
+        txMeta.id,
+        {},
+        { actionId: 12345 },
+      );
+      assert.equal(cancelTxMeta.type, TRANSACTION_TYPES.CANCEL);
+      const memTxMeta = txController.txStateManager.getTransaction(
+        cancelTxMeta.id,
+      );
+      assert.deepEqual(cancelTxMeta, memTxMeta);
+    });
+
+    it('should add only 1 cancel transaction when called twice with same actionId', async function () {
+      const txMeta = await txController.addUnapprovedTransaction({
+        from: selectedAddress,
+        to: recipientAddress,
+      });
+      await txController.approveTransaction(txMeta.id);
+      await txController.createCancelTransaction(
+        txMeta.id,
+        {},
+        { actionId: 12345 },
+      );
+      const transactionCount1 =
+        txController.txStateManager.getTransactions().length;
+      await txController.createCancelTransaction(
+        txMeta.id,
+        {},
+        { actionId: 12345 },
+      );
+      const transactionCount2 =
+        txController.txStateManager.getTransactions().length;
+      assert.equal(transactionCount1, transactionCount2);
+    });
+
+    it('should add multiple transactions when called with different actionId', async function () {
+      const txMeta = await txController.addUnapprovedTransaction({
+        from: selectedAddress,
+        to: recipientAddress,
+      });
+      await txController.approveTransaction(txMeta.id);
+      await txController.createCancelTransaction(
+        txMeta.id,
+        {},
+        undefined,
+        12345,
+      );
+      const transactionCount1 =
+        txController.txStateManager.getTransactions().length;
+      await txController.createCancelTransaction(
+        txMeta.id,
+        {},
+        { actionId: 11111 },
+      );
+      const transactionCount2 =
+        txController.txStateManager.getTransactions().length;
+      assert.equal(transactionCount1 + 1, transactionCount2);
+    });
+  });
+
   describe('#addTxGasDefaults', function () {
     it('should add the tx defaults if their are none', async function () {
       txController.txStateManager._addTransactionsToState([

app/scripts/metamask-controller.js

@@ -3278,18 +3278,14 @@ export default class MetamaskController extends EventEmitter {
    *  './controllers/transactions'
    * ).CustomGasSettings} [customGasSettings] - overrides to use for gas params
    *  instead of allowing this method to generate them
-   * @param newTxMetaProps
+   * @param options
    * @returns {object} MetaMask state
    */
-  async createCancelTransaction(
-    originalTxId,
-    customGasSettings,
-    newTxMetaProps,
-  ) {
+  async createCancelTransaction(originalTxId, customGasSettings, options) {
     await this.txController.createCancelTransaction(
       originalTxId,
       customGasSettings,
-      newTxMetaProps,
+      options,
     );
     const state = await this.getState();
     return state;

ui/store/actions.js

@@ -1974,19 +1974,16 @@ export function clearPendingTokens() {
   };
 }
 
-export function createCancelTransaction(
-  txId,
-  customGasSettings,
-  newTxMetaProps,
-) {
+export function createCancelTransaction(txId, customGasSettings, options) {
   log.debug('background.cancelTransaction');
   let newTxId;
 
   return (dispatch) => {
+    const actionId = Date.now() + Math.random();
     return new Promise((resolve, reject) => {
       callBackgroundMethod(
         'createCancelTransaction',
-        [txId, customGasSettings, newTxMetaProps],
+        [txId, customGasSettings, { ...options, actionId }],
         (err, newState) => {
           if (err) {
             dispatch(displayWarning(err.message));
@@ -1999,6 +1996,7 @@ export function createCancelTransaction(
           newTxId = id;
           resolve(newState);
         },
+        actionId,
       );
     })
       .then((newState) => dispatch(updateMetamaskState(newState)))