diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 6bcb903e0c2..f3e0bfb404d 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add sequential batch support when `publishBatchHook` is not defined ([#5762](https://github.com/MetaMask/core/pull/5762)) + ## [54.4.0] ### Changed diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index ae0b4bf18d2..900adf08ae1 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -1008,6 +1008,10 @@ export class TransactionController extends BaseController< publicKeyEIP7702: this.#publicKeyEIP7702, request, updateTransaction: this.#updateTransactionInternal.bind(this), + publishTransaction: ( + ethQuery: EthQuery, + transactionMeta: TransactionMeta, + ) => this.#publishTransaction(ethQuery, transactionMeta) as Promise, }); } diff --git a/packages/transaction-controller/src/hooks/SequentialPublishBatchHook.test.ts b/packages/transaction-controller/src/hooks/SequentialPublishBatchHook.test.ts new file mode 100644 index 00000000000..da99b9f926e --- /dev/null +++ b/packages/transaction-controller/src/hooks/SequentialPublishBatchHook.test.ts @@ -0,0 +1,266 @@ +import type EthQuery from '@metamask/eth-query'; +import { rpcErrors } from '@metamask/rpc-errors'; +import type { Hex } from '@metamask/utils'; + +import { SequentialPublishBatchHook } from './SequentialPublishBatchHook'; +import { flushPromises } from '../../../../tests/helpers'; +import type { PublishBatchHookTransaction, TransactionMeta } from '../types'; + +jest.mock('@metamask/controller-utils', () => ({ + query: jest.fn(), +})); + +const queryMock = jest.requireMock('@metamask/controller-utils').query; + +const TRANSACTION_CHECK_INTERVAL = 5000; // 5 seconds +const MAX_TRANSACTION_CHECK_ATTEMPTS = 60; // 5 minutes + +const TRANSACTION_HASH_MOCK = '0x123'; +const TRANSACTION_HASH_2_MOCK = '0x456'; +const NETWORK_CLIENT_ID_MOCK = 'testNetworkClientId'; +const TRANSACTION_ID_MOCK = 'testTransactionId'; +const TRANSACTION_ID_2_MOCK = 'testTransactionId2'; +const RECEIPT_STATUS_SUCCESS = '0x1'; +const RECEIPT_STATUS_FAILURE = '0x0'; +const TRANSACTION_SIGNED_MOCK = + '0xabcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890'; +const TRANSACTION_SIGNED_2_MOCK = + '0xabcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567891'; +const TRANSACTION_PARAMS_MOCK = { + from: '0x1234567890abcdef1234567890abcdef12345678' as Hex, + to: '0xabcdef1234567890abcdef1234567890abcdef12' as Hex, + value: '0x1' as Hex, +}; +const TRANSACTION_1_MOCK = { + id: TRANSACTION_ID_MOCK, + signedTx: TRANSACTION_SIGNED_MOCK, + params: TRANSACTION_PARAMS_MOCK, +} as PublishBatchHookTransaction; +const TRANSACTION_2_MOCK = { + id: TRANSACTION_ID_2_MOCK, + signedTx: TRANSACTION_SIGNED_2_MOCK, + params: TRANSACTION_PARAMS_MOCK, +} as PublishBatchHookTransaction; + +const TRANSACTION_META_MOCK = { + id: TRANSACTION_ID_MOCK, + rawTx: '0xabcdef', +} as TransactionMeta; + +const TRANSACTION_META_2_MOCK = { + id: TRANSACTION_ID_2_MOCK, + rawTx: '0x123456', +} as TransactionMeta; + +describe('SequentialPublishBatchHook', () => { + let publishTransactionMock: jest.MockedFn< + (ethQuery: EthQuery, transactionMeta: TransactionMeta) => Promise + >; + let getTransactionMock: jest.MockedFn<(id: string) => TransactionMeta>; + let getEthQueryMock: jest.MockedFn<(networkClientId: string) => EthQuery>; + let ethQueryInstanceMock: EthQuery; + + beforeEach(() => { + jest.resetAllMocks(); + + publishTransactionMock = jest.fn(); + getTransactionMock = jest.fn(); + getEthQueryMock = jest.fn(); + + ethQueryInstanceMock = {} as EthQuery; + getEthQueryMock.mockReturnValue(ethQueryInstanceMock); + + getTransactionMock.mockImplementation((id) => { + if (id === TRANSACTION_ID_MOCK) { + return TRANSACTION_META_MOCK; + } + if (id === TRANSACTION_ID_2_MOCK) { + return TRANSACTION_META_2_MOCK; + } + throw new Error(`Transaction with ID ${id} not found`); + }); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe('getHook', () => { + it('publishes transactions sequentially and waits for confirmation', async () => { + queryMock + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ + status: 'empty', + }) + .mockResolvedValue({ + status: RECEIPT_STATUS_SUCCESS, + }); + + const transactions: PublishBatchHookTransaction[] = [ + TRANSACTION_1_MOCK, + TRANSACTION_2_MOCK, + ]; + + publishTransactionMock + .mockResolvedValueOnce(TRANSACTION_HASH_MOCK) + .mockResolvedValueOnce(TRANSACTION_HASH_2_MOCK); + + const sequentialPublishBatchHook = new SequentialPublishBatchHook({ + publishTransaction: publishTransactionMock, + getTransaction: getTransactionMock, + getEthQuery: getEthQueryMock, + }); + + const hook = sequentialPublishBatchHook.getHook(); + + const result = await hook({ + from: '0x123', + networkClientId: NETWORK_CLIENT_ID_MOCK, + transactions, + }); + + expect(publishTransactionMock).toHaveBeenCalledTimes(2); + expect(publishTransactionMock).toHaveBeenNthCalledWith( + 1, + ethQueryInstanceMock, + TRANSACTION_META_MOCK, + ); + expect(publishTransactionMock).toHaveBeenNthCalledWith( + 2, + ethQueryInstanceMock, + TRANSACTION_META_2_MOCK, + ); + + expect(queryMock).toHaveBeenCalledTimes(4); + expect(queryMock).toHaveBeenCalledWith( + ethQueryInstanceMock, + 'getTransactionReceipt', + [TRANSACTION_HASH_MOCK], + ); + expect(queryMock).toHaveBeenCalledWith( + ethQueryInstanceMock, + 'getTransactionReceipt', + [TRANSACTION_HASH_2_MOCK], + ); + + expect(result).toStrictEqual({ + results: [ + { transactionHash: TRANSACTION_HASH_MOCK }, + { transactionHash: TRANSACTION_HASH_2_MOCK }, + ], + }); + }); + + it('throws if a transaction fails to publish', async () => { + const transactions: PublishBatchHookTransaction[] = [TRANSACTION_1_MOCK]; + + publishTransactionMock.mockRejectedValueOnce( + new Error('Failed to publish transaction'), + ); + + const sequentialPublishBatchHook = new SequentialPublishBatchHook({ + publishTransaction: publishTransactionMock, + getTransaction: getTransactionMock, + getEthQuery: getEthQueryMock, + }); + + const hook = sequentialPublishBatchHook.getHook(); + + await expect( + hook({ + from: '0x123', + networkClientId: NETWORK_CLIENT_ID_MOCK, + transactions, + }), + ).rejects.toThrow( + rpcErrors.internal('Failed to publish sequential batch transaction'), + ); + + expect(publishTransactionMock).toHaveBeenCalledTimes(1); + expect(publishTransactionMock).toHaveBeenCalledWith( + ethQueryInstanceMock, + TRANSACTION_META_MOCK, + ); + expect(queryMock).not.toHaveBeenCalled(); + }); + + it('throws if a transaction is not confirmed', async () => { + const transactions: PublishBatchHookTransaction[] = [TRANSACTION_1_MOCK]; + + publishTransactionMock.mockResolvedValueOnce(TRANSACTION_HASH_MOCK); + + queryMock.mockResolvedValueOnce({ + status: RECEIPT_STATUS_FAILURE, + }); + + const sequentialPublishBatchHook = new SequentialPublishBatchHook({ + publishTransaction: publishTransactionMock, + getTransaction: getTransactionMock, + getEthQuery: getEthQueryMock, + }); + + const hook = sequentialPublishBatchHook.getHook(); + + await expect( + hook({ + from: '0x123', + networkClientId: NETWORK_CLIENT_ID_MOCK, + transactions, + }), + ).rejects.toThrow(`Failed to publish sequential batch transaction`); + + expect(publishTransactionMock).toHaveBeenCalledTimes(1); + expect(publishTransactionMock).toHaveBeenCalledWith( + ethQueryInstanceMock, + TRANSACTION_META_MOCK, + ); + expect(queryMock).toHaveBeenCalledTimes(1); + expect(queryMock).toHaveBeenCalledWith( + ethQueryInstanceMock, + 'getTransactionReceipt', + [TRANSACTION_HASH_MOCK], + ); + }); + + it('returns false if transaction confirmation exceeds max attempts', async () => { + jest.useFakeTimers(); + + const transactions: PublishBatchHookTransaction[] = [TRANSACTION_1_MOCK]; + + publishTransactionMock.mockResolvedValueOnce(TRANSACTION_HASH_MOCK); + + queryMock.mockImplementation(undefined); + + const sequentialPublishBatchHook = new SequentialPublishBatchHook({ + publishTransaction: publishTransactionMock, + getTransaction: getTransactionMock, + getEthQuery: getEthQueryMock, + }); + + const hook = sequentialPublishBatchHook.getHook(); + + const hookPromise = hook({ + from: '0x123', + networkClientId: NETWORK_CLIENT_ID_MOCK, + transactions, + }); + + // Advance time 60 times by the interval (5s) to simulate 60 polling attempts + for (let i = 0; i < MAX_TRANSACTION_CHECK_ATTEMPTS; i++) { + jest.advanceTimersByTime(TRANSACTION_CHECK_INTERVAL); + await flushPromises(); + } + + jest.advanceTimersByTime(TRANSACTION_CHECK_INTERVAL); + + await expect(hookPromise).rejects.toThrow( + 'Failed to publish sequential batch transaction', + ); + + expect(publishTransactionMock).toHaveBeenCalledTimes(1); + expect(queryMock).toHaveBeenCalledTimes(MAX_TRANSACTION_CHECK_ATTEMPTS); + + jest.useRealTimers(); + }); + }); +}); diff --git a/packages/transaction-controller/src/hooks/SequentialPublishBatchHook.ts b/packages/transaction-controller/src/hooks/SequentialPublishBatchHook.ts new file mode 100644 index 00000000000..62ba91d823d --- /dev/null +++ b/packages/transaction-controller/src/hooks/SequentialPublishBatchHook.ts @@ -0,0 +1,174 @@ +import { query } from '@metamask/controller-utils'; +import type EthQuery from '@metamask/eth-query'; +import { rpcErrors } from '@metamask/rpc-errors'; +import { createModuleLogger } from '@metamask/utils'; +import type { Hex } from '@metamask/utils'; + +import { projectLogger } from '../logger'; +import type { + PublishBatchHookTransaction, + TransactionMeta, + TransactionReceipt, +} from '../types'; + +const TRANSACTION_CHECK_INTERVAL = 5000; // 5 seconds +const MAX_TRANSACTION_CHECK_ATTEMPTS = 60; // 5 minutes +const RECEIPT_STATUS_SUCCESS = '0x1'; +const RECEIPT_STATUS_FAILURE = '0x0'; + +const log = createModuleLogger(projectLogger, 'sequential-publish-batch-hook'); + +type SequentialPublishBatchHookParams = { + publishTransaction: ( + _ethQuery: EthQuery, + transactionMeta: TransactionMeta, + ) => Promise; + getTransaction: (id: string) => TransactionMeta; + getEthQuery: (networkClientId: string) => EthQuery; +}; +/** + * Custom publish logic that also publishes additional sequential transactions in an batch. + * Requires the batch to be successful to resolve. + */ +export class SequentialPublishBatchHook { + readonly #publishTransaction: ( + _ethQuery: EthQuery, + transactionMeta: TransactionMeta, + ) => Promise; + + readonly #getTransaction: (id: string) => TransactionMeta; + + readonly #getEthQuery: (networkClientId: string) => EthQuery; + + constructor({ + publishTransaction, + getTransaction, + getEthQuery, + }: SequentialPublishBatchHookParams) { + this.#publishTransaction = publishTransaction; + this.#getTransaction = getTransaction; + this.#getEthQuery = getEthQuery; + } + + /** + * Get the hook function for sequential publishing. + * + * @returns The hook function. + */ + getHook() { + return async ({ + from, + networkClientId, + transactions, + }: { + from: string; + networkClientId: string; + transactions: PublishBatchHookTransaction[]; + }) => { + log('Starting sequential publish batch hook', { from, networkClientId }); + + const results = []; + + for (const transaction of transactions) { + try { + const transactionMeta = this.#getTransaction(String(transaction.id)); + const transactionHash = await this.#publishTransaction( + this.#getEthQuery(networkClientId), + transactionMeta, + ); + log('Transaction published', { transactionHash }); + + const isConfirmed = await this.#waitForTransactionConfirmation( + transactionHash, + networkClientId, + ); + + if (!isConfirmed) { + throw new Error( + `Transaction ${transactionHash} failed or was not confirmed.`, + ); + } + + results.push({ transactionHash }); + } catch (error) { + log('Transaction failed', { transaction, error }); + throw rpcErrors.internal( + `Failed to publish sequential batch transaction`, + ); + } + } + + log('Sequential publish batch hook completed', { results }); + + return { results }; + }; + } + + async #waitForTransactionConfirmation( + transactionHash: string, + networkClientId: string, + ): Promise { + let attempts = 0; + + while (attempts < MAX_TRANSACTION_CHECK_ATTEMPTS) { + const isConfirmed = await this.#isTransactionConfirmed( + transactionHash, + networkClientId, + ); + + if (isConfirmed) { + return true; + } + + const waitPromise = new Promise((resolve) => + setTimeout(resolve, TRANSACTION_CHECK_INTERVAL), + ); + await waitPromise; + + attempts += 1; + } + + return false; + } + + async #getTransactionReceipt( + txHash: string, + networkClientId: string, + ): Promise { + return await query( + this.#getEthQuery(networkClientId), + 'getTransactionReceipt', + [txHash], + ); + } + + async #isTransactionConfirmed( + transactionHash: string, + networkClientId: string, + ): Promise { + try { + const receipt = await this.#getTransactionReceipt( + transactionHash, + networkClientId, + ); + if (!receipt) { + return false; + } + + const { status } = receipt; + + if (status === RECEIPT_STATUS_SUCCESS) { + return true; + } + + if (status === RECEIPT_STATUS_FAILURE) { + throw new Error(`Transaction ${transactionHash} failed.`); + } + + return false; + } catch (error) { + log('Error checking transaction status', { transactionHash, error }); + throw error; + } + } +} diff --git a/packages/transaction-controller/src/utils/batch.test.ts b/packages/transaction-controller/src/utils/batch.test.ts index 2e2b9c2ac2e..3dcb592b721 100644 --- a/packages/transaction-controller/src/utils/batch.test.ts +++ b/packages/transaction-controller/src/utils/batch.test.ts @@ -19,6 +19,7 @@ import { TransactionType, } from '..'; import { flushPromises } from '../../../../tests/helpers'; +import { SequentialPublishBatchHook } from '../hooks/SequentialPublishBatchHook'; import type { PublishBatchHook } from '../types'; jest.mock('./eip7702'); @@ -30,6 +31,8 @@ jest.mock('./validation', () => ({ validateBatchRequest: jest.fn(), })); +jest.mock('../hooks/SequentialPublishBatchHook'); + type AddBatchTransactionOptions = Parameters[0]; const CHAIN_ID_MOCK = '0x123'; @@ -71,6 +74,9 @@ describe('Batch Utils', () => { const getEIP7702SupportedChainsMock = jest.mocked(getEIP7702SupportedChains); const validateBatchRequestMock = jest.mocked(validateBatchRequest); const determineTransactionTypeMock = jest.mocked(determineTransactionType); + const sequentialPublishBatchHookMock = jest.mocked( + SequentialPublishBatchHook, + ); const isAccountUpgradedToEIP7702Mock = jest.mocked( isAccountUpgradedToEIP7702, @@ -97,6 +103,10 @@ describe('Batch Utils', () => { AddBatchTransactionOptions['updateTransaction'] >; + let publishTransactionMock: jest.MockedFn< + AddBatchTransactionOptions['publishTransaction'] + >; + let request: AddBatchTransactionOptions; beforeEach(() => { @@ -104,6 +114,7 @@ describe('Batch Utils', () => { addTransactionMock = jest.fn(); getChainIdMock = jest.fn(); updateTransactionMock = jest.fn(); + publishTransactionMock = jest.fn(); determineTransactionTypeMock.mockResolvedValue({ type: TransactionType.simpleSend, @@ -141,6 +152,7 @@ describe('Batch Utils', () => { ], }, updateTransaction: updateTransactionMock, + publishTransaction: publishTransactionMock, }; }); @@ -961,15 +973,6 @@ describe('Batch Utils', () => { ); }); - it('throws if no publish batch hook', async () => { - await expect( - addTransactionBatch({ - ...request, - request: { ...request.request, useHook: true }, - }), - ).rejects.toThrow(rpcErrors.internal('No publish batch hook provided')); - }); - it('rejects individual publish hooks if batch hook throws', async () => { const publishBatchHook: jest.MockedFn = jest.fn(); @@ -1072,6 +1075,146 @@ describe('Batch Utils', () => { await expect(publishHookPromise1).rejects.toThrow(ERROR_MESSAGE_MOCK); }); }); + + describe('with sequential publish batch hook', () => { + let publishBatchHook: jest.MockedFn; + + beforeEach(() => { + publishBatchHook = jest.fn(); + + addTransactionMock + .mockResolvedValueOnce({ + transactionMeta: { + ...TRANSACTION_META_MOCK, + id: TRANSACTION_ID_MOCK, + }, + result: Promise.resolve(''), + }) + .mockResolvedValueOnce({ + transactionMeta: { + ...TRANSACTION_META_MOCK, + id: TRANSACTION_ID_2_MOCK, + }, + result: Promise.resolve(''), + }); + }); + + const setupSequentialPublishBatchHookMock = ( + hookImplementation: () => PublishBatchHook | undefined, + ) => { + sequentialPublishBatchHookMock.mockReturnValue({ + getHook: hookImplementation, + } as unknown as SequentialPublishBatchHook); + }; + + const executePublishHooks = async () => { + const publishHooks = addTransactionMock.mock.calls.map( + ([, options]) => options.publishHook, + ); + + publishHooks[0]?.( + TRANSACTION_META_MOCK, + TRANSACTION_SIGNATURE_MOCK, + ).catch(() => { + // Intentionally empty + }); + + publishHooks[1]?.( + TRANSACTION_META_MOCK, + TRANSACTION_SIGNATURE_2_MOCK, + ).catch(() => { + // Intentionally empty + }); + + await flushPromises(); + }; + + it('calls sequentialPublishBatchHook when publishBatchHook is undefined', async () => { + publishBatchHook.mockResolvedValueOnce({ + results: [ + { + transactionHash: TRANSACTION_HASH_MOCK, + }, + { + transactionHash: TRANSACTION_HASH_2_MOCK, + }, + ], + }); + + setupSequentialPublishBatchHookMock(() => publishBatchHook); + + addTransactionBatch({ + ...request, + publishBatchHook: undefined, + request: { ...request.request, useHook: true }, + }).catch(() => { + // Intentionally empty + }); + + await flushPromises(); + await executePublishHooks(); + + expect(sequentialPublishBatchHookMock).toHaveBeenCalledTimes(1); + expect(publishBatchHook).toHaveBeenCalledTimes(1); + expect(publishBatchHook).toHaveBeenCalledWith({ + from: FROM_MOCK, + networkClientId: NETWORK_CLIENT_ID_MOCK, + transactions: [ + expect.objectContaining({ + id: TRANSACTION_ID_MOCK, + params: { data: DATA_MOCK, to: TO_MOCK, value: VALUE_MOCK }, + signedTx: TRANSACTION_SIGNATURE_MOCK, + }), + expect.objectContaining({ + id: TRANSACTION_ID_2_MOCK, + params: { data: DATA_MOCK, to: TO_MOCK, value: VALUE_MOCK }, + signedTx: TRANSACTION_SIGNATURE_2_MOCK, + }), + ], + }); + }); + + it('throws if sequentialPublishBatchHook does not return a result', async () => { + const publishBatchHookMock: jest.MockedFn = jest.fn(); + publishBatchHookMock.mockResolvedValueOnce(undefined); + setupSequentialPublishBatchHookMock(() => publishBatchHookMock); + + const resultPromise = addTransactionBatch({ + ...request, + publishBatchHook: undefined, + request: { ...request.request, useHook: true }, + }); + + resultPromise.catch(() => { + // Intentionally empty + }); + + await flushPromises(); + await executePublishHooks(); + + await expect(resultPromise).rejects.toThrow( + 'Publish batch hook did not return a result', + ); + await flushPromises(); + expect(sequentialPublishBatchHookMock).toHaveBeenCalledTimes(1); + }); + + it('handles individual transaction failures when using sequentialPublishBatchHook', async () => { + setupSequentialPublishBatchHookMock(() => { + throw new Error('Test error'); + }); + + await expect( + addTransactionBatch({ + ...request, + publishBatchHook: undefined, + request: { ...request.request, useHook: true }, + }), + ).rejects.toThrow('Test error'); + + expect(sequentialPublishBatchHookMock).toHaveBeenCalledTimes(1); + }); + }); }); describe('isAtomicBatchSupported', () => { diff --git a/packages/transaction-controller/src/utils/batch.ts b/packages/transaction-controller/src/utils/batch.ts index e79d3d33397..492ddea6a54 100644 --- a/packages/transaction-controller/src/utils/batch.ts +++ b/packages/transaction-controller/src/utils/batch.ts @@ -23,6 +23,7 @@ import { type TransactionMeta, } from '..'; import { CollectPublishHook } from '../hooks/CollectPublishHook'; +import { SequentialPublishBatchHook } from '../hooks/SequentialPublishBatchHook'; import { projectLogger } from '../logger'; import type { NestedTransactionMetadata, @@ -57,6 +58,10 @@ type AddTransactionBatchRequest = { options: { transactionId: string }, callback: (transactionMeta: TransactionMeta) => void, ) => void; + publishTransaction: ( + _ethQuery: EthQuery, + transactionMeta: TransactionMeta, + ) => Promise; }; type IsAtomicBatchSupportedRequestInternal = { @@ -332,7 +337,8 @@ async function getNestedTransactionMeta( async function addTransactionBatchWithHook( request: AddTransactionBatchRequest, ): Promise { - const { publishBatchHook, request: userRequest } = request; + const { publishBatchHook: initialPublishBatchHook, request: userRequest } = + request; const { from, @@ -342,10 +348,14 @@ async function addTransactionBatchWithHook( log('Adding transaction batch using hook', userRequest); - if (!publishBatchHook) { - log('No publish batch hook provided'); - throw new Error('No publish batch hook provided'); - } + const sequentialPublishBatchHook = new SequentialPublishBatchHook({ + publishTransaction: request.publishTransaction, + getTransaction: request.getTransaction, + getEthQuery: request.getEthQuery, + }); + + const publishBatchHook = + initialPublishBatchHook ?? sequentialPublishBatchHook.getHook(); const batchId = generateBatchId(); const transactionCount = nestedTransactions.length;