diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index a2cd6b03df2..e6cefaffa0e 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Bump `@metamask/core-backend` from `^6.1.1` to `^6.2.0` ([#8232](https://github.com/MetaMask/core/pull/8232)) +- Remove legacy existing transaction deduplication logic based on `actionId` ([#8256](https://github.com/MetaMask/core/pull/8256)) + - `addTransaction` no longer reuses an existing transaction when called with a previously used `actionId`. A new transaction is always created. + - `stopTransaction` and `speedUpTransaction` no longer skip execution when called with a previously used `actionId`. + - The `actionId` property is still persisted on `TransactionMeta` and included in event payloads. ## [63.0.0] diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 9a03533d141..1ce42ca440f 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -1532,219 +1532,6 @@ describe('TransactionController', () => { }); }); - describe('with actionId', () => { - it('adds single unapproved transaction when called twice with same actionId', async () => { - const { controller, mockTransactionApprovalRequest } = setupController(); - - const mockOrigin = 'origin'; - - await controller.addTransaction( - { - from: ACCOUNT_MOCK, - to: ACCOUNT_MOCK, - }, - { - origin: mockOrigin, - actionId: ACTION_ID_MOCK, - networkClientId: NETWORK_CLIENT_ID_MOCK, - }, - ); - - const firstTransactionCount = controller.state.transactions.length; - - await controller.addTransaction( - { - from: ACCOUNT_MOCK, - to: ACCOUNT_MOCK, - }, - { - origin: mockOrigin, - actionId: ACTION_ID_MOCK, - networkClientId: NETWORK_CLIENT_ID_MOCK, - }, - ); - const secondTransactionCount = controller.state.transactions.length; - - expect(firstTransactionCount).toStrictEqual(secondTransactionCount); - expect( - mockTransactionApprovalRequest.actionHandlerMock, - ).toHaveBeenCalledTimes(1); - expect( - mockTransactionApprovalRequest.actionHandlerMock, - ).toHaveBeenCalledWith( - { - id: expect.any(String), - origin: mockOrigin, - type: 'transaction', - requestData: { txId: expect.any(String) }, - expectsResult: true, - }, - true, - ); - }); - - it('adds multiple transactions with same actionId and ensures second transaction result does not resolves before the first transaction result', async () => { - const { controller, mockTransactionApprovalRequest } = setupController(); - - const mockOrigin = 'origin'; - let firstTransactionCompleted = false; - let secondTransactionCompleted = false; - - const { result: firstResult } = await controller.addTransaction( - { - from: ACCOUNT_MOCK, - to: ACCOUNT_MOCK, - }, - { - origin: mockOrigin, - actionId: ACTION_ID_MOCK, - networkClientId: NETWORK_CLIENT_ID_MOCK, - }, - ); - - firstResult - .then(() => { - firstTransactionCompleted = true; - return undefined; - }) - .catch(() => undefined); - - const { result: secondResult } = await controller.addTransaction( - { - from: ACCOUNT_MOCK, - to: ACCOUNT_MOCK, - }, - { - origin: mockOrigin, - actionId: ACTION_ID_MOCK, - networkClientId: NETWORK_CLIENT_ID_MOCK, - }, - ); - secondResult - .then(() => { - secondTransactionCompleted = true; - return undefined; - }) - .catch(() => undefined); - - await wait(0); - - expect(firstTransactionCompleted).toBe(false); - expect(secondTransactionCompleted).toBe(false); - - mockTransactionApprovalRequest.approve(); - await firstResult; - await secondResult; - - expect(firstTransactionCompleted).toBe(true); - expect(secondTransactionCompleted).toBe(true); - }); - - it.each([ - [ - 'does not add duplicate transaction if actionId already used', - ACTION_ID_MOCK, - ACTION_ID_MOCK, - 1, - ], - [ - 'adds additional transaction if actionId not used', - ACTION_ID_MOCK, - '00000', - 2, - ], - ])( - '%s', - async (_, firstActionId, secondActionId, expectedTransactionCount) => { - const { controller, mockTransactionApprovalRequest } = - setupController(); - const expectedRequestApprovalCalledTimes = expectedTransactionCount; - - const mockOrigin = 'origin'; - - await controller.addTransaction( - { - from: ACCOUNT_MOCK, - to: ACCOUNT_MOCK, - }, - { - origin: mockOrigin, - actionId: firstActionId, - networkClientId: NETWORK_CLIENT_ID_MOCK, - }, - ); - - await controller.addTransaction( - { - from: ACCOUNT_MOCK, - to: ACCOUNT_MOCK, - }, - { - origin: mockOrigin, - actionId: secondActionId, - networkClientId: NETWORK_CLIENT_ID_MOCK, - }, - ); - const { transactions } = controller.state; - - expect(transactions).toHaveLength(expectedTransactionCount); - expect( - mockTransactionApprovalRequest.actionHandlerMock, - ).toHaveBeenCalledTimes(expectedRequestApprovalCalledTimes); - }, - ); - - it.each([ - [ - 'adds single transaction when speed up called twice with the same actionId', - ACTION_ID_MOCK, - 2, - 1, - ], - [ - 'adds multiple transactions when speed up called with non-existent actionId', - '00000', - 3, - 2, - ], - ])( - '%s', - async ( - _, - actionId, - expectedTransactionCount, - expectedSignCalledTimes, - ) => { - const { controller } = setupController(); - - const { transactionMeta } = await controller.addTransaction( - { - from: ACCOUNT_MOCK, - gas: '0x0', - gasPrice: '0x50fd51da', - to: ACCOUNT_MOCK, - value: '0x0', - }, - { - networkClientId: NETWORK_CLIENT_ID_MOCK, - }, - ); - - await controller.speedUpTransaction(transactionMeta.id, undefined, { - actionId: ACTION_ID_MOCK, - }); - - await controller.speedUpTransaction(transactionMeta.id, undefined, { - actionId, - }); - - const { transactions } = controller.state; - expect(transactions).toHaveLength(expectedTransactionCount); - expect(signMock).toHaveBeenCalledTimes(expectedSignCalledTimes); - }, - ); - }); - describe('addTransaction', () => { it('adds unapproved transaction to state', async () => { const { controller } = setupController(); @@ -3978,36 +3765,6 @@ describe('TransactionController', () => { }); describe('stopTransaction', () => { - it('should avoid creating cancel transaction if actionId already exist', async () => { - const mockActionId = 'mockActionId'; - const { controller } = setupController({ - options: { - state: { - transactions: [ - { - actionId: mockActionId, - id: '2', - chainId: toHex(5), - networkClientId: NETWORK_CLIENT_ID_MOCK, - status: TransactionStatus.submitted, - type: TransactionType.cancel, - time: 123456789, - txParams: { - from: ACCOUNT_MOCK, - }, - }, - ], - }, - }, - }); - - await controller.stopTransaction('2', undefined, { - actionId: mockActionId, - }); - - expect(controller.state.transactions).toHaveLength(1); - }); - it('should throw error if transaction already confirmed', async () => { const { controller } = setupController({ options: { @@ -4366,36 +4123,6 @@ describe('TransactionController', () => { expect(speedUpTransaction.type).toBe(TransactionType.retry); }); - it('should avoid creating speedup transaction if actionId already exist', async () => { - const mockActionId = 'mockActionId'; - const { controller } = setupController({ - options: { - state: { - transactions: [ - { - actionId: mockActionId, - id: '2', - networkClientId: NETWORK_CLIENT_ID_MOCK, - chainId: toHex(5), - status: TransactionStatus.submitted, - type: TransactionType.retry, - time: 123456789, - txParams: { - from: ACCOUNT_MOCK, - }, - }, - ], - }, - }, - }); - - await controller.speedUpTransaction('2', undefined, { - actionId: mockActionId, - }); - - expect(controller.state.transactions).toHaveLength(1); - }); - it('should throw error if transaction already confirmed', async () => { const { controller } = setupController({ options: { diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index c059e498d55..2ea459dc65f 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -1284,40 +1284,34 @@ export class TransactionController extends BaseController< const transactionType = type ?? (await determineTransactionType(txParams, ethQuery)).type; - const existingTransactionMeta = this.#getTransactionWithActionId(actionId); - - // If a request to add a transaction with the same actionId is submitted again, a new transaction will not be created for it. - let addedTransactionMeta: TransactionMeta = existingTransactionMeta - ? cloneDeep(existingTransactionMeta) - : { - // Add actionId to txMeta to check if same actionId is seen again - actionId, - assetsFiatValues, - batchId, - chainId, - dappSuggestedGasFees, - deviceConfirmedOn, - disableGasBuffer, - id: random(), - isGasFeeTokenIgnoredIfBalance: Boolean(gasFeeToken), - isGasFeeIncluded, - isGasFeeSponsored, - isFirstTimeInteraction: undefined, - isStateOnly, - nestedTransactions, - networkClientId, - origin, - requestId, - requiredAssets, - securityAlertResponse, - selectedGasFeeToken: gasFeeToken, - status: TransactionStatus.unapproved as const, - time: Date.now(), - txParams, - type: transactionType, - userEditedGasLimit: false, - verifiedOnBlockchain: false, - }; + let addedTransactionMeta: TransactionMeta = { + actionId, + assetsFiatValues, + batchId, + chainId, + dappSuggestedGasFees, + deviceConfirmedOn, + disableGasBuffer, + id: random(), + isGasFeeTokenIgnoredIfBalance: Boolean(gasFeeToken), + isGasFeeIncluded, + isGasFeeSponsored, + isFirstTimeInteraction: undefined, + isStateOnly, + nestedTransactions, + networkClientId, + origin, + requestId, + requiredAssets, + securityAlertResponse, + selectedGasFeeToken: gasFeeToken, + status: TransactionStatus.unapproved as const, + time: Date.now(), + txParams, + type: transactionType, + userEditedGasLimit: false, + verifiedOnBlockchain: false, + }; const { updateTransaction } = await this.#afterAdd({ transactionMeta: addedTransactionMeta, @@ -1368,85 +1362,80 @@ export class TransactionController extends BaseController< .catch(noop); } - // Checks if a transaction already exists with a given actionId - if (!existingTransactionMeta) { - // Set security provider response - if (method && this.#securityProviderRequest) { - const securityProviderResponse = await this.#securityProviderRequest( - addedTransactionMeta, - method, - ); - // eslint-disable-next-line require-atomic-updates - addedTransactionMeta.securityProviderResponse = - securityProviderResponse; - } - - addedTransactionMeta = updateSwapsTransaction( + // Set security provider response + if (method && this.#securityProviderRequest) { + const securityProviderResponse = await this.#securityProviderRequest( addedTransactionMeta, - transactionType, - swaps, - { - isSwapsDisabled: this.#isSwapsDisabled, - cancelTransaction: this.#rejectTransaction.bind(this), - messenger: this.messenger, - }, + method, ); + // eslint-disable-next-line require-atomic-updates + addedTransactionMeta.securityProviderResponse = securityProviderResponse; + } - this.#addMetadata(addedTransactionMeta); - - delegationAddressPromise - .then((delegationAddress) => { - this.#updateTransactionInternal( - { - transactionId: addedTransactionMeta.id, - skipResimulateCheck: true, - skipValidation: true, - }, - (tx) => { - tx.delegationAddress = delegationAddress; - }, - ); + addedTransactionMeta = updateSwapsTransaction( + addedTransactionMeta, + transactionType, + swaps, + { + isSwapsDisabled: this.#isSwapsDisabled, + cancelTransaction: this.#rejectTransaction.bind(this), + messenger: this.messenger, + }, + ); - return undefined; - }) - .catch(noop); + this.#addMetadata(addedTransactionMeta); + + delegationAddressPromise + .then((delegationAddress) => { + this.#updateTransactionInternal( + { + transactionId: addedTransactionMeta.id, + skipResimulateCheck: true, + skipValidation: true, + }, + (tx) => { + tx.delegationAddress = delegationAddress; + }, + ); - if (requireApproval !== false && !isStateOnly) { - this.#updateSimulationData(addedTransactionMeta, { - traceContext, - }).catch((error) => { - log('Error while updating simulation data', error); - throw error; - }); + return undefined; + }) + .catch(noop); - updateFirstTimeInteraction({ - existingTransactions: this.state.transactions, - getTransaction: (transactionId: string) => - this.#getTransaction(transactionId), - isFirstTimeInteractionEnabled: this.#isFirstTimeInteractionEnabled, - trace: this.#trace, - traceContext, - transactionMeta: addedTransactionMeta, - updateTransaction: this.#updateTransactionInternal.bind(this), - }).catch((error) => { - log('Error while updating first interaction properties', error); - }); - } else { - log( - 'Skipping simulation & first interaction update as approval not required', - ); - } + if (requireApproval !== false && !isStateOnly) { + this.#updateSimulationData(addedTransactionMeta, { + traceContext, + }).catch((error) => { + log('Error while updating simulation data', error); + throw error; + }); - this.messenger.publish( - `${controllerName}:unapprovedTransactionAdded`, - addedTransactionMeta, + updateFirstTimeInteraction({ + existingTransactions: this.state.transactions, + getTransaction: (transactionId: string) => + this.#getTransaction(transactionId), + isFirstTimeInteractionEnabled: this.#isFirstTimeInteractionEnabled, + trace: this.#trace, + traceContext, + transactionMeta: addedTransactionMeta, + updateTransaction: this.#updateTransactionInternal.bind(this), + }).catch((error) => { + log('Error while updating first interaction properties', error); + }); + } else { + log( + 'Skipping simulation & first interaction update as approval not required', ); } + this.messenger.publish( + `${controllerName}:unapprovedTransactionAdded`, + addedTransactionMeta, + ); + return { result: this.#processApproval(addedTransactionMeta, { actionId, - isExisting: Boolean(existingTransactionMeta), publishHook, requireApproval, traceContext, @@ -1581,11 +1570,6 @@ export class TransactionController extends BaseController< transactionId: string; transactionType: TransactionType; }): Promise { - // If transaction is found for same action id, do not create a new transaction. - if (this.#getTransactionWithActionId(actionId)) { - return; - } - if (gasValues) { // Not good practice to reassign a parameter but temporarily avoiding a larger refactor. // eslint-disable-next-line no-param-reassign @@ -3032,14 +3016,12 @@ export class TransactionController extends BaseController< transactionMeta: TransactionMeta, { actionId, - isExisting = false, publishHook, requireApproval, shouldShowRequest = true, traceContext, }: { actionId?: string; - isExisting?: boolean; publishHook?: PublishHook; requireApproval?: boolean | undefined; shouldShowRequest?: boolean; @@ -3067,7 +3049,7 @@ export class TransactionController extends BaseController< ? Promise.resolve(meta) : this.#waitForTransactionFinished(transactionId); - if (meta && !isExisting && !isCompleted) { + if (meta && !isCompleted) { try { if (requireApproval !== false) { const acceptResult = await this.#trace( @@ -3816,18 +3798,6 @@ export class TransactionController extends BaseController< this.#onTransactionStatusChange(updatedTransactionMeta); } - /** - * Get transaction with provided actionId. - * - * @param actionId - Unique ID to prevent duplicate requests - * @returns the filtered transaction - */ - #getTransactionWithActionId(actionId?: string): TransactionMeta | undefined { - return this.state.transactions.find( - (transaction) => actionId && transaction.actionId === actionId, - ); - } - async #waitForTransactionFinished( transactionId: string, ): Promise {