From 0b1744d4eb74c943ed73f2743250452aa9872316 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Tue, 13 Sep 2022 18:13:38 +0530 Subject: [PATCH] Make createSpeedUpTransaction action idempotent (#15688) --- app/scripts/controllers/transactions/index.js | 15 +++- .../controllers/transactions/index.test.js | 74 +++++++++++++++++++ app/scripts/metamask-controller.js | 10 +-- ui/store/actions.js | 10 +-- 4 files changed, 94 insertions(+), 15 deletions(-) diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 2b18bfe2f..a6a00064d 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -772,7 +772,7 @@ export default class TransactionController extends EventEmitter { ); } - // In transaction is found for same action id, do not create a new transaction. + // If a transaction is found with the same actionId, do not create a new speed-up transaction. if (actionId) { let existingTxMeta = this.txStateManager.getTransactionWithActionId(actionId); @@ -1228,13 +1228,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 createSpeedUpTransaction( originalTxId, customGasSettings, - { estimatedBaseFee } = {}, + { estimatedBaseFee, actionId } = {}, ) { + // If transaction is found for same action id, do not create a new speed-up transaction. + if (actionId) { + const existingTxMeta = + this.txStateManager.getTransactionWithActionId(actionId); + if (existingTxMeta) { + return existingTxMeta; + } + } + const originalTxMeta = this.txStateManager.getTransaction(originalTxId); const { txParams } = originalTxMeta; @@ -1253,6 +1263,7 @@ export default class TransactionController extends EventEmitter { status: TRANSACTION_STATUSES.APPROVED, type: TRANSACTION_TYPES.RETRY, originalType: originalTxMeta.type, + actionId, }); if (estimatedBaseFee) { diff --git a/app/scripts/controllers/transactions/index.test.js b/app/scripts/controllers/transactions/index.test.js index b1db9e697..e75cd19d1 100644 --- a/app/scripts/controllers/transactions/index.test.js +++ b/app/scripts/controllers/transactions/index.test.js @@ -1171,11 +1171,35 @@ describe('Transaction Controller', function () { let approveTransactionSpy; let txParams; let expectedTxParams; + const selectedAddress = '0x1678a085c290ebd122dc42cba69373b5953b831d'; + const recipientAddress = '0xc42edfcc21ed14dda456aa0756c153f7985d8813'; + + let getSelectedAddress, + getPermittedAccounts, + getDefaultGasFees, + getDefaultGasLimit; beforeEach(function () { addTransactionSpy = sinon.spy(txController, 'addTransaction'); approveTransactionSpy = sinon.spy(txController, 'approveTransaction'); + 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]); + txParams = { nonce: '0x00', from: '0xB09d8505E1F4EF1CeA089D47094f5DD3464083d4', @@ -1201,6 +1225,10 @@ describe('Transaction Controller', function () { afterEach(function () { addTransactionSpy.restore(); approveTransactionSpy.restore(); + getSelectedAddress.restore(); + getPermittedAccounts.restore(); + getDefaultGasFees.restore(); + getDefaultGasLimit.restore(); }); it('should call this.addTransaction and this.approveTransaction with the expected args', async function () { @@ -1242,6 +1270,52 @@ describe('Transaction Controller', function () { }, ); }); + + it('should add only 1 speedup 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.createSpeedUpTransaction( + txMeta.id, + {}, + { actionId: 12345 }, + ); + const transactionCount1 = + txController.txStateManager.getTransactions().length; + await txController.createSpeedUpTransaction( + 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.createSpeedUpTransaction( + txMeta.id, + {}, + { actionId: 12345 }, + ); + const transactionCount1 = + txController.txStateManager.getTransactions().length; + await txController.createSpeedUpTransaction( + txMeta.id, + {}, + { actionId: 11111 }, + ); + const transactionCount2 = + txController.txStateManager.getTransactions().length; + assert.equal(transactionCount1 + 1, transactionCount2); + }); }); describe('#signTransaction', function () { diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index b4e1e4a49..0f678d8bd 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -3301,18 +3301,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 createSpeedUpTransaction( - originalTxId, - customGasSettings, - newTxMetaProps, - ) { + async createSpeedUpTransaction(originalTxId, customGasSettings, options) { await this.txController.createSpeedUpTransaction( originalTxId, customGasSettings, - newTxMetaProps, + options, ); const state = await this.getState(); return state; diff --git a/ui/store/actions.js b/ui/store/actions.js index 25e07483e..108f1b316 100644 --- a/ui/store/actions.js +++ b/ui/store/actions.js @@ -2004,19 +2004,16 @@ export function createCancelTransaction(txId, customGasSettings, options) { }; } -export function createSpeedUpTransaction( - txId, - customGasSettings, - newTxMetaProps, -) { +export function createSpeedUpTransaction(txId, customGasSettings, options) { log.debug('background.createSpeedUpTransaction'); let newTx; return (dispatch) => { + const actionId = Date.now() + Math.random(); return new Promise((resolve, reject) => { callBackgroundMethod( 'createSpeedUpTransaction', - [txId, customGasSettings, newTxMetaProps], + [txId, customGasSettings, { ...options, actionId }], (err, newState) => { if (err) { dispatch(displayWarning(err.message)); @@ -2028,6 +2025,7 @@ export function createSpeedUpTransaction( newTx = currentNetworkTxList[currentNetworkTxList.length - 1]; resolve(newState); }, + actionId, ); }) .then((newState) => dispatch(updateMetamaskState(newState)))