From fdc3558988cab3fcc893ac690e89200e2ad1f7a6 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Tue, 25 Jul 2023 09:50:55 +0100 Subject: [PATCH] Use single controller method to add transactions (#20007) --- app/scripts/controllers/transactions/index.js | 268 +++-- .../controllers/transactions/index.test.js | 922 +++++++----------- app/scripts/metamask-controller.js | 41 +- ui/ducks/send/send.js | 12 +- ui/ducks/send/send.test.js | 18 +- ui/ducks/swaps/swaps.js | 43 +- ui/store/actions.ts | 58 +- 7 files changed, 553 insertions(+), 809 deletions(-) diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 37de2830e..0d06fbc99 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/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} the hash of the transaction after being submitted to the network - * @param {object} txParams - txParams for the transaction - * @param {object} opts - with the key origin to put the origin on the txMeta + * @param {object} txParams - Standard parameters for an Ethereum transaction + * @param {object} opts - Options + * @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}>} 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 = {}) { - log.debug( - `MetaMaskController newUnapprovedTransaction ${JSON.stringify(txParams)}`, - ); + async addTransaction( + txParams, + { + actionId, + method, + origin, + requireApproval, + sendFlowHistory, + swaps: { hasApproveTx, meta } = {}, + type, + } = {}, + ) { + log.debug(`MetaMaskController addTransaction ${JSON.stringify(txParams)}`); - const { txMeta: initialTxMeta, isExisting } = await this._createTransaction( - opts.method, - txParams, - opts.origin, - undefined, - undefined, - opts.id, - ); + const { txMeta, isExisting } = await this._createTransaction(txParams, { + actionId, + method, + origin, + sendFlowHistory, + swaps: { hasApproveTx, meta }, + type, + }); - const txId = initialTxMeta.id; - const isCompleted = this._isTransactionCompleted(initialTxMeta); - - const finishedPromise = isCompleted - ? Promise.resolve(initialTxMeta) - : this._waitForTransactionFinished(txId); - - if (!isExisting && !isCompleted) { - try { - 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, - )}`, - ), - ); - } + return { + transactionMeta: txMeta, + result: this._processApproval(txMeta, { + isExisting, + requireApproval, + actionId, + }), + }; } /** @@ -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, - transactionType, - sendFlowHistory = [], - actionId, - options, + { actionId, method, origin, sendFlowHistory = [], swaps, type }, ) { if ( - transactionType !== undefined && - !VALID_UNAPPROVED_TRANSACTION_TYPES.includes(transactionType) + type !== undefined && + !VALID_UNAPPROVED_TRANSACTION_TYPES.includes(type) ) { - throw new Error( - `TransactionController - invalid transactionType value: ${transactionType}`, - ); + throw new Error(`TransactionController - invalid type value: ${type}`); } // 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 ( - [TransactionType.swap, TransactionType.swapApproval].includes( - transactionType, - ) - ) { - txMeta = await this._createSwapsTransaction( - options?.swaps, - transactionType, - txMeta, - ); + if ([TransactionType.swap, TransactionType.swapApproval].includes(type)) { + txMeta = await this._createSwapsTransaction(swaps, type, 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( diff --git a/app/scripts/controllers/transactions/index.test.js b/app/scripts/controllers/transactions/index.test.js index 8600f08dc..9c1f7a131 100644 --- a/app/scripts/controllers/transactions/index.test.js +++ b/app/scripts/controllers/transactions/index.test.js @@ -30,7 +30,6 @@ import { GasEstimateTypes, GasRecommendations, } from '../../../../shared/constants/gas'; -import { METAMASK_CONTROLLER_EVENTS } from '../../metamask-controller'; import { ORIGIN_METAMASK } from '../../../../shared/constants/app'; import { NetworkStatus } from '../../../../shared/constants/network'; import { TRANSACTION_ENVELOPE_TYPE_NAMES } from '../../../../shared/lib/transactions-controller-utils'; @@ -328,14 +327,23 @@ describe('Transaction Controller', function () { }); }); - describe('#newUnapprovedTransaction', function () { - let txMeta, txParams, getPermittedAccounts, signStub; + describe('#addTransaction', function () { + const selectedAddress = '0xc684832530fcbddae4b4230a47e991ddcec2831d'; + const recipientAddress = '0xc684832530fcbddae4b4230a47e991ddcec2831d'; + + let txMeta, + txParams, + getPermittedAccounts, + signStub, + getSelectedAddress, + getDefaultGasFees; beforeEach(function () { txParams = { - from: '0xc684832530fcbddae4b4230a47e991ddcec2831d', - to: '0xc684832530fcbddae4b4230a47e991ddcec2831d', + from: selectedAddress, + to: recipientAddress, }; + txMeta = { status: TransactionStatus.unapproved, id: 1, @@ -343,151 +351,35 @@ describe('Transaction Controller', function () { txParams, history: [{}], }; + txController.txStateManager._addTransactionsToState([txMeta]); + getPermittedAccounts = sinon .stub(txController, 'getPermittedAccounts') .returns([txParams.from]); + + getSelectedAddress = sinon + .stub(txController, 'getSelectedAddress') + .returns(selectedAddress); + + getDefaultGasFees = sinon + .stub(txController, '_getDefaultGasFees') + .returns({}); }); afterEach(function () { txController.txStateManager._addTransactionsToState([]); getPermittedAccounts.restore(); signStub?.restore(); - }); - - it('should resolve when finished and status is submitted and resolve with the hash', async function () { - const hash = await txController.newUnapprovedTransaction(txParams); - assert.ok(hash, 'newUnapprovedTransaction needs to return the hash'); - }); - - it('should reject when finished and status is rejected', async function () { - messengerMock.call.returns( - Promise.reject({ code: errorCodes.provider.userRejectedRequest }), - ); - - await assert.rejects(txController.newUnapprovedTransaction(txParams), { - message: 'MetaMask Tx Signature: User denied transaction signature.', - }); - }); - - it('rejects when finished and status is failed', async function () { - const signError = new Error('TestSigningError'); - - signStub = sinon.stub(txController, 'signEthTx').throws(signError); - - await assert.rejects(txController.newUnapprovedTransaction(txParams), { - message: signError.message, - }); - }); - - it('creates an approval request', async function () { - await txController.newUnapprovedTransaction(txParams); - - const txId = getLastTxMeta().id; - - assert.equal(messengerMock.call.callCount, 1); - assert.deepEqual(messengerMock.call.getCall(0).args, [ - 'ApprovalController:addRequest', - { - id: String(txId), - origin: undefined, - requestData: { txId }, - type: ApprovalType.Transaction, - expectsResult: true, - }, - true, // Show popup - ]); - }); - - describe('if transaction with same actionId exists', function () { - it('does not create an additional approval request', async function () { - await txController.newUnapprovedTransaction(txParams, { id: '12345' }); - await txController.newUnapprovedTransaction(txParams, { id: '12345' }); - - const txId = getLastTxMeta().id; - - assert.equal(messengerMock.call.callCount, 1); - assert.deepEqual(messengerMock.call.getCall(0).args, [ - 'ApprovalController:addRequest', - { - id: String(txId), - origin: undefined, - requestData: { txId }, - type: ApprovalType.Transaction, - expectsResult: true, - }, - true, // Show popup - ]); - }); - - it('does not resolve until transaction approved', async function () { - let firstTransactionResolve; - let firstTransactionCompleted = false; - let secondTransactionCompleted = false; - - messengerMock.call.returns( - new Promise((resolve) => { - firstTransactionResolve = resolve; - }), - ); - - txController - .newUnapprovedTransaction(txParams, { id: '12345' }) - .then(() => { - firstTransactionCompleted = true; - }); - - await flushPromises(); - - txController - .newUnapprovedTransaction(txParams, { id: '12345' }) - .then(() => { - secondTransactionCompleted = true; - }); - - await flushPromises(); - - assert.equal(firstTransactionCompleted, false); - assert.equal(secondTransactionCompleted, false); - - firstTransactionResolve({ value: { txMeta: getLastTxMeta() } }); - - await flushPromises(); - - assert.equal(secondTransactionCompleted, true); - assert.equal(secondTransactionCompleted, true); - }); - }); - }); - - describe('#addUnapprovedTransaction', function () { - const selectedAddress = '0x1678a085c290ebd122dc42cba69373b5953b831d'; - const recipientAddress = '0xc42edfcc21ed14dda456aa0756c153f7985d8813'; - - 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]); - }); - - afterEach(function () { getSelectedAddress.restore(); - getPermittedAccounts.restore(); getDefaultGasFees.restore(); }); - it('should add an unapproved transaction and return a valid txMeta', async function () { - const txMeta = await txController.addUnapprovedTransaction(undefined, { + it('adds an unapproved transaction and returns transaction metadata', async function () { + ({ transactionMeta: txMeta } = await txController.addTransaction({ from: selectedAddress, to: recipientAddress, - }); + })); assert.ok('id' in txMeta, 'should have a id'); assert.ok('time' in txMeta, 'should have a time stamp'); assert.ok( @@ -506,103 +398,18 @@ 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( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - undefined, - undefined, - undefined, - '12345', - ); - const transactionCount1 = - txController.txStateManager.getTransactions().length; - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - undefined, - undefined, - undefined, - '12345', - ); - const transactionCount2 = - txController.txStateManager.getTransactions().length; - assert.equal(transactionCount1, transactionCount2); - }); + it('creates an approval request', async function () { + await txController.addTransaction(txParams); - it('should add multiple transactions when called with different actionId', async function () { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - undefined, - undefined, - undefined, - '12345', - ); - const transactionCount1 = - txController.txStateManager.getTransactions().length; - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - undefined, - undefined, - undefined, - '00000', - ); - const transactionCount2 = - txController.txStateManager.getTransactions().length; - assert.equal(transactionCount1 + 1, transactionCount2); - }); - - it("should fail if the from address isn't the selected address", async function () { - await assert.rejects(() => - txController.addUnapprovedTransaction({ - from: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2', - }), - ); - }); - - it('should fail if the network status is not "available"', async function () { - networkStatusStore.putState(NetworkStatus.Unknown); - await assert.rejects( - () => - txController.addUnapprovedTransaction(undefined, { - from: selectedAddress, - to: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2', - }), - { message: 'MetaMask is having trouble connecting to the network' }, - ); - }); - - it('should create an approval request', async function () { - const txMeta = await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - ); + const txId = getLastTxMeta().id; assert.equal(messengerMock.call.callCount, 1); assert.deepEqual(messengerMock.call.getCall(0).args, [ 'ApprovalController:addRequest', { - id: String(txMeta.id), - origin: ORIGIN_METAMASK, - requestData: { txId: txMeta.id }, + id: String(txId), + origin: undefined, + requestData: { txId }, type: ApprovalType.Transaction, expectsResult: true, }, @@ -610,18 +417,38 @@ describe('Transaction Controller', function () { ]); }); - it('updates meta if transaction type is swap approval', async function () { - await txController.addUnapprovedTransaction( - undefined, + it('throws if the from address is not the selected address', async function () { + await assert.rejects(() => + txController.addTransaction({ + from: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2', + }), + ); + }); + + it('throws if the network status is not available', async function () { + networkStatusStore.putState(NetworkStatus.Unknown); + await assert.rejects( + () => + txController.addTransaction({ + from: selectedAddress, + to: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2', + }), + { message: 'MetaMask is having trouble connecting to the network' }, + ); + }); + + it('updates meta if type is swap approval', async function () { + await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - TransactionType.swapApproval, - undefined, - '12345', - { swaps: { meta: { type: 'swapApproval', sourceTokenSymbol: 'XBN' } } }, + { + origin: ORIGIN_METAMASK, + type: TransactionType.swapApproval, + actionId: '12345', + swaps: { meta: { type: 'swapApproval', sourceTokenSymbol: 'XBN' } }, + }, ); const transaction = txController.getTransactions({ @@ -632,18 +459,16 @@ describe('Transaction Controller', function () { assert.equal(transaction.sourceTokenSymbol, 'XBN'); }); - it('updates meta if transaction type is swap', async function () { - await txController.addUnapprovedTransaction( - undefined, + it('updates meta if type is swap', async function () { + await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - TransactionType.swap, - undefined, - '12345', { + origin: ORIGIN_METAMASK, + type: TransactionType.swap, + actionId: '12345', swaps: { meta: { sourceTokenSymbol: 'BTCX', @@ -687,17 +512,15 @@ describe('Transaction Controller', function () { it('throws error', async function () { await assert.rejects( - txController.addUnapprovedTransaction( - undefined, + txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - TransactionType.swap, - undefined, - '12345', { + origin: ORIGIN_METAMASK, + type: TransactionType.swap, + actionId: '12345', swaps: { hasApproveTx: false, }, @@ -713,17 +536,15 @@ describe('Transaction Controller', function () { txController.on('tx:status-update', listener); try { - await txController.addUnapprovedTransaction( - undefined, + await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - TransactionType.swap, - undefined, - '12345', { + origin: ORIGIN_METAMASK, + type: TransactionType.swap, + actionId: '12345', swaps: { hasApproveTx: false, }, @@ -742,39 +563,42 @@ describe('Transaction Controller', function () { }); }); - describe('if transaction with same actionId exists', function () { - it('does not create an additional approval request', async function () { - await txController.addUnapprovedTransaction( - undefined, + describe('with actionId', function () { + it('adds single unapproved transaction when called twice with same actionId', async function () { + await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', + { actionId: '12345' }, ); + const transactionCount1 = + txController.txStateManager.getTransactions().length; + await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { actionId: '12345' }, + ); + const transactionCount2 = + txController.txStateManager.getTransactions().length; + assert.equal(transactionCount1, transactionCount2); + }); - const secondTxMeta = await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - undefined, - undefined, - undefined, - '12345', - ); + it('adds single approval request when called twice with same actionId', async function () { + await txController.addTransaction(txParams, { actionId: '12345' }); + await txController.addTransaction(txParams, { actionId: '12345' }); + + const txId = getLastTxMeta().id; assert.equal(messengerMock.call.callCount, 1); assert.deepEqual(messengerMock.call.getCall(0).args, [ 'ApprovalController:addRequest', { - id: String(secondTxMeta.id), - origin: ORIGIN_METAMASK, - requestData: { txId: secondTxMeta.id }, + id: String(txId), + origin: undefined, + requestData: { txId }, type: ApprovalType.Transaction, expectsResult: true, }, @@ -782,7 +606,29 @@ describe('Transaction Controller', function () { ]); }); - it('does not resolve until transaction approved', async function () { + it('adds multiple transactions when called with different actionId', async function () { + await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { actionId: '12345' }, + ); + const transactionCount1 = + txController.txStateManager.getTransactions().length; + await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { actionId: '00000' }, + ); + const transactionCount2 = + txController.txStateManager.getTransactions().length; + assert.equal(transactionCount1 + 1, transactionCount2); + }); + + it('resolves second result when first transaction is finished', async function () { let firstTransactionResolve; let firstTransactionCompleted = false; let secondTransactionCompleted = false; @@ -793,39 +639,23 @@ describe('Transaction Controller', function () { }), ); - txController - .addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ) - .then(() => { - firstTransactionCompleted = true; - }); + const { result: firstResult } = await txController.addTransaction( + txParams, + { actionId: '12345' }, + ); - await flushPromises(); + firstResult.then(() => { + firstTransactionCompleted = true; + }); - txController - .addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - undefined, - undefined, - undefined, - '12345', - ) - .then(() => { - secondTransactionCompleted = true; - }); + const { result: secondResult } = await txController.addTransaction( + txParams, + { actionId: '12345' }, + ); + + secondResult.then(() => { + secondTransactionCompleted = true; + }); await flushPromises(); @@ -835,26 +665,32 @@ describe('Transaction Controller', function () { firstTransactionResolve({ value: { txMeta: getLastTxMeta() } }); await flushPromises(); + await firstResult; + await secondResult; - assert.equal(secondTransactionCompleted, true); + assert.equal(firstTransactionCompleted, true); assert.equal(secondTransactionCompleted, true); }); }); - describe('on approval', function () { + describe('on success', function () { + it('resolves result with the transaction hash', async function () { + const { result } = await txController.addTransaction(txParams); + const hash = await result; + assert.ok(hash, 'addTransaction needs to return the hash'); + }); + it('changes status to submitted', async function () { - await txController.addUnapprovedTransaction( - undefined, + const { result } = await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', + { origin: ORIGIN_METAMASK, actionId: '12345' }, ); + await result; + const transaction = txController.getTransactions({ searchCriteria: { id: getLastTxMeta().id }, })[0]; @@ -867,18 +703,16 @@ describe('Transaction Controller', function () { txController.on('tx:status-update', listener); - await txController.addUnapprovedTransaction( - undefined, + const { result } = await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', + { origin: ORIGIN_METAMASK, actionId: '12345' }, ); + await result; + const txId = getLastTxMeta().id; assert.equal(listener.callCount, 3); @@ -891,18 +725,16 @@ describe('Transaction Controller', function () { }); it('reports success to approval request acceptor', async function () { - await txController.addUnapprovedTransaction( - undefined, + const { result } = await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', + { origin: ORIGIN_METAMASK, actionId: '12345' }, ); + await result; + assert.equal(resultCallbacksMock.success.callCount, 1); }); @@ -913,7 +745,7 @@ describe('Transaction Controller', function () { providerResultStub.eth_gasPrice = wrongValue; providerResultStub.eth_estimateGas = '0x5209'; - const signStub = sinon + signStub = sinon .stub(txController, 'signTransaction') .callsFake(() => Promise.resolve()); @@ -925,8 +757,7 @@ describe('Transaction Controller', function () { txController.txStateManager.setTxStatusSubmitted(txId); }); - await txController.addUnapprovedTransaction( - undefined, + const { result } = await txController.addTransaction( { from: selectedAddress, to: recipientAddress, @@ -934,21 +765,20 @@ describe('Transaction Controller', function () { gas: originalValue, gasPrice: originalValue, }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', + { origin: ORIGIN_METAMASK, actionId: '12345' }, ); + await result; + const txId = getLastTxMeta().id; - const result = txController.txStateManager.getTransaction(txId); - const params = result.txParams; + const finalMeta = txController.txStateManager.getTransaction(txId); + const params = finalMeta.txParams; assert.equal(params.gas, originalValue, 'gas unmodified'); assert.equal(params.gasPrice, originalValue, 'gas price unmodified'); - assert.equal(result.hash, originalValue); + assert.equal(finalMeta.hash, originalValue); assert.equal( - result.status, + finalMeta.status, TransactionStatus.submitted, 'should have reached the submitted status.', ); @@ -965,21 +795,19 @@ describe('Transaction Controller', function () { ); }); - it('throws error', async function () { - await assert.rejects( - txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ), - { code: ethErrors.provider.userRejectedRequest().code }, + it('rejects result', async function () { + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, ); + + await assert.rejects(result, { + code: ethErrors.provider.userRejectedRequest().code, + message: 'MetaMask Tx Signature: User denied transaction signature.', + }); }); it('emits rejected status event', async function () { @@ -987,18 +815,16 @@ describe('Transaction Controller', function () { txController.on('tx:status-update', listener); + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionid: '12345' }, + ); + try { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ); + await result; } catch (error) { // Expected error } @@ -1014,7 +840,6 @@ describe('Transaction Controller', function () { describe('on signing error', function () { const signError = new Error('TestSignError'); - let signStub; beforeEach(async function () { signStub = sinon.stub(txController, 'signEthTx').throws(signError); @@ -1025,18 +850,16 @@ describe('Transaction Controller', function () { }); it('changes status to failed', async function () { + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, + ); + try { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ); + await result; } catch { // Expected error } @@ -1048,21 +871,16 @@ describe('Transaction Controller', function () { assert.equal(transaction.status, TransactionStatus.failed); }); - it('throws error', async function () { - await assert.rejects( - txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ), - signError, + it('rejects result', async function () { + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, ); + + await assert.rejects(result, signError); }); it('emits approved and failed status events', async function () { @@ -1070,18 +888,16 @@ describe('Transaction Controller', function () { txController.on('tx:status-update', listener); + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, + ); + try { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ); + await result; } catch (error) { // Expected error } @@ -1096,18 +912,16 @@ describe('Transaction Controller', function () { }); it('reports error to approval request acceptor', async function () { + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, + ); + try { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ); + await result; } catch { // Expected error } @@ -1137,18 +951,16 @@ describe('Transaction Controller', function () { }); it('changes status to failed', async function () { + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, + ); + try { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ); + await result; } catch { // Expected error } @@ -1160,21 +972,16 @@ describe('Transaction Controller', function () { assert.equal(transaction.status, TransactionStatus.failed); }); - it('throws error', async function () { - await assert.rejects( - txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ), - publishError, + it('rejects result', async function () { + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, ); + + await assert.rejects(result, publishError); }); it('emits approved, signed, and failed status events', async function () { @@ -1182,18 +989,16 @@ describe('Transaction Controller', function () { txController.on('tx:status-update', listener); + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, + ); + try { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ); + await result; } catch (error) { // Expected error } @@ -1210,18 +1015,16 @@ describe('Transaction Controller', function () { }); it('reports error to approval request acceptor', async function () { + const { result } = await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { origin: ORIGIN_METAMASK, actionId: '12345' }, + ); + try { - await txController.addUnapprovedTransaction( - undefined, - { - from: selectedAddress, - to: recipientAddress, - }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - ); + await result; } catch { // Expected error } @@ -1240,20 +1043,29 @@ describe('Transaction Controller', function () { messengerMock.call.callsFake(() => Promise.reject()); }); + it('resolves result with the transaction hash', async function () { + const { result } = await txController.addTransaction(txParams, { + requireApproval: false, + }); + const hash = await result; + assert.ok(hash, 'addTransaction needs to return the hash'); + }); + it('changes status to submitted', async function () { - await txController.addUnapprovedTransaction( - undefined, + const { result } = await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - { requireApproval: false }, + { + origin: ORIGIN_METAMASK, + actionid: '12345', + requireApproval: false, + }, ); + await result; + const transaction = txController.getTransactions({ searchCriteria: { id: getLastTxMeta().id }, })[0]; @@ -1266,19 +1078,20 @@ describe('Transaction Controller', function () { txController.on('tx:status-update', listener); - await txController.addUnapprovedTransaction( - undefined, + const { result } = await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, - ORIGIN_METAMASK, - undefined, - undefined, - '12345', - { requireApproval: false }, + { + origin: ORIGIN_METAMASK, + actionId: '12345', + requireApproval: false, + }, ); + await result; + const txId = getLastTxMeta().id; assert.equal(listener.callCount, 3); @@ -1326,11 +1139,13 @@ describe('Transaction Controller', function () { getDefaultGasLimit.restore(); }); - it('should add an cancel transaction and return a valid txMeta', async function () { - const txMeta = await txController.addUnapprovedTransaction(undefined, { - from: selectedAddress, - to: recipientAddress, - }); + it('should add a cancel transaction and return a valid txMeta', async function () { + const { transactionMeta: txMeta, result } = + await txController.addTransaction({ + from: selectedAddress, + to: recipientAddress, + }); + await result; const cancelTxMeta = await txController.createCancelTransaction( txMeta.id, {}, @@ -1345,10 +1160,12 @@ describe('Transaction Controller', function () { }); it('should add only 1 cancel transaction when called twice with same actionId', async function () { - const txMeta = await txController.addUnapprovedTransaction(undefined, { - from: selectedAddress, - to: recipientAddress, - }); + const { transactionMeta: txMeta, result } = + await txController.addTransaction({ + from: selectedAddress, + to: recipientAddress, + }); + await result; await txController.createCancelTransaction( txMeta.id, {}, @@ -1367,10 +1184,12 @@ describe('Transaction Controller', function () { }); it('should add multiple transactions when called with different actionId', async function () { - const txMeta = await txController.addUnapprovedTransaction(undefined, { - from: selectedAddress, - to: recipientAddress, - }); + const { transactionMeta: txMeta, result } = + await txController.addTransaction({ + from: selectedAddress, + to: recipientAddress, + }); + await result; await txController.createCancelTransaction( txMeta.id, {}, @@ -1710,94 +1529,9 @@ describe('Transaction Controller', function () { }); }); - describe('#addTransaction', function () { - let trackTransactionMetricsEventSpy; - - beforeEach(function () { - trackTransactionMetricsEventSpy = sinon.spy( - txController, - '_trackTransactionMetricsEvent', - ); - }); - - afterEach(function () { - trackTransactionMetricsEventSpy.restore(); - }); - - it('should emit updates', function (done) { - const txMeta = { - id: '1', - status: TransactionStatus.unapproved, - metamaskNetworkId: currentNetworkId, - txParams: { - to: VALID_ADDRESS, - from: VALID_ADDRESS_TWO, - }, - }; - - const eventNames = [ - METAMASK_CONTROLLER_EVENTS.UPDATE_BADGE, - '1:unapproved', - ]; - const listeners = []; - eventNames.forEach((eventName) => { - listeners.push( - new Promise((resolve) => { - txController.once(eventName, (arg) => { - resolve(arg); - }); - }), - ); - }); - Promise.all(listeners) - .then((returnValues) => { - assert.deepEqual( - returnValues.pop(), - txMeta, - 'last event 1:unapproved should return txMeta', - ); - done(); - }) - .catch(done); - txController.addTransaction(txMeta); - }); - - it('should call _trackTransactionMetricsEvent with the correct params', function () { - const txMeta = { - id: 1, - status: TransactionStatus.unapproved, - txParams: { - from: fromAccount.address, - to: '0x1678a085c290ebd122dc42cba69373b5953b831d', - gasPrice: '0x77359400', - gas: '0x7b0d', - nonce: '0x4b', - }, - type: TransactionType.simpleSend, - transaction_envelope_type: TRANSACTION_ENVELOPE_TYPE_NAMES.LEGACY, - origin: ORIGIN_METAMASK, - chainId: currentChainId, - time: 1624408066355, - metamaskNetworkId: currentNetworkId, - }; - - txController.addTransaction(txMeta); - - assert.equal(trackTransactionMetricsEventSpy.callCount, 1); - assert.deepEqual( - trackTransactionMetricsEventSpy.getCall(0).args[0], - txMeta, - ); - assert.equal( - trackTransactionMetricsEventSpy.getCall(0).args[1], - TransactionMetaMetricsEvent.added, - ); - }); - }); - describe('#sign replay-protected tx', function () { it('prepares a tx with the chainId set', async function () { - txController.addTransaction( + txController._addTransaction( { id: '1', status: TransactionStatus.unapproved, @@ -1847,7 +1581,7 @@ describe('Transaction Controller', function () { getDefaultGasLimit; beforeEach(function () { - addTransactionSpy = sinon.spy(txController, 'addTransaction'); + addTransactionSpy = sinon.spy(txController, '_addTransaction'); approveTransactionSpy = sinon.spy(txController, '_approveTransaction'); const hash = @@ -1940,10 +1674,12 @@ describe('Transaction Controller', function () { }); it('should add only 1 speedup transaction when called twice with same actionId', async function () { - const txMeta = await txController.addUnapprovedTransaction(undefined, { - from: selectedAddress, - to: recipientAddress, - }); + const { transactionMeta: txMeta, result } = + await txController.addTransaction({ + from: selectedAddress, + to: recipientAddress, + }); + await result; await txController.createSpeedUpTransaction( txMeta.id, {}, @@ -1962,10 +1698,12 @@ describe('Transaction Controller', function () { }); it('should add multiple transactions when called with different actionId', async function () { - const txMeta = await txController.addUnapprovedTransaction(undefined, { - from: selectedAddress, - to: recipientAddress, - }); + const { transactionMeta: txMeta, result } = + await txController.addTransaction({ + from: selectedAddress, + to: recipientAddress, + }); + await result; await txController.createSpeedUpTransaction( txMeta.id, {}, @@ -1984,13 +1722,15 @@ describe('Transaction Controller', function () { }); it('should add multiple transactions when called with different actionId and txMethodType defined', async function () { - const txMeta = await txController.addUnapprovedTransaction( - 'eth_sendTransaction', - { - from: selectedAddress, - to: recipientAddress, - }, - ); + const { transactionMeta: txMeta, result } = + await txController.addTransaction( + { + from: selectedAddress, + to: recipientAddress, + }, + { method: 'eth_sendTransaction' }, + ); + await result; await txController.createSpeedUpTransaction( txMeta.id, {}, @@ -2009,12 +1749,12 @@ describe('Transaction Controller', function () { }); it('should call securityProviderRequest and have flagAsDangerous inside txMeta', async function () { - const txMeta = await txController.addUnapprovedTransaction( - 'eth_sendTransaction', + const { transactionMeta: txMeta } = await txController.addTransaction( { from: selectedAddress, to: recipientAddress, }, + { method: 'eth_sendTransaction' }, ); assert.ok( @@ -3490,8 +3230,8 @@ describe('Transaction Controller', function () { }, }; - txController.addTransaction(firstTxMeta); - txController.addTransaction(secondTxMeta); + txController._addTransaction(firstTxMeta); + txController._addTransaction(secondTxMeta); await txController.initApprovals(); @@ -3535,7 +3275,7 @@ describe('Transaction Controller', function () { }, }; - txController.addTransaction(txMeta); + txController._addTransaction(txMeta); const transaction1 = txController.updateTransactionSendFlowHistory( txId, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5c5c64fc1..d58adf26c 100644 --- a/app/scripts/metamask-controller.js +++ b/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: - txController.addUnapprovedTransaction.bind(txController), + addTransaction: this.addTransaction.bind(this), + 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; } /** diff --git a/ui/ducks/send/send.js b/ui/ducks/send/send.js index 45a681eb9..f63e4a13a 100644 --- a/ui/ducks/send/send.js +++ b/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( - undefined, - txParams, - transactionType, - draftTransaction.history, - ), + addTransactionAndRouteToConfirmationPage(txParams, { + sendFlowHistory: draftTransaction.history, + type: transactionType, + }), ); } }; diff --git a/ui/ducks/send/send.test.js b/ui/ducks/send/send.test.js index cb10c7a87..904a181ab 100644 --- a/ui/ducks/send/send.test.js +++ b/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) => { - cb(null); + addTransaction: jest.fn((_u, _v, cb) => { + 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 - .calls[0][1].data, + addTransactionAndRouteToConfirmationPageStub.mock.calls[0][0].data, ).toStrictEqual( '0xa9059cbb0000000000000000000000004f90e18605fd46f9f9fab0e225d88e1acf5f53240000000000000000000000000000000000000000000000000000000000000001', ); expect( - addUnapprovedTransactionAndRouteToConfirmationPageStub.mock - .calls[0][1].to, + addTransactionAndRouteToConfirmationPageStub.mock.calls[0][0].to, ).toStrictEqual('0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2'); }); }); diff --git a/ui/ducks/swaps/swaps.js b/ui/ducks/swaps/swaps.js index 1c3501cb2..c9907b370 100644 --- a/ui/ducks/swaps/swaps.js +++ b/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( - undefined, + finalApproveTxMeta = await addTransactionAndWaitForPublish( { ...approveTxParams, amount: '0x0' }, - TransactionType.swapApproval, { requireApproval: false, + type: TransactionType.swapApproval, swaps: { hasApproveTx: true, meta: { @@ -1236,28 +1235,24 @@ export const signAndSendTransactions = ( } try { - await addUnapprovedTransaction( - undefined, - usedTradeTxParams, - TransactionType.swap, - { - requireApproval: false, - swaps: { - hasApproveTx: Boolean(approveTxParams), - meta: { - estimatedBaseFee: decEstimatedBaseFee, - sourceTokenSymbol: sourceTokenInfo.symbol, - destinationTokenSymbol: destinationTokenInfo.symbol, - type: TransactionType.swap, - destinationTokenDecimals: destinationTokenInfo.decimals, - destinationTokenAddress: destinationTokenInfo.address, - swapMetaData, - swapTokenValue, - approvalTxId: finalApproveTxMeta?.id, - }, + await addTransactionAndWaitForPublish(usedTradeTxParams, { + requireApproval: false, + type: TransactionType.swap, + swaps: { + hasApproveTx: Boolean(approveTxParams), + meta: { + estimatedBaseFee: decEstimatedBaseFee, + sourceTokenSymbol: sourceTokenInfo.symbol, + destinationTokenSymbol: destinationTokenInfo.symbol, + type: TransactionType.swap, + destinationTokenDecimals: destinationTokenInfo.decimals, + destinationTokenAddress: destinationTokenInfo.address, + swapMetaData, + swapTokenValue, + approvalTxId: finalApproveTxMeta?.id, }, }, - ); + }); } catch (e) { const errorKey = e.message.includes('EthAppPleaseEnableContractData') ? CONTRACT_DATA_DISABLED_ERROR diff --git a/ui/store/actions.ts b/ui/store/actions.ts index d9a145806..1ebd83eac 100644 --- a/ui/store/actions.ts +++ b/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 sendFlowHistory - The history of the send flow at time of creation. + * @param options + * @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( - method: string, +export function addTransactionAndRouteToConfirmationPage( txParams: TxParams, - type: TransactionType, - sendFlowHistory: DraftTransaction['history'], + options?: { + sendFlowHistory?: DraftTransaction['history']; + type?: TransactionType; + }, ): ThunkAction< Promise, MetaMaskReduxState, @@ -975,15 +976,18 @@ export function addUnapprovedTransactionAndRouteToConfirmationPage( > { return async (dispatch: MetaMaskReduxDispatch) => { const actionId = generateActionId(); + try { - log.debug('background.addUnapprovedTransaction'); - const txMeta = await submitRequestToBackground( - 'addUnapprovedTransaction', - [method, txParams, ORIGIN_METAMASK, type, sendFlowHistory, actionId], + log.debug('background.addTransaction'); + + const transactionMeta = await submitRequestToBackground( + '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( - method: string, +export async function addTransactionAndWaitForPublish( txParams: TxParams, - type: TransactionType, - options?: { + options: { + method?: string; requireApproval?: boolean; swaps?: { hasApproveTx?: boolean; meta?: Record }; + type?: TransactionType; }, ): Promise { - log.debug('background.addUnapprovedTransaction'); + log.debug('background.addTransactionAndWaitForPublish'); + const actionId = generateActionId(); - const txMeta = await submitRequestToBackground( - 'addUnapprovedTransaction', - [method, txParams, ORIGIN_METAMASK, type, undefined, actionId, options], + + return await submitRequestToBackground( + 'addTransactionAndWaitForPublish', + [ + txParams, + { + ...options, + origin: ORIGIN_METAMASK, + actionId, + }, + ], actionId, ); - return txMeta; } export function updateAndApproveTx(