Make addUnapprovedTransaction action idempotent (#15667)

2 years ago · 3fb7de5768
parent e817ff8a77
commit 3fb7de5768
6 changed files with 159 additions and 32 deletions
--- a/app/scripts/controllers/transactions/index.js
+++ b/app/scripts/controllers/transactions/index.js
@ -52,6 +52,7 @@ import {
 } from '../../../../shared/constants/network';
 import {
  determineTransactionAssetType,
+  determineTransactionContractCode,
  determineTransactionType,
  isEIP1559Transaction,
 } from '../../../../shared/modules/transaction.utils';
@ -696,16 +697,54 @@ export default class TransactionController extends EventEmitter {
    return this._getTransaction(txId);
  }

+  async addTransactionGasDefaults(txMeta) {
+    const contractCode = await determineTransactionContractCode(
+      txMeta.txParams,
+      this.query,
+    );
+
+    let updateTxMeta = txMeta;
+    try {
+      updateTxMeta = await this.addTxGasDefaults(txMeta, contractCode);
+    } catch (error) {
+      log.warn(error);
+      updateTxMeta = this.txStateManager.getTransaction(txMeta.id);
+      updateTxMeta.loadingDefaults = false;
+      this.txStateManager.updateTransaction(
+        txMeta,
+        'Failed to calculate gas defaults.',
+      );
+      throw error;
+    }
+
+    updateTxMeta.loadingDefaults = false;
+
+    // The history note used here 'Added new unapproved transaction.' is confusing update call only updated the gas defaults.
+    // We need to improve `this.addTransaction` to accept history note and change note here.
+    this.txStateManager.updateTransaction(
+      updateTxMeta,
+      'Added new unapproved transaction.',
+    );
+
+    return updateTxMeta;
+  }
+
  // ====================================================================================================================================================

  /**
   * Validates and generates a txMeta with defaults and puts it in txStateManager
   * store.
   *
+   * actionId is used to uniquely identify a request to create a transaction.
+   * Only 1 transaction will be created for multiple requests with same actionId.
+   * actionId is fix used for making this action idempotent to deal with scenario when
+   * action is invoked multiple times with same parameters in MV3 due to service worker re-activation.
+   *
   * @param txParams
   * @param origin
   * @param transactionType
   * @param sendFlowHistory
+   * @param actionId
   * @returns {txMeta}
   */
  async addUnapprovedTransaction(
@ -713,6 +752,7 @@ export default class TransactionController extends EventEmitter {
    origin,
    transactionType,
    sendFlowHistory = [],
+    actionId,
  ) {
    if (
      transactionType !== undefined &&
@ -723,6 +763,17 @@ export default class TransactionController extends EventEmitter {
      );
    }

+    // In transaction is found for same action id, do not create a new transaction.
+    if (actionId) {
+      let existingTxMeta =
+        this.txStateManager.getTransactionWithActionId(actionId);
+      if (existingTxMeta) {
+        this.emit('newUnapprovedTx', existingTxMeta);
+        existingTxMeta = await this.addTransactionGasDefaults(existingTxMeta);
+        return existingTxMeta;
+      }
+    }
+
    // validate
    const normalizedTxParams = txUtils.normalizeTxParams(txParams);
    const eip1559Compatibility = await this.getEIP1559Compatibility();
@ -741,6 +792,12 @@ export default class TransactionController extends EventEmitter {
      sendFlowHistory,
    });

+    // Add actionId to txMeta to check if same actionId is seen again
+    // IF request to create transaction with same actionId is submitted again, new transaction will not be added for it.
+    if (actionId) {
+      txMeta.actionId = actionId;
+    }
+
    if (origin === ORIGIN_METAMASK) {
      // Assert the from address is the selected address
      if (normalizedTxParams.from !== this.getSelectedAddress()) {
@ -762,10 +819,7 @@ export default class TransactionController extends EventEmitter {
      }
    }

-    const { type, getCodeResponse } = await determineTransactionType(
-      txParams,
-      this.query,
-    );
+    const { type } = await determineTransactionType(txParams, this.query);
    txMeta.type = transactionType || type;

    // ensure value
@ -776,25 +830,7 @@ export default class TransactionController extends EventEmitter {
    this.addTransaction(txMeta);
    this.emit('newUnapprovedTx', txMeta);

-    try {
-      txMeta = await this.addTxGasDefaults(txMeta, getCodeResponse);
-    } catch (error) {
-      log.warn(error);
-      txMeta = this.txStateManager.getTransaction(txMeta.id);
-      txMeta.loadingDefaults = false;
-      this.txStateManager.updateTransaction(
-        txMeta,
-        'Failed to calculate gas defaults.',
-      );
-      throw error;
-    }
-
-    txMeta.loadingDefaults = false;
-    // save txMeta
-    this.txStateManager.updateTransaction(
-      txMeta,
-      'Added new unapproved transaction.',
-    );
+    txMeta = await this.addTransactionGasDefaults(txMeta);

    return txMeta;
  }
--- a/app/scripts/controllers/transactions/index.test.js
+++ b/app/scripts/controllers/transactions/index.test.js
@ -334,11 +334,14 @@ describe('Transaction Controller', function () {
    const selectedAddress = '0x1678a085c290ebd122dc42cba69373b5953b831d';
    const recipientAddress = '0xc42edfcc21ed14dda456aa0756c153f7985d8813';

-    let getSelectedAddress, getPermittedAccounts;
+    let getSelectedAddress, getPermittedAccounts, getDefaultGasFees;
    beforeEach(function () {
      getSelectedAddress = sinon
        .stub(txController, 'getSelectedAddress')
        .returns(selectedAddress);
+      getDefaultGasFees = sinon
+        .stub(txController, '_getDefaultGasFees')
+        .returns({});
      getPermittedAccounts = sinon
        .stub(txController, 'getPermittedAccounts')
        .returns([selectedAddress]);
@ -347,6 +350,7 @@ describe('Transaction Controller', function () {
    afterEach(function () {
      getSelectedAddress.restore();
      getPermittedAccounts.restore();
+      getDefaultGasFees.restore();
    });

    it('should add an unapproved transaction and return a valid txMeta', async function () {
@ -372,6 +376,62 @@ describe('Transaction Controller', function () {
      assert.deepEqual(txMeta, memTxMeta);
    });

+    it('should add only 1 unapproved transaction when called twice with same actionId', async function () {
+      await txController.addUnapprovedTransaction(
+        {
+          from: selectedAddress,
+          to: recipientAddress,
+        },
+        undefined,
+        undefined,
+        undefined,
+        '12345',
+      );
+      const transactionCount1 =
+        txController.txStateManager.getTransactions().length;
+      await txController.addUnapprovedTransaction(
+        {
+          from: selectedAddress,
+          to: recipientAddress,
+        },
+        undefined,
+        undefined,
+        undefined,
+        '12345',
+      );
+      const transactionCount2 =
+        txController.txStateManager.getTransactions().length;
+      assert.equal(transactionCount1, transactionCount2);
+    });
+
+    it('should add multiple transactions when called with different actionId', async function () {
+      await txController.addUnapprovedTransaction(
+        {
+          from: selectedAddress,
+          to: recipientAddress,
+        },
+        undefined,
+        undefined,
+        undefined,
+        '12345',
+      );
+      const transactionCount1 =
+        txController.txStateManager.getTransactions().length;
+      await txController.addUnapprovedTransaction(
+        {
+          from: selectedAddress,
+          to: recipientAddress,
+        },
+        undefined,
+        undefined,
+        undefined,
+        '00000',
+      );
+      const transactionCount2 =
+        txController.txStateManager.getTransactions().length;
+      assert.equal(transactionCount1 + 1, transactionCount2);
+    });
+
    it('should emit newUnapprovedTx event and pass txMeta as the first argument', function (done) {
      providerResultStub.eth_gasPrice = '4a817c800';
      txController.once('newUnapprovedTx', (txMetaFromEmit) => {
--- a/app/scripts/controllers/transactions/tx-state-manager.js
+++ b/app/scripts/controllers/transactions/tx-state-manager.js
@ -1,7 +1,7 @@
 import EventEmitter from 'safe-event-emitter';
 import { ObservableStore } from '@metamask/obs-store';
 import log from 'loglevel';
-import { keyBy, mapValues, omitBy, pickBy, sortBy } from 'lodash';
+import { values, keyBy, mapValues, omitBy, pickBy, sortBy } from 'lodash';
 import createId from '../../../../shared/modules/random-id';
 import { TRANSACTION_STATUSES } from '../../../../shared/constants/transaction';
 import { METAMASK_CONTROLLER_EVENTS } from '../../metamask-controller';
@ -201,6 +201,21 @@ export default class TransactionStateManager extends EventEmitter {
    return this.getTransactions({ searchCriteria });
  }

+  /**
+   * Get transaction with provided.
+   *
+   * @param {string} [actionId]
+   * @returns {TransactionMeta} the filtered transaction
+   */
+  getTransactionWithActionId(actionId) {
+    return values(
+      pickBy(
+        this.store.getState().transactions,
+        (transaction) => transaction.actionId === actionId,
+      ),
+    )[0];
+  }
+
  /**
   * Adds the txMeta to the list of transactions in the store.
   * if the list is over txHistoryLimit it will remove a transaction that
--- a/shared/modules/transaction.utils.js
+++ b/shared/modules/transaction.utils.js
@ -131,6 +131,19 @@ export function parseStandardTokenTransactionData(data) {
  return undefined;
 }

+/**
+ * Determines the contractCode of the transaction by analyzing the txParams.
+ *
+ * @param {object} txParams - Parameters for the transaction
+ * @param {EthQuery} query - EthQuery instance
+ * @returns {InferTransactionTypeResult}
+ */
+export async function determineTransactionContractCode(txParams, query) {
+  const { to } = txParams;
+  const { contractCode } = await readAddressAsContract(query, to);
+  return contractCode;
+}
+
 /**
 * Determines the type of the transaction by analyzing the txParams.
 * This method will return one of the types defined in shared/constants/transactions
--- a/ui/ducks/send/send.test.js
+++ b/ui/ducks/send/send.test.js
@ -94,7 +94,7 @@ jest.mock('lodash', () => ({

 setBackgroundConnection({
  addPollingTokenToAppState: jest.fn(),
-  addUnapprovedTransaction: jest.fn((_w, _x, _y, _z, cb) => {
+  addUnapprovedTransaction: jest.fn((_v, _w, _x, _y, _z, cb) => {
    cb(null);
  }),
  updateTransactionSendFlowHistory: jest.fn((_x, _y, cb) => cb(null)),
--- a/ui/store/actions.js
+++ b/ui/store/actions.js
@ -889,11 +889,13 @@ export function addUnapprovedTransactionAndRouteToConfirmationPage(
  sendFlowHistory,
 ) {
  return async (dispatch) => {
+    const actionId = Date.now() + Math.random();
    try {
      log.debug('background.addUnapprovedTransaction');
      const txMeta = await submitRequestToBackground(
        'addUnapprovedTransaction',
-        [txParams, ORIGIN_METAMASK, type, sendFlowHistory],
+        [txParams, ORIGIN_METAMASK, type, sendFlowHistory, actionId],
+        actionId,
      );
      dispatch(showConfTxPage());
      return txMeta;
@ -920,11 +922,12 @@ export function addUnapprovedTransactionAndRouteToConfirmationPage(
 */
 export async function addUnapprovedTransaction(txParams, type) {
  log.debug('background.addUnapprovedTransaction');
-  const txMeta = await submitRequestToBackground('addUnapprovedTransaction', [
-    txParams,
-    ORIGIN_METAMASK,
-    type,
-  ]);
+  const actionId = Date.now() + Math.random();
+  const txMeta = await submitRequestToBackground(
+    'addUnapprovedTransaction',
+    [txParams, ORIGIN_METAMASK, type, undefined, actionId],
+    actionId,
+  );
  return txMeta;
 }