Use single controller method to add transactions (#20007)

app/scripts/controllers/transactions/index.js

@@ -315,22 +315,6 @@ export default class TransactionController extends EventEmitter {
     });
   }
 
-  /**
-   * Adds a tx to the txlist
-   *
-   * @param txMeta
-   * @fires ${txMeta.id}:unapproved
-   */
-  addTransaction(txMeta) {
-    this.txStateManager.addTransaction(txMeta);
-    this.emit(`${txMeta.id}:unapproved`, txMeta);
-    this._trackTransactionMetricsEvent(
-      txMeta,
-      TransactionMetaMetricsEvent.added,
-      txMeta.actionId,
-    );
-  }
-
   /**
    * Wipes the transactions for a given account
    *
@@ -341,64 +325,52 @@ export default class TransactionController extends EventEmitter {
   }
 
   /**
-   * Add a new unapproved transaction to the pipeline
+   * Add a new unapproved transaction
    *
-   * @returns {Promise<string>} the hash of the transaction after being submitted to the network
+   * @param {object} txParams - Standard parameters for an Ethereum transaction
-   * @param {object} txParams - txParams for the transaction
+   * @param {object} opts - Options
-   * @param {object} opts - with the key origin to put the origin on the txMeta
+   * @param {string} opts.actionId - Unique ID to prevent duplicate requests
+   * @param {string} opts.method - RPC method that requested the transaction
+   * @param {string} opts.origin - Origin of the transaction request, such as the hostname of a dApp
+   * @param {boolean} opts.requireApproval - Whether the transaction requires approval by the user
+   * @param {object[]} opts.sendFlowHistory - Associated history to store with the transaction
+   * @param {object} opts.swaps - Options specific to swap transactions
+   * @param {boolean} opts.swaps.hasApproveTx - Whether this transaction required an approval transaction
+   * @param {boolean} opts.swaps.meta - Additional metadata to store for the transaction
+   * @param {TransactionType} opts.type - Type of transaction to add, such as 'cancel' or 'swap'
+   * @returns {Promise<{transactionMeta: TransactionMeta, result: Promise<string>}>} An object containing the transaction metadata, and a promise that resolves to the transaction hash after being submitted to the network
    */
-  async newUnapprovedTransaction(txParams, opts = {}) {
+  async addTransaction(
-    log.debug(
-      `MetaMaskController newUnapprovedTransaction ${JSON.stringify(txParams)}`,
-    );
-
-    const { txMeta: initialTxMeta, isExisting } = await this._createTransaction(
-      opts.method,
     txParams,
-      opts.origin,
+    {
-      undefined,
+      actionId,
-      undefined,
+      method,
-      opts.id,
+      origin,
-    );
+      requireApproval,
+      sendFlowHistory,
+      swaps: { hasApproveTx, meta } = {},
+      type,
+    } = {},
+  ) {
+    log.debug(`MetaMaskController addTransaction ${JSON.stringify(txParams)}`);
 
-    const txId = initialTxMeta.id;
+    const { txMeta, isExisting } = await this._createTransaction(txParams, {
-    const isCompleted = this._isTransactionCompleted(initialTxMeta);
+      actionId,
+      method,
+      origin,
+      sendFlowHistory,
+      swaps: { hasApproveTx, meta },
+      type,
+    });
 
-    const finishedPromise = isCompleted
+    return {
-      ? Promise.resolve(initialTxMeta)
+      transactionMeta: txMeta,
-      : this._waitForTransactionFinished(txId);
+      result: this._processApproval(txMeta, {
-
+        isExisting,
-    if (!isExisting && !isCompleted) {
+        requireApproval,
-      try {
+        actionId,
-        await this._requestTransactionApproval(initialTxMeta);
+      }),
-      } catch (error) {
+    };
-        // Errors generated from final status using finished event
-      }
-    }
-
-    const finalTxMeta = await finishedPromise;
-    const finalStatus = finalTxMeta?.status;
-
-    switch (finalStatus) {
-      case TransactionStatus.submitted:
-        return finalTxMeta.hash;
-      case TransactionStatus.rejected:
-        throw cleanErrorStack(
-          ethErrors.provider.userRejectedRequest(
-            'MetaMask Tx Signature: User denied transaction signature.',
-          ),
-        );
-      case TransactionStatus.failed:
-        throw cleanErrorStack(ethErrors.rpc.internal(finalTxMeta.err.message));
-      default:
-        throw cleanErrorStack(
-          ethErrors.rpc.internal(
-            `MetaMask Tx Signature: Unknown problem: ${JSON.stringify(
-              finalTxMeta?.txParams,
-            )}`,
-          ),
-        );
-    }
   }
 
   /**
@@ -694,7 +666,7 @@ export default class TransactionController extends EventEmitter {
     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.
+    // We need to improve `this._addTransaction` to accept history note and change note here.
     this.txStateManager.updateTransaction(
       updateTxMeta,
       'Added new unapproved transaction.',
@@ -705,58 +677,6 @@ export default class TransactionController extends EventEmitter {
 
   // ====================================================================================================================================================
 
-  /**
-   * 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 txMethodType
-   * @param txParams
-   * @param origin
-   * @param transactionType
-   * @param sendFlowHistory
-   * @param actionId
-   * @param options
-   */
-  async addUnapprovedTransaction(
-    txMethodType,
-    txParams,
-    origin,
-    transactionType,
-    sendFlowHistory = [],
-    actionId,
-    options,
-  ) {
-    const { txMeta, isExisting } = await this._createTransaction(
-      txMethodType,
-      txParams,
-      origin,
-      transactionType,
-      sendFlowHistory,
-      actionId,
-      options,
-    );
-    if (isExisting) {
-      const isCompleted = this._isTransactionCompleted(txMeta);
-
-      return isCompleted
-        ? txMeta
-        : await this._waitForTransactionFinished(txMeta.id);
-    }
-
-    if (options?.requireApproval === false) {
-      await this._updateAndApproveTransaction(txMeta, actionId);
-    } else {
-      await this._requestTransactionApproval(txMeta, { actionId });
-    }
-
-    return txMeta;
-  }
-
   /**
    * Adds the tx gas defaults: gas && gasPrice
    *
@@ -1122,7 +1042,7 @@ export default class TransactionController extends EventEmitter {
       newTxMeta.estimatedBaseFee = estimatedBaseFee;
     }
 
-    this.addTransaction(newTxMeta);
+    this._addTransaction(newTxMeta);
     await this._approveTransaction(newTxMeta.id, actionId, {
       hasApprovalRequest: false,
     });
@@ -1182,7 +1102,7 @@ export default class TransactionController extends EventEmitter {
       newTxMeta.estimatedBaseFee = estimatedBaseFee;
     }
 
-    this.addTransaction(newTxMeta);
+    this._addTransaction(newTxMeta);
     await this._approveTransaction(newTxMeta.id, actionId);
     return newTxMeta;
   }
@@ -1593,21 +1513,14 @@ export default class TransactionController extends EventEmitter {
   }
 
   async _createTransaction(
-    txMethodType,
     txParams,
-    origin,
+    { actionId, method, origin, sendFlowHistory = [], swaps, type },
-    transactionType,
-    sendFlowHistory = [],
-    actionId,
-    options,
   ) {
     if (
-      transactionType !== undefined &&
+      type !== undefined &&
-      !VALID_UNAPPROVED_TRANSACTION_TYPES.includes(transactionType)
+      !VALID_UNAPPROVED_TRANSACTION_TYPES.includes(type)
     ) {
-      throw new Error(
+      throw new Error(`TransactionController - invalid type value: ${type}`);
-        `TransactionController - invalid transactionType value: ${transactionType}`,
-      );
     }
 
     // If a transaction is found with the same actionId, do not create a new speed-up transaction.
@@ -1665,40 +1578,32 @@ export default class TransactionController extends EventEmitter {
       }
     }
 
-    const { type } = await determineTransactionType(
+    const { type: determinedType } = await determineTransactionType(
       normalizedTxParams,
       this.query,
     );
-    txMeta.type = transactionType || type;
+    txMeta.type = type || determinedType;
 
     // ensure value
     txMeta.txParams.value = txMeta.txParams.value
       ? addHexPrefix(txMeta.txParams.value)
       : '0x0';
 
-    if (txMethodType && this.securityProviderRequest) {
+    if (method && this.securityProviderRequest) {
       const securityProviderResponse = await this.securityProviderRequest(
         txMeta,
-        txMethodType,
+        method,
       );
 
       txMeta.securityProviderResponse = securityProviderResponse;
     }
 
-    this.addTransaction(txMeta);
+    this._addTransaction(txMeta);
 
     txMeta = await this.addTransactionGasDefaults(txMeta);
 
-    if (
+    if ([TransactionType.swap, TransactionType.swapApproval].includes(type)) {
-      [TransactionType.swap, TransactionType.swapApproval].includes(
+      txMeta = await this._createSwapsTransaction(swaps, type, txMeta);
-        transactionType,
-      )
-    ) {
-      txMeta = await this._createSwapsTransaction(
-        options?.swaps,
-        transactionType,
-        txMeta,
-      );
     }
 
     return { txMeta, isExisting: false };
@@ -1830,6 +1735,51 @@ export default class TransactionController extends EventEmitter {
     await this._approveTransaction(txMeta.id, actionId);
   }
 
+  async _processApproval(txMeta, { actionId, isExisting, requireApproval }) {
+    const txId = txMeta.id;
+    const isCompleted = this._isTransactionCompleted(txMeta);
+
+    const finishedPromise = isCompleted
+      ? Promise.resolve(txMeta)
+      : this._waitForTransactionFinished(txId);
+
+    if (!isExisting && !isCompleted) {
+      try {
+        if (requireApproval === false) {
+          await this._updateAndApproveTransaction(txMeta, actionId);
+        } else {
+          await this._requestTransactionApproval(txMeta, { actionId });
+        }
+      } catch (error) {
+        // Errors generated from final status using finished event
+      }
+    }
+
+    const finalTxMeta = await finishedPromise;
+    const finalStatus = finalTxMeta?.status;
+
+    switch (finalStatus) {
+      case TransactionStatus.submitted:
+        return finalTxMeta.hash;
+      case TransactionStatus.rejected:
+        throw cleanErrorStack(
+          ethErrors.provider.userRejectedRequest(
+            'MetaMask Tx Signature: User denied transaction signature.',
+          ),
+        );
+      case TransactionStatus.failed:
+        throw cleanErrorStack(ethErrors.rpc.internal(finalTxMeta.err.message));
+      default:
+        throw cleanErrorStack(
+          ethErrors.rpc.internal(
+            `MetaMask Tx Signature: Unknown problem: ${JSON.stringify(
+              finalTxMeta?.txParams,
+            )}`,
+          ),
+        );
+    }
+  }
+
   /**
    * sets the tx status to approved
    * auto fills the nonce
@@ -2762,6 +2712,22 @@ export default class TransactionController extends EventEmitter {
     );
   }
 
+  /**
+   * Adds a tx to the txlist
+   *
+   * @param txMeta
+   * @fires ${txMeta.id}:unapproved
+   */
+  _addTransaction(txMeta) {
+    this.txStateManager.addTransaction(txMeta);
+    this.emit(`${txMeta.id}:unapproved`, txMeta);
+    this._trackTransactionMetricsEvent(
+      txMeta,
+      TransactionMetaMetricsEvent.added,
+      txMeta.actionId,
+    );
+  }
+
   // Approvals
 
   async _requestTransactionApproval(

app/scripts/metamask-controller.js

@@ -2433,8 +2433,9 @@ export default class MetamaskController extends EventEmitter {
       createSpeedUpTransaction: this.createSpeedUpTransaction.bind(this),
       estimateGas: this.estimateGas.bind(this),
       getNextNonce: this.getNextNonce.bind(this),
-      addUnapprovedTransaction:
+      addTransaction: this.addTransaction.bind(this),
-        txController.addUnapprovedTransaction.bind(txController),
+      addTransactionAndWaitForPublish:
+        this.addTransactionAndWaitForPublish.bind(this),
       createTransactionEventFragment:
         txController.createTransactionEventFragment.bind(txController),
       getTransactions: txController.getTransactions.bind(txController),
@@ -3562,7 +3563,41 @@ export default class MetamaskController extends EventEmitter {
    * @param {object} [req] - The original request, containing the origin.
    */
   async newUnapprovedTransaction(txParams, req) {
-    return await this.txController.newUnapprovedTransaction(txParams, req);
+    // Options are passed explicitly as an additional security measure
+    // to ensure approval is not disabled
+    const { result } = await this.txController.addTransaction(txParams, {
+      actionId: req.id,
+      method: req.method,
+      origin: req.origin,
+      // This is the default behaviour but specified here for clarity
+      requireApproval: true,
+    });
+
+    return await result;
+  }
+
+  async addTransactionAndWaitForPublish(txParams, options) {
+    const { transactionMeta, result } = await this.txController.addTransaction(
+      txParams,
+      options,
+    );
+
+    await result;
+
+    return transactionMeta;
+  }
+
+  async addTransaction(txParams, options) {
+    const { transactionMeta, result } = await this.txController.addTransaction(
+      txParams,
+      options,
+    );
+
+    result.catch(() => {
+      // Not concerned with result
+    });
+
+    return transactionMeta;
   }
 
   /**

ui/ducks/send/send.js

@@ -53,7 +53,7 @@ import {
   isNftOwner,
   getTokenStandardAndDetails,
   showModal,
-  addUnapprovedTransactionAndRouteToConfirmationPage,
+  addTransactionAndRouteToConfirmationPage,
   updateTransactionSendFlowHistory,
   getCurrentNetworkEIP1559Compatibility,
 } from '../../store/actions';
@@ -2332,12 +2332,10 @@ export function signTransaction() {
       );
 
       dispatch(
-        addUnapprovedTransactionAndRouteToConfirmationPage(
+        addTransactionAndRouteToConfirmationPage(txParams, {
-          undefined,
+          sendFlowHistory: draftTransaction.history,
-          txParams,
+          type: transactionType,
-          transactionType,
+        }),
-          draftTransaction.history,
-        ),
       );
     }
   };

ui/ducks/send/send.test.js

@@ -93,8 +93,8 @@ jest.mock('lodash', () => ({
 
 setBackgroundConnection({
   addPollingTokenToAppState: jest.fn(),
-  addUnapprovedTransaction: jest.fn((_u, _v, _w, _x, _y, _z, cb) => {
+  addTransaction: jest.fn((_u, _v, cb) => {
-    cb(null);
+    cb(null, { transactionMeta: null });
   }),
   updateTransactionSendFlowHistory: jest.fn((_x, _y, _z, cb) => cb(null)),
 });
@@ -103,7 +103,7 @@ const getTestUUIDTx = (state) => state.draftTransactions['test-uuid'];
 
 describe('Send Slice', () => {
   let getTokenStandardAndDetailsStub;
-  let addUnapprovedTransactionAndRouteToConfirmationPageStub;
+  let addTransactionAndRouteToConfirmationPageStub;
   beforeEach(() => {
     jest.useFakeTimers();
     getTokenStandardAndDetailsStub = jest
@@ -116,9 +116,9 @@ describe('Send Slice', () => {
           decimals: 18,
         }),
       );
-    addUnapprovedTransactionAndRouteToConfirmationPageStub = jest.spyOn(
+    addTransactionAndRouteToConfirmationPageStub = jest.spyOn(
       Actions,
-      'addUnapprovedTransactionAndRouteToConfirmationPage',
+      'addTransactionAndRouteToConfirmationPage',
     );
     jest
       .spyOn(Actions, 'estimateGas')
@@ -2271,7 +2271,7 @@ describe('Send Slice', () => {
       });
 
       describe('with token transfers', () => {
-        it('should pass the correct transaction parameters to addUnapprovedTransactionAndRouteToConfirmationPage', async () => {
+        it('should pass the correct transaction parameters to addTransactionAndRouteToConfirmationPage', async () => {
           const tokenTransferTxState = {
             metamask: {
               unapprovedTxs: {
@@ -2313,14 +2313,12 @@ describe('Send Slice', () => {
           await store.dispatch(signTransaction());
 
           expect(
-            addUnapprovedTransactionAndRouteToConfirmationPageStub.mock
+            addTransactionAndRouteToConfirmationPageStub.mock.calls[0][0].data,
-              .calls[0][1].data,
           ).toStrictEqual(
             '0xa9059cbb0000000000000000000000004f90e18605fd46f9f9fab0e225d88e1acf5f53240000000000000000000000000000000000000000000000000000000000000001',
           );
           expect(
-            addUnapprovedTransactionAndRouteToConfirmationPageStub.mock
+            addTransactionAndRouteToConfirmationPageStub.mock.calls[0][0].to,
-              .calls[0][1].to,
           ).toStrictEqual('0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2');
         });
       });

ui/ducks/swaps/swaps.js

@@ -6,7 +6,7 @@ import { captureMessage } from '@sentry/browser';
 
 import {
   addToken,
-  addUnapprovedTransaction,
+  addTransactionAndWaitForPublish,
   fetchAndSetQuotes,
   forceUpdateMetamaskState,
   resetSwapsPostFetchState,
@@ -1213,12 +1213,11 @@ export const signAndSendTransactions = (
       }
 
       try {
-        finalApproveTxMeta = await addUnapprovedTransaction(
+        finalApproveTxMeta = await addTransactionAndWaitForPublish(
-          undefined,
           { ...approveTxParams, amount: '0x0' },
-          TransactionType.swapApproval,
           {
             requireApproval: false,
+            type: TransactionType.swapApproval,
             swaps: {
               hasApproveTx: true,
               meta: {
@@ -1236,12 +1235,9 @@ export const signAndSendTransactions = (
     }
 
     try {
-      await addUnapprovedTransaction(
+      await addTransactionAndWaitForPublish(usedTradeTxParams, {
-        undefined,
-        usedTradeTxParams,
-        TransactionType.swap,
-        {
         requireApproval: false,
+        type: TransactionType.swap,
         swaps: {
           hasApproveTx: Boolean(approveTxParams),
           meta: {
@@ -1256,8 +1252,7 @@ export const signAndSendTransactions = (
             approvalTxId: finalApproveTxMeta?.id,
           },
         },
-        },
+      });
-      );
     } catch (e) {
       const errorKey = e.message.includes('EthAppPleaseEnableContractData')
         ? CONTRACT_DATA_DISABLED_ERROR

ui/store/actions.ts

@@ -956,17 +956,18 @@ export function updateTransaction(
  * confirmation page. Returns the newly created txMeta in case additional logic
  * should be applied to the transaction after creation.
  *
- * @param method
  * @param txParams - The transaction parameters
- * @param type - The type of the transaction being added.
+ * @param options
- * @param sendFlowHistory - The history of the send flow at time of creation.
+ * @param options.sendFlowHistory - The history of the send flow at time of creation.
+ * @param options.type - The type of the transaction being added.
  * @returns
  */
-export function addUnapprovedTransactionAndRouteToConfirmationPage(
+export function addTransactionAndRouteToConfirmationPage(
-  method: string,
   txParams: TxParams,
-  type: TransactionType,
+  options?: {
-  sendFlowHistory: DraftTransaction['history'],
+    sendFlowHistory?: DraftTransaction['history'];
+    type?: TransactionType;
+  },
 ): ThunkAction<
   Promise<TransactionMeta | null>,
   MetaMaskReduxState,
@@ -975,15 +976,18 @@ export function addUnapprovedTransactionAndRouteToConfirmationPage(
 > {
   return async (dispatch: MetaMaskReduxDispatch) => {
     const actionId = generateActionId();
+
     try {
-      log.debug('background.addUnapprovedTransaction');
+      log.debug('background.addTransaction');
-      const txMeta = await submitRequestToBackground<TransactionMeta>(
+
-        'addUnapprovedTransaction',
+      const transactionMeta = await submitRequestToBackground<TransactionMeta>(
-        [method, txParams, ORIGIN_METAMASK, type, sendFlowHistory, actionId],
+        'addTransaction',
+        [txParams, { ...options, actionId, origin: ORIGIN_METAMASK }],
         actionId,
       );
+
       dispatch(showConfTxPage());
-      return txMeta;
+      return transactionMeta;
     } catch (error) {
       dispatch(hideLoadingIndication());
       dispatch(displayWarning(error));
@@ -998,33 +1002,41 @@ export function addUnapprovedTransactionAndRouteToConfirmationPage(
  * This method does not show errors or route to a confirmation page and is
  * used primarily for swaps functionality.
  *
- * @param method
  * @param txParams - the transaction parameters
- * @param type - The type of the transaction being added.
  * @param options - Additional options for the transaction.
+ * @param options.method
  * @param options.requireApproval - Whether the transaction requires approval.
  * @param options.swaps - Options specific to swaps transactions.
  * @param options.swaps.hasApproveTx - Whether the swap required an approval transaction.
  * @param options.swaps.meta - Additional transaction metadata required by swaps.
+ * @param options.type
  * @returns
  */
-export async function addUnapprovedTransaction(
+export async function addTransactionAndWaitForPublish(
-  method: string,
   txParams: TxParams,
-  type: TransactionType,
+  options: {
-  options?: {
+    method?: string;
     requireApproval?: boolean;
     swaps?: { hasApproveTx?: boolean; meta?: Record<string, unknown> };
+    type?: TransactionType;
   },
 ): Promise<TransactionMeta> {
-  log.debug('background.addUnapprovedTransaction');
+  log.debug('background.addTransactionAndWaitForPublish');
+
   const actionId = generateActionId();
-  const txMeta = await submitRequestToBackground<TransactionMeta>(
+
-    'addUnapprovedTransaction',
+  return await submitRequestToBackground<TransactionMeta>(
-    [method, txParams, ORIGIN_METAMASK, type, undefined, actionId, options],
+    'addTransactionAndWaitForPublish',
+    [
+      txParams,
+      {
+        ...options,
+        origin: ORIGIN_METAMASK,
+        actionId,
+      },
+    ],
     actionId,
   );
-  return txMeta;
 }
 
 export function updateAndApproveTx(