From ccd95c875f6d334f59aad3ba9109acf7733367f4 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Fri, 13 Sep 2024 12:11:22 +0100 Subject: [PATCH] fix: transaction controller startup (#4658) Refactor the handling of incomplete transactions when initialising the `TransactionController`. Remove `approveTransaction` flow in `PendingTransactionTracker`. Add updated `rawTx` to cancel and speed up transactions. Skip normalization and validation when marking transaction as failed. Fix transaction resubmit if a single transaction fails. --- .../transaction-controller/jest.config.js | 8 +- .../src/TransactionController.test.ts | 1262 ++++++++--------- .../src/TransactionController.ts | 129 +- .../TransactionControllerIntegration.test.ts | 6 +- .../helpers/PendingTransactionTracker.test.ts | 44 - .../src/helpers/PendingTransactionTracker.ts | 19 +- 6 files changed, 673 insertions(+), 795 deletions(-) diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index c228664b80..c33dbefbb5 100644 --- a/packages/transaction-controller/jest.config.js +++ b/packages/transaction-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 93.44, - functions: 97.39, - lines: 98.4, - statements: 98.42, + branches: 94.37, + functions: 97.94, + lines: 98.52, + statements: 98.53, }, }, diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 05dd039e87..7c7c45f1dd 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -530,6 +530,8 @@ describe('TransactionController', () => { * @param args.selectedAccount - The selected account to use with the controller. * @param args.mockNetworkClientConfigurationsByNetworkClientId - Network * client configurations by network client ID. + * @param args.updateToInitialState - Whether to apply the controller state after instantiation via the `update` method. + * This is required if unapproved transactions are included since they are cleared during instantiation. * @returns The new TransactionController instance. */ function setupController({ @@ -538,6 +540,7 @@ describe('TransactionController', () => { messengerOptions = {}, selectedAccount = INTERNAL_ACCOUNT_MOCK, mockNetworkClientConfigurationsByNetworkClientId = {}, + updateToInitialState = false, }: { options?: Partial[0]>; network?: { @@ -558,6 +561,7 @@ describe('TransactionController', () => { NetworkClientId, NetworkClientConfiguration >; + updateToInitialState?: boolean; } = {}) { let networkState = { ...getDefaultNetworkControllerState(), @@ -646,6 +650,13 @@ describe('TransactionController', () => { messenger: restrictedMessenger, }); + const state = givenOptions?.state; + + if (updateToInitialState && state) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (controller as any).update(() => state); + } + return { controller, messenger: unrestrictedMessenger, @@ -891,18 +902,6 @@ describe('TransactionController', () => { ); }); - it('checks pending transactions', () => { - expect( - pendingTransactionTrackerMock.startIfPendingTransactions, - ).toHaveBeenCalledTimes(0); - - setupController(); - - expect( - pendingTransactionTrackerMock.startIfPendingTransactions, - ).toHaveBeenCalledTimes(1); - }); - describe('nonce tracker', () => { it('uses external pending transactions', async () => { const nonceTrackerMock = jest @@ -954,64 +953,137 @@ describe('TransactionController', () => { }); }); - describe('onBootCleanup', () => { - afterEach(() => { - updateGasMock.mockReset(); - updateGasFeesMock.mockReset(); - }); - - it('submits approved transactions for all chains', async () => { - const mockTransactionMeta = { + it('fails approved and signed transactions for all chains', async () => { + const mockTransactionMeta = { + from: ACCOUNT_MOCK, + txParams: { from: ACCOUNT_MOCK, + to: ACCOUNT_2_MOCK, + }, + }; + const mockedTransactions = [ + { + id: '123', + history: [{ ...mockTransactionMeta, id: '123' }], + chainId: toHex(5), status: TransactionStatus.approved, - txParams: { - from: ACCOUNT_MOCK, - to: ACCOUNT_2_MOCK, - }, - }; - const mockedTransactions = [ - { - id: '123', - history: [{ ...mockTransactionMeta, id: '123' }], - chainId: toHex(5), - ...mockTransactionMeta, - }, - { - id: '456', - history: [{ ...mockTransactionMeta, id: '456' }], - chainId: toHex(1), - ...mockTransactionMeta, - }, - { - id: '789', - history: [{ ...mockTransactionMeta, id: '789' }], - chainId: toHex(16), - ...mockTransactionMeta, - }, - ]; + ...mockTransactionMeta, + }, + { + id: '456', + history: [{ ...mockTransactionMeta, id: '456' }], + chainId: toHex(1), + status: TransactionStatus.approved, + ...mockTransactionMeta, + }, + { + id: '789', + history: [{ ...mockTransactionMeta, id: '789' }], + chainId: toHex(16), + status: TransactionStatus.approved, + ...mockTransactionMeta, + }, + { + id: '111', + history: [{ ...mockTransactionMeta, id: '111' }], + chainId: toHex(5), + status: TransactionStatus.signed, + ...mockTransactionMeta, + }, + { + id: '222', + history: [{ ...mockTransactionMeta, id: '222' }], + chainId: toHex(1), + status: TransactionStatus.signed, + ...mockTransactionMeta, + }, + { + id: '333', + history: [{ ...mockTransactionMeta, id: '333' }], + chainId: toHex(16), + status: TransactionStatus.signed, + ...mockTransactionMeta, + }, + ]; - const mockedControllerState = { - transactions: mockedTransactions, - methodData: {}, - lastFetchedBlockNumbers: {}, - }; + const mockedControllerState = { + transactions: mockedTransactions, + methodData: {}, + lastFetchedBlockNumbers: {}, + }; - const { controller } = setupController({ - options: { - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - state: mockedControllerState as any, - }, - }); + const { controller } = setupController({ + options: { + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + state: mockedControllerState as any, + }, + }); - await flushPromises(); + await flushPromises(); - const { transactions } = controller.state; + const { transactions } = controller.state; - expect(transactions[0].status).toBe(TransactionStatus.submitted); - expect(transactions[1].status).toBe(TransactionStatus.submitted); - expect(transactions[2].status).toBe(TransactionStatus.submitted); + expect(transactions[0].status).toBe(TransactionStatus.failed); + expect(transactions[1].status).toBe(TransactionStatus.failed); + expect(transactions[2].status).toBe(TransactionStatus.failed); + expect(transactions[3].status).toBe(TransactionStatus.failed); + expect(transactions[4].status).toBe(TransactionStatus.failed); + expect(transactions[5].status).toBe(TransactionStatus.failed); + }); + + it('removes unapproved transactions', async () => { + const mockTransactionMeta = { + from: ACCOUNT_MOCK, + txParams: { + from: ACCOUNT_MOCK, + to: ACCOUNT_2_MOCK, + }, + }; + + const mockedTransactions = [ + { + id: '123', + history: [{ ...mockTransactionMeta, id: '123' }], + chainId: toHex(5), + status: TransactionStatus.unapproved, + ...mockTransactionMeta, + }, + { + id: '456', + history: [{ ...mockTransactionMeta, id: '456' }], + chainId: toHex(1), + status: TransactionStatus.unapproved, + ...mockTransactionMeta, + }, + { + id: '789', + history: [{ ...mockTransactionMeta, id: '789' }], + chainId: toHex(16), + status: TransactionStatus.unapproved, + ...mockTransactionMeta, + }, + ]; + + const mockedControllerState = { + transactions: mockedTransactions, + methodData: {}, + lastFetchedBlockNumbers: {}, + }; + + const { controller } = setupController({ + options: { + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + state: mockedControllerState as any, + }, }); + + await flushPromises(); + + const { transactions } = controller.state; + + expect(transactions).toHaveLength(0); }); }); @@ -3184,6 +3256,7 @@ describe('TransactionController', () => { ], }, }, + updateToInitialState: true, }); messenger.subscribe( 'TransactionController:transactionDropped', @@ -3806,6 +3879,7 @@ describe('TransactionController', () => { ], }, }, + updateToInitialState: true, }); const gas = '0xgas'; @@ -3871,6 +3945,7 @@ describe('TransactionController', () => { ], }, }, + updateToInitialState: true, }); controller.updateTransactionGasFees(transactionId, { @@ -3915,6 +3990,7 @@ describe('TransactionController', () => { ], }, }, + updateToInitialState: true, }); expect(() => controller.updatePreviousGasParams(transactionId, { @@ -3945,6 +4021,7 @@ describe('TransactionController', () => { ], }, }, + updateToInitialState: true, }); const gasLimit = '0xgasLimit'; @@ -4089,6 +4166,7 @@ describe('TransactionController', () => { ] as TransactionMeta[], }, }, + updateToInitialState: true, }); firePendingTransactionTrackerEvent('transaction-confirmed', confirmed); @@ -4439,7 +4517,6 @@ describe('TransactionController', () => { options: { hooks: { afterSign: () => false, - beforeApproveOnInit: () => false, beforePublish: () => false, getAdditionalSignArguments: () => [metadataMock], }, @@ -4481,7 +4558,6 @@ describe('TransactionController', () => { options: { hooks: { afterSign: () => false, - beforeApproveOnInit: () => false, beforePublish: () => false, getAdditionalSignArguments: () => [metadataMock], }, @@ -4699,6 +4775,7 @@ describe('TransactionController', () => { ], }, }, + updateToInitialState: true, }); expect(controller.state.transactions[0]).toBeDefined(); @@ -4749,687 +4826,539 @@ describe('TransactionController', () => { 'Cannot update security alert response as no transaction metadata found', ); }); + }); - describe('updateCustodialTransaction', () => { - let transactionId: string; - let statusMock: TransactionStatus; - let baseTransaction: TransactionMeta; - let transactionMeta: TransactionMeta; - - beforeEach(() => { - transactionId = '1'; - statusMock = TransactionStatus.unapproved as const; - baseTransaction = { - id: transactionId, - chainId: toHex(5), - status: statusMock, - time: 123456789, - txParams: { - from: ACCOUNT_MOCK, - to: ACCOUNT_2_MOCK, - }, - }; - transactionMeta = { - ...baseTransaction, - custodyId: '123', - history: [{ ...baseTransaction }], - }; - }); - - it.each([ - { - newStatus: TransactionStatus.signed, - }, - { - newStatus: TransactionStatus.submitted, - }, - { - newStatus: TransactionStatus.failed, - errorMessage: 'Error mock', - }, - ])( - 'updates transaction status to $newStatus', - async ({ newStatus, errorMessage }) => { - const { controller } = setupController({ - options: { - state: { - transactions: [transactionMeta], - }, - }, - }); - - controller.updateCustodialTransaction(transactionId, { - status: newStatus, - errorMessage, - }); - - const updatedTransaction = controller.state.transactions[0]; - - expect(updatedTransaction?.status).toStrictEqual(newStatus); - }, - ); - - it.each([ - { - newStatus: TransactionStatus.submitted, - }, - { - newStatus: TransactionStatus.failed, - errorMessage: 'Error mock', - }, - ])( - 'publishes TransactionController:transactionFinished when update transaction status to $newStatus', - async ({ newStatus, errorMessage }) => { - const finishedEventListener = jest.fn(); - const { controller, messenger } = setupController({ - options: { - state: { - transactions: [transactionMeta], - }, - }, - }); - messenger.subscribe( - 'TransactionController:transactionFinished', - finishedEventListener, - ); - - controller.updateCustodialTransaction(transactionId, { - status: newStatus, - errorMessage, - }); - - const updatedTransaction = controller.state.transactions[0]; - - expect(finishedEventListener).toHaveBeenCalledTimes(1); - expect(finishedEventListener).toHaveBeenCalledWith( - expect.objectContaining({ - ...transactionMeta, - status: newStatus, - }), - ); - expect(updatedTransaction.status).toStrictEqual(newStatus); + describe('updateCustodialTransaction', () => { + let transactionId: string; + let statusMock: TransactionStatus; + let baseTransaction: TransactionMeta; + let transactionMeta: TransactionMeta; + + beforeEach(() => { + transactionId = '1'; + statusMock = TransactionStatus.unapproved as const; + baseTransaction = { + id: transactionId, + chainId: toHex(5), + status: statusMock, + time: 123456789, + txParams: { + from: ACCOUNT_MOCK, + to: ACCOUNT_2_MOCK, }, - ); + }; + transactionMeta = { + ...baseTransaction, + custodyId: '123', + history: [{ ...baseTransaction }], + }; + }); - it('updates transaction hash', async () => { - const newHash = '1234'; + it.each([ + { + newStatus: TransactionStatus.signed, + }, + { + newStatus: TransactionStatus.submitted, + }, + { + newStatus: TransactionStatus.failed, + errorMessage: 'Error mock', + }, + ])( + 'updates transaction status to $newStatus', + async ({ newStatus, errorMessage }) => { const { controller } = setupController({ options: { state: { transactions: [transactionMeta], }, }, + updateToInitialState: true, }); controller.updateCustodialTransaction(transactionId, { - hash: newHash, + status: newStatus, + errorMessage, }); const updatedTransaction = controller.state.transactions[0]; - expect(updatedTransaction.hash).toStrictEqual(newHash); - }); - - it('throws if custodial transaction does not exists', async () => { - const nonExistentId = 'nonExistentId'; - const newStatus = TransactionStatus.approved as const; - const { controller } = setupController(); - - expect(() => - controller.updateCustodialTransaction(nonExistentId, { - status: newStatus, - }), - ).toThrow( - 'Cannot update custodial transaction as no transaction metadata found', - ); - }); + expect(updatedTransaction?.status).toStrictEqual(newStatus); + }, + ); - it('throws if transaction is not a custodial transaction', async () => { - const nonCustodialTransaction: TransactionMeta = { - ...baseTransaction, - history: [{ ...baseTransaction }], - }; - const newStatus = TransactionStatus.approved as const; - const { controller } = setupController({ + it.each([ + { + newStatus: TransactionStatus.submitted, + }, + { + newStatus: TransactionStatus.failed, + errorMessage: 'Error mock', + }, + ])( + 'publishes TransactionController:transactionFinished when update transaction status to $newStatus', + async ({ newStatus, errorMessage }) => { + const finishedEventListener = jest.fn(); + const { controller, messenger } = setupController({ options: { state: { - transactions: [nonCustodialTransaction], + transactions: [transactionMeta], }, }, + updateToInitialState: true, }); + messenger.subscribe( + 'TransactionController:transactionFinished', + finishedEventListener, + ); - expect(() => - controller.updateCustodialTransaction(nonCustodialTransaction.id, { - status: newStatus, - }), - ).toThrow('Transaction must be a custodian transaction'); - }); - - it('throws if status is invalid', async () => { - const newStatus = TransactionStatus.approved as const; - const { controller } = setupController({ - options: { - state: { - transactions: [transactionMeta], - }, - }, + controller.updateCustodialTransaction(transactionId, { + status: newStatus, + errorMessage, }); - expect(() => - controller.updateCustodialTransaction(transactionMeta.id, { + const updatedTransaction = controller.state.transactions[0]; + + expect(finishedEventListener).toHaveBeenCalledTimes(1); + expect(finishedEventListener).toHaveBeenCalledWith( + expect.objectContaining({ + ...transactionMeta, status: newStatus, }), - ).toThrow( - `Cannot update custodial transaction with status: ${newStatus}`, ); - }); + expect(updatedTransaction.status).toStrictEqual(newStatus); + }, + ); - it('no property was updated', async () => { - const { controller } = setupController({ - options: { - state: { - transactions: [transactionMeta], - }, + it('updates transaction hash', async () => { + const newHash = '1234'; + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMeta], }, - }); + }, + updateToInitialState: true, + }); - controller.updateCustodialTransaction(transactionId, {}); + controller.updateCustodialTransaction(transactionId, { + hash: newHash, + }); - const updatedTransaction = controller.state.transactions[0]; + const updatedTransaction = controller.state.transactions[0]; - expect(updatedTransaction.status).toStrictEqual(transactionMeta.status); - expect(updatedTransaction.hash).toStrictEqual(transactionMeta.hash); - }); + expect(updatedTransaction.hash).toStrictEqual(newHash); }); - describe('initApprovals', () => { - it('creates approvals for all unapproved transaction', async () => { - const mockTransactionMeta = { - from: ACCOUNT_MOCK, - chainId: toHex(5), - status: TransactionStatus.unapproved, - txParams: { - from: ACCOUNT_MOCK, - to: ACCOUNT_2_MOCK, - }, - }; - - const mockedTransactions = [ - { - id: '123', - ...mockTransactionMeta, - history: [{ ...mockTransactionMeta, id: '123' }], - }, - { - id: '1234', - ...mockTransactionMeta, - history: [{ ...mockTransactionMeta, id: '1234' }], - }, - { - id: '12345', - ...mockTransactionMeta, - history: [{ ...mockTransactionMeta, id: '12345' }], - isUserOperation: true, - }, - ]; + it('throws if custodial transaction does not exists', async () => { + const nonExistentId = 'nonExistentId'; + const newStatus = TransactionStatus.approved as const; + const { controller } = setupController(); - const mockedControllerState = { - transactions: mockedTransactions, - methodData: {}, - lastFetchedBlockNumbers: {}, - }; + expect(() => + controller.updateCustodialTransaction(nonExistentId, { + status: newStatus, + }), + ).toThrow( + 'Cannot update custodial transaction as no transaction metadata found', + ); + }); - const { controller, messenger } = setupController({ - options: { - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - state: mockedControllerState as any, + it('throws if transaction is not a custodial transaction', async () => { + const nonCustodialTransaction: TransactionMeta = { + ...baseTransaction, + history: [{ ...baseTransaction }], + }; + const newStatus = TransactionStatus.approved as const; + const { controller } = setupController({ + options: { + state: { + transactions: [nonCustodialTransaction], }, - }); - const messengerCallSpy = jest.spyOn(messenger, 'call'); + }, + updateToInitialState: true, + }); - controller.initApprovals(); - await flushPromises(); + expect(() => + controller.updateCustodialTransaction(nonCustodialTransaction.id, { + status: newStatus, + }), + ).toThrow('Transaction must be a custodian transaction'); + }); - expect( - messengerCallSpy.mock.calls.filter( - (args) => args[0] === 'ApprovalController:addRequest', - ), - ).toHaveLength(2); - expect(messengerCallSpy).toHaveBeenCalledWith( - 'ApprovalController:addRequest', - { - expectsResult: true, - id: '123', - origin: 'metamask', - requestData: { txId: '123' }, - type: 'transaction', - }, - false, - ); - expect(messengerCallSpy).toHaveBeenCalledWith( - 'ApprovalController:addRequest', - { - expectsResult: true, - id: '1234', - origin: 'metamask', - requestData: { txId: '1234' }, - type: 'transaction', + it('throws if status is invalid', async () => { + const newStatus = TransactionStatus.approved as const; + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMeta], }, - false, - ); + }, + updateToInitialState: true, }); - it('catches error without code property in error object while creating approval', async () => { - const mockTransactionMeta = { - from: ACCOUNT_MOCK, - chainId: toHex(5), - status: TransactionStatus.unapproved, - txParams: { - from: ACCOUNT_MOCK, - to: ACCOUNT_2_MOCK, - }, - }; + expect(() => + controller.updateCustodialTransaction(transactionMeta.id, { + status: newStatus, + }), + ).toThrow( + `Cannot update custodial transaction with status: ${newStatus}`, + ); + }); - const mockedTransactions = [ - { - id: '123', - ...mockTransactionMeta, - history: [{ ...mockTransactionMeta, id: '123' }], - }, - { - id: '1234', - ...mockTransactionMeta, - history: [{ ...mockTransactionMeta, id: '1234' }], + it('no property was updated', async () => { + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMeta], }, - ]; - - const mockedControllerState = { - transactions: mockedTransactions, - methodData: {}, - lastFetchedBlockNumbers: {}, - }; - - const mockedErrorMessage = 'mocked error'; - - const { controller, messenger, mockTransactionApprovalRequest } = - setupController({ - options: { - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - state: mockedControllerState as any, - }, - }); - const messengerCallSpy = jest.spyOn(messenger, 'call'); - // Expect both calls to throw error, one with code property to check if it is handled - mockTransactionApprovalRequest.actionHandlerMock - .mockImplementationOnce(() => { - // eslint-disable-next-line @typescript-eslint/no-throw-literal - throw { message: mockedErrorMessage }; - }) - .mockImplementationOnce(() => { - // eslint-disable-next-line @typescript-eslint/no-throw-literal - throw { - message: mockedErrorMessage, - code: errorCodes.provider.userRejectedRequest, - }; - }); - const consoleSpy = jest.spyOn(console, 'error').mockImplementation(); + }, + updateToInitialState: true, + }); - controller.initApprovals(); + controller.updateCustodialTransaction(transactionId, {}); - await flushPromises(); + const updatedTransaction = controller.state.transactions[0]; - expect(consoleSpy).toHaveBeenCalledTimes(1); - expect(consoleSpy).toHaveBeenCalledWith( - 'Error during persisted transaction approval', - new Error(mockedErrorMessage), - ); - expect( - messengerCallSpy.mock.calls.filter((args) => { - return args[0] === 'ApprovalController:addRequest'; - }), - ).toHaveLength(2); - }); - - it('does not create any approval when there is no unapproved transaction', async () => { - const { controller, messenger } = setupController(); - jest.spyOn(messenger, 'call'); - controller.initApprovals(); - await flushPromises(); - expect(messenger.call).not.toHaveBeenCalledWith( - 'ApprovalController:addRequest', - ); - }); + expect(updatedTransaction.status).toStrictEqual(transactionMeta.status); + expect(updatedTransaction.hash).toStrictEqual(transactionMeta.hash); }); + }); - describe('getTransactions', () => { - it('returns transactions matching values in search criteria', () => { - const transactions: TransactionMeta[] = [ - { - chainId: '0x1', - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId2', - status: TransactionStatus.unapproved, - time: 2, - txParams: { from: '0x2' }, - }, - { - chainId: '0x1', - id: 'testId3', - status: TransactionStatus.submitted, - time: 1, - txParams: { from: '0x3' }, - }, - ]; - - const { controller } = setupController({ - options: { - state: { transactions }, - }, - }); + describe('getTransactions', () => { + it('returns transactions matching values in search criteria', () => { + const transactions: TransactionMeta[] = [ + { + chainId: '0x1', + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId2', + status: TransactionStatus.unapproved, + time: 2, + txParams: { from: '0x2' }, + }, + { + chainId: '0x1', + id: 'testId3', + status: TransactionStatus.submitted, + time: 1, + txParams: { from: '0x3' }, + }, + ]; - expect( - controller.getTransactions({ - searchCriteria: { time: 1 }, - filterToCurrentNetwork: false, - }), - ).toStrictEqual([transactions[0], transactions[2]]); + const { controller } = setupController({ + options: { + state: { transactions }, + }, + updateToInitialState: true, }); - it('returns transactions matching param values in search criteria', () => { - const transactions: TransactionMeta[] = [ - { - chainId: '0x1', - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId2', - status: TransactionStatus.unapproved, - time: 2, - txParams: { from: '0x2' }, - }, - { - chainId: '0x1', - id: 'testId3', - status: TransactionStatus.submitted, - time: 3, - txParams: { from: '0x1' }, - }, - ]; + expect( + controller.getTransactions({ + searchCriteria: { time: 1 }, + filterToCurrentNetwork: false, + }), + ).toStrictEqual([transactions[0], transactions[2]]); + }); - const { controller } = setupController({ - options: { - state: { transactions }, - }, - }); + it('returns transactions matching param values in search criteria', () => { + const transactions: TransactionMeta[] = [ + { + chainId: '0x1', + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId2', + status: TransactionStatus.unapproved, + time: 2, + txParams: { from: '0x2' }, + }, + { + chainId: '0x1', + id: 'testId3', + status: TransactionStatus.submitted, + time: 3, + txParams: { from: '0x1' }, + }, + ]; - expect( - controller.getTransactions({ - searchCriteria: { from: '0x1' }, - filterToCurrentNetwork: false, - }), - ).toStrictEqual([transactions[0], transactions[2]]); + const { controller } = setupController({ + options: { + state: { transactions }, + }, + updateToInitialState: true, }); - it('returns transactions matching multiple values in search criteria', () => { - const transactions: TransactionMeta[] = [ - { - chainId: '0x1', - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId2', - status: TransactionStatus.unapproved, - time: 2, - txParams: { from: '0x2' }, - }, - { - chainId: '0x1', - id: 'testId3', - status: TransactionStatus.submitted, - time: 1, - txParams: { from: '0x1' }, - }, - ]; + expect( + controller.getTransactions({ + searchCriteria: { from: '0x1' }, + filterToCurrentNetwork: false, + }), + ).toStrictEqual([transactions[0], transactions[2]]); + }); - const { controller } = setupController({ - options: { - state: { transactions }, - }, - }); + it('returns transactions matching multiple values in search criteria', () => { + const transactions: TransactionMeta[] = [ + { + chainId: '0x1', + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId2', + status: TransactionStatus.unapproved, + time: 2, + txParams: { from: '0x2' }, + }, + { + chainId: '0x1', + id: 'testId3', + status: TransactionStatus.submitted, + time: 1, + txParams: { from: '0x1' }, + }, + ]; - expect( - controller.getTransactions({ - searchCriteria: { from: '0x1', time: 1 }, - filterToCurrentNetwork: false, - }), - ).toStrictEqual([transactions[0], transactions[2]]); + const { controller } = setupController({ + options: { + state: { transactions }, + }, + updateToInitialState: true, }); - it('returns transactions matching function in search criteria', () => { - const transactions: TransactionMeta[] = [ - { - chainId: '0x1', - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId2', - status: TransactionStatus.unapproved, - time: 2, - txParams: { from: '0x2' }, - }, - { - chainId: '0x1', - id: 'testId3', - status: TransactionStatus.submitted, - time: 1, - txParams: { from: '0x3' }, - }, - ]; + expect( + controller.getTransactions({ + searchCriteria: { from: '0x1', time: 1 }, + filterToCurrentNetwork: false, + }), + ).toStrictEqual([transactions[0], transactions[2]]); + }); - const { controller } = setupController({ - options: { - state: { transactions }, - }, - }); + it('returns transactions matching function in search criteria', () => { + const transactions: TransactionMeta[] = [ + { + chainId: '0x1', + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId2', + status: TransactionStatus.unapproved, + time: 2, + txParams: { from: '0x2' }, + }, + { + chainId: '0x1', + id: 'testId3', + status: TransactionStatus.submitted, + time: 1, + txParams: { from: '0x3' }, + }, + ]; - expect( - controller.getTransactions({ - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - searchCriteria: { time: (v: any) => v === 1 }, - filterToCurrentNetwork: false, - }), - ).toStrictEqual([transactions[0], transactions[2]]); + const { controller } = setupController({ + options: { + state: { transactions }, + }, + updateToInitialState: true, }); - it('returns transactions matching current network', () => { - const transactions: TransactionMeta[] = [ - { - chainId: MOCK_NETWORK.chainId, - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1' }, - }, - { - chainId: '0x2', - id: 'testId2', - status: TransactionStatus.unapproved, - time: 2, - txParams: { from: '0x2' }, - }, - { - chainId: MOCK_NETWORK.chainId, - id: 'testId3', - status: TransactionStatus.submitted, - time: 1, - txParams: { from: '0x3' }, - }, - ]; + expect( + controller.getTransactions({ + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + searchCriteria: { time: (v: any) => v === 1 }, + filterToCurrentNetwork: false, + }), + ).toStrictEqual([transactions[0], transactions[2]]); + }); - const { controller } = setupController({ - options: { - state: { transactions }, - }, - }); + it('returns transactions matching current network', () => { + const transactions: TransactionMeta[] = [ + { + chainId: MOCK_NETWORK.chainId, + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1' }, + }, + { + chainId: '0x2', + id: 'testId2', + status: TransactionStatus.unapproved, + time: 2, + txParams: { from: '0x2' }, + }, + { + chainId: MOCK_NETWORK.chainId, + id: 'testId3', + status: TransactionStatus.submitted, + time: 1, + txParams: { from: '0x3' }, + }, + ]; - expect( - controller.getTransactions({ - filterToCurrentNetwork: true, - }), - ).toStrictEqual([transactions[0], transactions[2]]); + const { controller } = setupController({ + options: { + state: { transactions }, + }, + updateToInitialState: true, }); - it('returns transactions from specified list', () => { - const { controller } = setupController(); + expect( + controller.getTransactions({ + filterToCurrentNetwork: true, + }), + ).toStrictEqual([transactions[0], transactions[2]]); + }); - const transactions: TransactionMeta[] = [ - { - chainId: '0x1', - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId2', - status: TransactionStatus.unapproved, - time: 2, - txParams: { from: '0x2' }, - }, - { - chainId: '0x1', - id: 'testId3', - status: TransactionStatus.submitted, - time: 1, - txParams: { from: '0x3' }, - }, - ]; + it('returns transactions from specified list', () => { + const { controller } = setupController(); - expect( - controller.getTransactions({ - searchCriteria: { time: 1 }, - initialList: transactions, - filterToCurrentNetwork: false, - }), - ).toStrictEqual([transactions[0], transactions[2]]); - }); + const transactions: TransactionMeta[] = [ + { + chainId: '0x1', + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId2', + status: TransactionStatus.unapproved, + time: 2, + txParams: { from: '0x2' }, + }, + { + chainId: '0x1', + id: 'testId3', + status: TransactionStatus.submitted, + time: 1, + txParams: { from: '0x3' }, + }, + ]; - it('returns limited number of transactions sorted by ascending time', () => { - const transactions: TransactionMeta[] = [ - { - chainId: '0x1', - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1', nonce: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId2', - status: TransactionStatus.confirmed, - time: 2, - txParams: { from: '0x1', nonce: '0x2' }, - }, - { - chainId: '0x1', - id: 'testId3', - status: TransactionStatus.unapproved, - time: 3, - txParams: { from: '0x2', nonce: '0x3' }, - }, - { - chainId: '0x1', - id: 'testId4', - status: TransactionStatus.submitted, - time: 4, - txParams: { from: '0x1', nonce: '0x4' }, - }, - ]; + expect( + controller.getTransactions({ + searchCriteria: { time: 1 }, + initialList: transactions, + filterToCurrentNetwork: false, + }), + ).toStrictEqual([transactions[0], transactions[2]]); + }); - const { controller } = setupController({ - options: { - state: { transactions }, - }, - }); + it('returns limited number of transactions sorted by ascending time', () => { + const transactions: TransactionMeta[] = [ + { + chainId: '0x1', + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1', nonce: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId2', + status: TransactionStatus.confirmed, + time: 2, + txParams: { from: '0x1', nonce: '0x2' }, + }, + { + chainId: '0x1', + id: 'testId3', + status: TransactionStatus.unapproved, + time: 3, + txParams: { from: '0x2', nonce: '0x3' }, + }, + { + chainId: '0x1', + id: 'testId4', + status: TransactionStatus.submitted, + time: 4, + txParams: { from: '0x1', nonce: '0x4' }, + }, + ]; - expect( - controller.getTransactions({ - searchCriteria: { from: '0x1' }, - filterToCurrentNetwork: false, - limit: 2, - }), - ).toStrictEqual([transactions[1], transactions[3]]); + const { controller } = setupController({ + options: { + state: { transactions }, + }, + updateToInitialState: true, }); - it('returns limited number of transactions except for duplicate nonces', () => { - const transactions: TransactionMeta[] = [ - { - chainId: '0x1', - id: 'testId1', - status: TransactionStatus.confirmed, - time: 1, - txParams: { from: '0x1', nonce: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId2', + expect( + controller.getTransactions({ + searchCriteria: { from: '0x1' }, + filterToCurrentNetwork: false, + limit: 2, + }), + ).toStrictEqual([transactions[1], transactions[3]]); + }); - status: TransactionStatus.unapproved, - time: 2, - txParams: { from: '0x2', nonce: '0x2' }, - }, - { - chainId: '0x1', - id: 'testId3', - status: TransactionStatus.submitted, - time: 3, - txParams: { from: '0x1', nonce: '0x1' }, - }, - { - chainId: '0x1', - id: 'testId4', - status: TransactionStatus.submitted, - time: 4, - txParams: { from: '0x1', nonce: '0x3' }, - }, - ]; + it('returns limited number of transactions except for duplicate nonces', () => { + const transactions: TransactionMeta[] = [ + { + chainId: '0x1', + id: 'testId1', + status: TransactionStatus.confirmed, + time: 1, + txParams: { from: '0x1', nonce: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId2', - const { controller } = setupController({ - options: { - state: { transactions }, - }, - }); + status: TransactionStatus.unapproved, + time: 2, + txParams: { from: '0x2', nonce: '0x2' }, + }, + { + chainId: '0x1', + id: 'testId3', + status: TransactionStatus.submitted, + time: 3, + txParams: { from: '0x1', nonce: '0x1' }, + }, + { + chainId: '0x1', + id: 'testId4', + status: TransactionStatus.submitted, + time: 4, + txParams: { from: '0x1', nonce: '0x3' }, + }, + ]; - expect( - controller.getTransactions({ - searchCriteria: { from: '0x1' }, - filterToCurrentNetwork: false, - limit: 2, - }), - ).toStrictEqual([transactions[0], transactions[2], transactions[3]]); + const { controller } = setupController({ + options: { + state: { transactions }, + }, + updateToInitialState: true, }); + + expect( + controller.getTransactions({ + searchCriteria: { from: '0x1' }, + filterToCurrentNetwork: false, + limit: 2, + }), + ).toStrictEqual([transactions[0], transactions[2], transactions[3]]); }); }); @@ -5470,6 +5399,7 @@ describe('TransactionController', () => { transactions: [transactionMeta], }, }, + updateToInitialState: true, }); const updatedTransaction = await controller.updateEditableParams( @@ -5487,6 +5417,7 @@ describe('TransactionController', () => { transactions: [transactionMeta], }, }, + updateToInitialState: true, }); const updatedTransaction = await controller.updateEditableParams( @@ -5546,6 +5477,7 @@ describe('TransactionController', () => { ], }, }, + updateToInitialState: true, }); expect(getSimulationDataMock).toHaveBeenCalledTimes(0); diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 3b120e0734..abd07f91f4 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -305,7 +305,6 @@ export type TransactionControllerOptions = { transactionMeta: TransactionMeta, signedTx: TypedTransaction, ) => boolean; - beforeApproveOnInit?: (transactionMeta: TransactionMeta) => boolean; beforeCheckPendingTransaction?: ( transactionMeta: TransactionMeta, ) => boolean; @@ -633,10 +632,6 @@ export class TransactionController extends BaseController< signedTx: TypedTransaction, ) => boolean; - private readonly beforeApproveOnInit: ( - transactionMeta: TransactionMeta, - ) => boolean; - private readonly beforeCheckPendingTransaction: ( transactionMeta: TransactionMeta, ) => boolean; @@ -657,24 +652,48 @@ export class TransactionController extends BaseController< error: Error, actionId?: string, ) { - const newTransactionMeta = merge({}, transactionMeta, { - error: normalizeTxError(error), - status: TransactionStatus.failed as const, - }); + let newTransactionMeta: TransactionMeta; + + try { + newTransactionMeta = this.#updateTransactionInternal( + { + transactionId: transactionMeta.id, + note: 'TransactionController#failTransaction - Add error message and set status to failed', + skipValidation: true, + }, + (draftTransactionMeta) => { + draftTransactionMeta.status = TransactionStatus.failed; + + ( + draftTransactionMeta as TransactionMeta & { + status: TransactionStatus.failed; + } + ).error = normalizeTxError(error); + }, + ); + } catch (err: unknown) { + log('Failed to mark transaction as failed', err); + + newTransactionMeta = { + ...transactionMeta, + status: TransactionStatus.failed, + error: normalizeTxError(error), + }; + } + this.messagingSystem.publish(`${controllerName}:transactionFailed`, { actionId, error: error.message, transactionMeta: newTransactionMeta, }); - this.updateTransaction( - newTransactionMeta, - 'TransactionController#failTransaction - Add error message and set status to failed', - ); + this.onTransactionStatusChange(newTransactionMeta); + this.messagingSystem.publish( `${controllerName}:transactionFinished`, newTransactionMeta, ); + this.#internalEvents.emit( `${transactionMeta.id}:finished`, newTransactionMeta, @@ -800,7 +819,6 @@ export class TransactionController extends BaseController< this.#trace = trace ?? (((_request, fn) => fn?.()) as TraceCallback); this.afterSign = hooks?.afterSign ?? (() => true); - this.beforeApproveOnInit = hooks?.beforeApproveOnInit ?? (() => true); this.beforeCheckPendingTransaction = hooks?.beforeCheckPendingTransaction ?? /* istanbul ignore next */ @@ -1346,15 +1364,16 @@ export class TransactionController extends BaseController< const newTransactionMeta = { ...transactionMetaWithRsv, + actionId, estimatedBaseFee, - id: random(), - time: Date.now(), hash, - actionId, + id: random(), originalGasEstimate: transactionMeta.txParams.gas, - type: transactionType, - txParams: newTxParams, originalType: transactionMeta.type, + rawTx, + time: Date.now(), + txParams: newTxParams, + type: transactionType, }; this.addMetadata(newTransactionMeta); @@ -2010,30 +2029,6 @@ export class TransactionController extends BaseController< } } - /** - * Creates approvals for all unapproved transactions persisted. - */ - initApprovals() { - const chainId = this.getChainId(); - const unapprovedTxs = this.state.transactions.filter( - (transaction) => - transaction.status === TransactionStatus.unapproved && - transaction.chainId === chainId && - !transaction.isUserOperation, - ); - - for (const txMeta of unapprovedTxs) { - this.processApproval(txMeta, { - shouldShowRequest: false, - }).catch((error) => { - if (error?.code === errorCodes.provider.userRejectedRequest) { - return; - } - console.error('Error during persisted transaction approval', error); - }); - } - } - /** * Search transaction metadata for matching entries. * @@ -2349,24 +2344,23 @@ export class TransactionController extends BaseController< } private onBootCleanup() { - this.submitApprovedTransactions(); + this.clearUnapprovedTransactions(); + this.failIncompleteTransactions(); } - /** - * Force submit approved transactions for all chains. - */ - private submitApprovedTransactions() { - const approvedTransactions = this.state.transactions.filter( - (transaction) => transaction.status === TransactionStatus.approved, + private failIncompleteTransactions() { + const incompleteTransactions = this.state.transactions.filter( + (transaction) => + [TransactionStatus.approved, TransactionStatus.signed].includes( + transaction.status, + ), ); - for (const transactionMeta of approvedTransactions) { - if (this.beforeApproveOnInit(transactionMeta)) { - this.approveTransaction(transactionMeta.id).catch((error) => { - /* istanbul ignore next */ - console.error('Error while submitting persisted transaction', error); - }); - } + for (const transactionMeta of incompleteTransactions) { + this.failTransaction( + transactionMeta, + new Error('Transaction incomplete at startup'), + ); } } @@ -3359,9 +3353,6 @@ export class TransactionController extends BaseController< const getChainId = chainId ? () => chainId : this.getChainId.bind(this); const pendingTransactionTracker = new PendingTransactionTracker({ - approveTransaction: async (transactionId: string) => { - await this.approveTransaction(transactionId); - }, blockTracker, getChainId, getEthQuery: () => ethQuery, @@ -3527,7 +3518,13 @@ export class TransactionController extends BaseController< transactionId, note, skipHistory, - }: { transactionId: string; note?: string; skipHistory?: boolean }, + skipValidation, + }: { + transactionId: string; + note?: string; + skipHistory?: boolean; + skipValidation?: boolean; + }, callback: (transactionMeta: TransactionMeta) => TransactionMeta | void, ): Readonly { let updatedTransactionParams: (keyof TransactionParams)[] = []; @@ -3542,11 +3539,13 @@ export class TransactionController extends BaseController< // eslint-disable-next-line n/callback-return transactionMeta = callback(transactionMeta) ?? transactionMeta; - transactionMeta.txParams = normalizeTransactionParams( - transactionMeta.txParams, - ); + if (skipValidation !== true) { + transactionMeta.txParams = normalizeTransactionParams( + transactionMeta.txParams, + ); - validateTxParams(transactionMeta.txParams); + validateTxParams(transactionMeta.txParams); + } updatedTransactionParams = this.#checkIfTransactionParamsUpdated(transactionMeta); diff --git a/packages/transaction-controller/src/TransactionControllerIntegration.test.ts b/packages/transaction-controller/src/TransactionControllerIntegration.test.ts index a6234dd909..519f16ed3c 100644 --- a/packages/transaction-controller/src/TransactionControllerIntegration.test.ts +++ b/packages/transaction-controller/src/TransactionControllerIntegration.test.ts @@ -286,7 +286,7 @@ describe('TransactionController Integration', () => { }); // eslint-disable-next-line jest/no-disabled-tests - it('should submit all approved transactions in state', async () => { + it('should fail all approved transactions in state', async () => { mockNetwork({ networkClientConfiguration: buildInfuraNetworkClientConfiguration( InfuraNetworkType.goerli, @@ -396,10 +396,10 @@ describe('TransactionController Integration', () => { expect(transactionController.state.transactions).toMatchObject([ expect.objectContaining({ - status: 'submitted', + status: 'failed', }), expect.objectContaining({ - status: 'submitted', + status: 'failed', }), ]); transactionController.destroy(); diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts index 920e90d75c..46b6e26151 100644 --- a/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts +++ b/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts @@ -1008,50 +1008,6 @@ describe('PendingTransactionTracker', () => { expect(options.publishTransaction).toHaveBeenCalledTimes(0); }); }); - - describe('approves transaction', () => { - it('if no raw transaction', async () => { - const getTransactions = jest.fn().mockReturnValue( - freeze( - [ - { - ...TRANSACTION_SUBMITTED_MOCK, - rawTx: undefined, - }, - ], - true, - ), - ); - - pendingTransactionTracker = new PendingTransactionTracker({ - ...options, - getTransactions, - }); - - queryMock.mockResolvedValueOnce(undefined); - queryMock.mockResolvedValueOnce('0x1'); - - await onLatestBlock(BLOCK_NUMBER_MOCK); - getTransactions.mockReturnValue( - freeze( - [ - { - ...TRANSACTION_SUBMITTED_MOCK, - rawTx: undefined, - firstRetryBlockNumber: BLOCK_NUMBER_MOCK, - }, - ], - true, - ), - ); - await onLatestBlock('0x124'); - - expect(options.approveTransaction).toHaveBeenCalledTimes(1); - expect(options.approveTransaction).toHaveBeenCalledWith( - TRANSACTION_SUBMITTED_MOCK.id, - ); - }); - }); }); }); diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts index 7f29ec45fb..a6930afc4c 100644 --- a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts +++ b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts @@ -63,8 +63,6 @@ export interface PendingTransactionTrackerEventEmitter extends EventEmitter { export class PendingTransactionTracker { hub: PendingTransactionTrackerEventEmitter; - #approveTransaction: (transactionId: string) => Promise; - #blockTracker: BlockTracker; #droppedBlockCountByHash: Map; @@ -92,7 +90,6 @@ export class PendingTransactionTracker { #beforePublish: (transactionMeta: TransactionMeta) => boolean; constructor({ - approveTransaction, blockTracker, getChainId, getEthQuery, @@ -102,7 +99,6 @@ export class PendingTransactionTracker { publishTransaction, hooks, }: { - approveTransaction: (transactionId: string) => Promise; blockTracker: BlockTracker; getChainId: () => string; getEthQuery: (networkClientId?: NetworkClientId) => EthQuery; @@ -119,7 +115,6 @@ export class PendingTransactionTracker { }) { this.hub = new EventEmitter() as PendingTransactionTrackerEventEmitter; - this.#approveTransaction = approveTransaction; this.#blockTracker = blockTracker; this.#droppedBlockCountByHash = new Map(); this.#getChainId = getChainId; @@ -252,11 +247,13 @@ export class PendingTransactionTracker { } catch (error: any) { /* istanbul ignore next */ const errorMessage = - error.value?.message?.toLowerCase() || error.message.toLowerCase(); + error.value?.message?.toLowerCase() || + error.message?.toLowerCase() || + String(error); if (this.#isKnownTransactionError(errorMessage)) { log('Ignoring known transaction error', errorMessage); - return; + continue; } this.#warnTransaction( @@ -288,14 +285,8 @@ export class PendingTransactionTracker { return; } - if (!rawTx?.length) { - log('Approving transaction as no raw value'); - await this.#approveTransaction(txMeta.id); - return; - } - const ethQuery = this.#getEthQuery(txMeta.networkClientId); - await this.#publishTransaction(ethQuery, rawTx); + await this.#publishTransaction(ethQuery, rawTx as string); const retryCount = (txMeta.retryCount ?? 0) + 1;