From 25c2c5642e0848474b0d7f9ccc00fab5dc7d7716 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Thu, 14 Sep 2023 08:17:22 +0100 Subject: [PATCH 1/6] Provide local transactions to IncomingTransactionHelper --- packages/transaction-controller/src/TransactionController.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index abd07f91f48..518a74a7c88 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -3327,6 +3327,7 @@ export class TransactionController extends BaseController< blockTracker, getCurrentAccount: () => this.#getSelectedAccount(), getLastFetchedBlockNumbers: () => this.state.lastFetchedBlockNumbers, + getLocalTransactions: () => this.state.transactions, getChainId: chainId ? () => chainId : this.getChainId.bind(this), isEnabled: this.#incomingTransactionOptions.isEnabled, queryEntireHistory: this.#incomingTransactionOptions.queryEntireHistory, From 9e546471182356c8b76d82c640688b70ed951f15 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Sun, 17 Dec 2023 13:23:01 +0000 Subject: [PATCH 2/6] Add submit history --- .../src/TransactionController.test.ts | 1 + .../src/TransactionController.ts | 79 ++++++++++++++++--- .../helpers/PendingTransactionTracker.test.ts | 5 +- .../src/helpers/PendingTransactionTracker.ts | 14 ++-- packages/transaction-controller/src/types.ts | 37 +++++++++ 5 files changed, 117 insertions(+), 19 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 7c7c45f1dd7..3ede22a2228 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -873,6 +873,7 @@ describe('TransactionController', () => { methodData: {}, transactions: [], lastFetchedBlockNumbers: {}, + submitHistory: [], }); }); diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 518a74a7c88..458d9a3afc5 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -81,6 +81,7 @@ import type { GasFeeFlowResponse, GasPriceValue, FeeMarketEIP1559Values, + SubmitHistoryEntry, } from './types'; import { TransactionEnvelopeType, @@ -141,9 +142,14 @@ const metadata = { persist: true, anonymous: false, }, + submitHistory: { + persist: true, + anonymous: false, + }, }; export const HARDFORK = Hardfork.London; +const SUBMIT_HISTORY_LIMIT = 100; /** * Object with new transaction's meta and a promise resolving to the @@ -196,6 +202,7 @@ export type TransactionControllerState = { transactions: TransactionMeta[]; methodData: Record; lastFetchedBlockNumbers: { [key: string]: number }; + submitHistory: SubmitHistoryEntry[]; }; /** @@ -555,6 +562,7 @@ function getDefaultTransactionControllerState(): TransactionControllerState { methodData: {}, transactions: [], lastFetchedBlockNumbers: {}, + submitHistory: [], }; } @@ -1356,17 +1364,10 @@ export class TransactionController extends BaseController< chainId: transactionMeta.chainId, }); - const hash = await this.publishTransactionForRetry( - ethQuery, - rawTx, - transactionMeta, - ); - const newTransactionMeta = { ...transactionMetaWithRsv, actionId, estimatedBaseFee, - hash, id: random(), originalGasEstimate: transactionMeta.txParams.gas, originalType: transactionMeta.type, @@ -1376,6 +1377,13 @@ export class TransactionController extends BaseController< type: transactionType, }; + const hash = await this.publishTransactionForRetry(ethQuery, { + ...newTransactionMeta, + origin: ORIGIN_METAMASK, + }); + + newTransactionMeta.hash = hash; + this.addMetadata(newTransactionMeta); // speedUpTransaction has no approval request, so we assume the user has already approved the transaction @@ -2611,7 +2619,10 @@ export class TransactionController extends BaseController< )); if (hash === undefined) { - hash = await this.publishTransaction(ethQuery, rawTx); + hash = await this.publishTransaction(ethQuery, { + ...transactionMeta, + rawTx, + }); } }, ); @@ -2658,9 +2669,15 @@ export class TransactionController extends BaseController< private async publishTransaction( ethQuery: EthQuery, - rawTransaction: string, + transactionMeta: TransactionMeta, ): Promise { - return await query(ethQuery, 'sendRawTransaction', [rawTransaction]); + const transactionHash = await query(ethQuery, 'sendRawTransaction', [ + transactionMeta.rawTx, + ]); + + this.#updateSubmitHistory(transactionMeta, transactionHash); + + return transactionHash; } /** @@ -3469,12 +3486,10 @@ export class TransactionController extends BaseController< private async publishTransactionForRetry( ethQuery: EthQuery, - rawTx: string, transactionMeta: TransactionMeta, ): Promise { try { - const hash = await this.publishTransaction(ethQuery, rawTx); - return hash; + return await this.publishTransaction(ethQuery, transactionMeta); } catch (error: unknown) { if (this.isTransactionAlreadyConfirmedError(error as Error)) { await this.pendingTransactionTracker.forceCheckTransaction( @@ -3765,4 +3780,42 @@ export class TransactionController extends BaseController< #getSelectedAccount() { return this.messagingSystem.call('AccountsController:getSelectedAccount'); } + + #updateSubmitHistory(transactionMeta: TransactionMeta, hash: string): void { + const { chainId, networkClientId, origin, rawTx, txParams } = + transactionMeta; + + const { networkConfigurationsByChainId } = this.getNetworkState(); + const networkConfiguration = networkConfigurationsByChainId[chainId as Hex]; + + const endpoint = networkConfiguration?.rpcEndpoints.find( + (currentEndpoint) => currentEndpoint.networkClientId === networkClientId, + ); + + const networkUrl = endpoint?.url; + const networkType = endpoint?.name ?? networkClientId; + + const submitHistoryEntry: SubmitHistoryEntry = { + chainId, + hash, + networkType, + networkUrl, + origin, + rawTransaction: rawTx as string, + time: Date.now(), + transaction: txParams, + }; + + log('Updating submit history', submitHistoryEntry); + + this.update((state) => { + const { submitHistory } = state; + + if (submitHistory.length === SUBMIT_HISTORY_LIMIT) { + submitHistory.pop(); + } + + submitHistory.unshift(submitHistoryEntry); + }); + } } diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts index 46b6e261513..b28b9693925 100644 --- a/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts +++ b/packages/transaction-controller/src/helpers/PendingTransactionTracker.test.ts @@ -887,7 +887,10 @@ describe('PendingTransactionTracker', () => { expect(options.publishTransaction).toHaveBeenCalledTimes(1); expect(options.publishTransaction).toHaveBeenCalledWith( ETH_QUERY_MOCK, - TRANSACTION_SUBMITTED_MOCK.rawTx, + { + ...TRANSACTION_SUBMITTED_MOCK, + firstRetryBlockNumber: BLOCK_NUMBER_MOCK, + }, ); }); diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts index a6930afc4cd..dc4e12d8c7b 100644 --- a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts +++ b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts @@ -81,7 +81,10 @@ export class PendingTransactionTracker { #getGlobalLock: () => Promise<() => void>; - #publishTransaction: (ethQuery: EthQuery, rawTx: string) => Promise; + #publishTransaction: ( + ethQuery: EthQuery, + transactionMeta: TransactionMeta, + ) => Promise; #running: boolean; @@ -105,7 +108,10 @@ export class PendingTransactionTracker { getTransactions: () => TransactionMeta[]; isResubmitEnabled?: () => boolean; getGlobalLock: () => Promise<() => void>; - publishTransaction: (ethQuery: EthQuery, rawTx: string) => Promise; + publishTransaction: ( + ethQuery: EthQuery, + transactionMeta: TransactionMeta, + ) => Promise; hooks?: { beforeCheckPendingTransaction?: ( transactionMeta: TransactionMeta, @@ -279,14 +285,12 @@ export class PendingTransactionTracker { return; } - const { rawTx } = txMeta; - if (!this.#beforePublish(txMeta)) { return; } const ethQuery = this.#getEthQuery(txMeta.networkClientId); - await this.#publishTransaction(ethQuery, rawTx as string); + await this.#publishTransaction(ethQuery, txMeta); const retryCount = (txMeta.retryCount ?? 0) + 1; diff --git a/packages/transaction-controller/src/types.ts b/packages/transaction-controller/src/types.ts index 143c8295407..b8839cc1cce 100644 --- a/packages/transaction-controller/src/types.ts +++ b/packages/transaction-controller/src/types.ts @@ -1314,3 +1314,40 @@ export type FeeMarketEIP1559Values = { /** Maximum amount per gas to give to the validator as an incentive. */ maxPriorityFeePerGas: string; }; + +/** + * Data concerning a successfully submitted transaction. + * Used for debugging purposes. + */ +export type SubmitHistoryEntry = { + /** The chain ID of the transaction as a hexadecimal string. */ + chainId?: Hex; + + /** The hash of the transaction returned from the RPC provider. */ + hash: string; + + /** True if the entry was generated using the migration and existing transaction metadata. */ + migration?: boolean; + + /** The type of the network where the transaction was submitted. */ + networkType?: string; + + /** + * The URL of the network the transaction was submitted to. + * A single network URL if it was recorded when submitted. + * An array of potential network URLs if it cannot be confirmed since the migration was used. + */ + networkUrl?: string | string[]; + + /** The origin of the transaction. */ + origin?: string; + + /** The raw transaction data that was submitted. */ + rawTransaction: string; + + /** When the transaction was submitted. */ + time: number; + + /** The transaction parameters that were submitted. */ + transaction: TransactionParams; +}; From 340e40be738e87480bf93150fb53de83d965fc29 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Fri, 5 Jul 2024 13:45:38 +0100 Subject: [PATCH 3/6] Export constants --- packages/transaction-controller/src/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/transaction-controller/src/index.ts b/packages/transaction-controller/src/index.ts index 45651a46ec7..b6bbe1af56a 100644 --- a/packages/transaction-controller/src/index.ts +++ b/packages/transaction-controller/src/index.ts @@ -27,6 +27,7 @@ export type { export { HARDFORK, CANCEL_RATE, + SPEED_UP_RATE, TransactionController, } from './TransactionController'; export type { @@ -81,3 +82,4 @@ export { isEIP1559Transaction, normalizeTransactionParams, } from './utils/utils'; +export { CHAIN_IDS, ETHERSCAN_SUPPORTED_NETWORKS } from './constants'; From b2dc33eb8f7e084654f1439b69411620c1aef723 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Wed, 21 Aug 2024 18:13:22 +0100 Subject: [PATCH 4/6] Make getPermittedAccounts callback optional --- .../transaction-controller/src/TransactionController.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 458d9a3afc5..1684acf0564 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -288,7 +288,7 @@ export type TransactionControllerOptions = { ) => Promise; getNetworkClientRegistry: NetworkController['getNetworkClientRegistry']; getNetworkState: () => NetworkState; - getPermittedAccounts: (origin?: string) => Promise; + getPermittedAccounts?: (origin?: string) => Promise; getSavedGasFees?: (chainId: Hex) => SavedGasFees | undefined; incomingTransactions?: IncomingTransactionOptions; isMultichainEnabled: boolean; @@ -606,7 +606,9 @@ export class TransactionController extends BaseController< options: FetchGasFeeEstimateOptions, ) => Promise; - private readonly getPermittedAccounts: (origin?: string) => Promise; + private readonly getPermittedAccounts?: ( + origin?: string, + ) => Promise; private readonly getExternalPendingTransactions: ( address: string, @@ -1048,7 +1050,7 @@ export class TransactionController extends BaseController< validateTxParams(txParams, isEIP1559Compatible); - if (origin) { + if (origin && this.getPermittedAccounts) { await validateTransactionOrigin( await this.getPermittedAccounts(origin), this.#getSelectedAccount().address, From 78ca0129400d79512c9befa5e9a32fd9b795b50d Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Mon, 16 Sep 2024 12:31:14 +0100 Subject: [PATCH 5/6] Add unit tests --- .../transaction-controller/jest.config.js | 8 +- .../src/TransactionController.test.ts | 205 +++++++++++++++++- .../src/TransactionController.ts | 2 +- 3 files changed, 209 insertions(+), 6 deletions(-) diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index c33dbefbb59..c8779f9f119 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: 94.37, - functions: 97.94, - lines: 98.52, - statements: 98.53, + branches: 94.41, + functions: 97.7, + lines: 98.48, + statements: 98.5, }, }, diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 3ede22a2228..030703f8cba 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -46,6 +46,7 @@ import { buildCustomNetworkClientConfiguration, buildMockGetNetworkClientById, } from '../../network-controller/tests/helpers'; +import { CHAIN_IDS } from './constants'; import { DefaultGasFeeFlow } from './gas-flows/DefaultGasFeeFlow'; import { LineaGasFeeFlow } from './gas-flows/LineaGasFeeFlow'; import { TestGasFeeFlow } from './gas-flows/TestGasFeeFlow'; @@ -69,6 +70,7 @@ import type { SimulationData, GasFeeFlow, GasFeeFlowResponse, + SubmitHistoryEntry, } from './types'; import { GasFeeEstimateType, @@ -97,6 +99,7 @@ type UnrestrictedControllerMessenger = ControllerMessenger< >; const MOCK_V1_UUID = '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d'; +const TRANSACTION_HASH_MOCK = '0x123456'; jest.mock('@metamask/eth-query'); jest.mock('./gas-flows/DefaultGasFeeFlow'); @@ -191,7 +194,7 @@ function buildMockEthQuery(): EthQuery { // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any sendRawTransaction: (_transaction: unknown, callback: any) => { - callback(undefined, 'somehash'); + callback(undefined, TRANSACTION_HASH_MOCK); }, // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -2265,6 +2268,100 @@ describe('TransactionController', () => { ); }); }); + + describe('updates submit history', () => { + it('adds entry to start of array', async () => { + const { controller } = setupController({ + messengerOptions: { + addTransactionApprovalRequest: { + state: 'approved', + }, + }, + options: { + state: { + submitHistory: [ + { + chainId: CHAIN_IDS.LINEA_MAINNET, + } as unknown as SubmitHistoryEntry, + ], + }, + }, + }); + + const { result } = await controller.addTransaction( + { + from: ACCOUNT_MOCK, + to: ACCOUNT_MOCK, + }, + { origin: ORIGIN_METAMASK }, + ); + + await result; + + expect(controller.state.submitHistory).toStrictEqual([ + { + chainId: MOCK_NETWORK.chainId, + hash: TRANSACTION_HASH_MOCK, + networkType: NetworkType.goerli, + networkUrl: expect.stringContaining('goerli.infura.io'), + origin: ORIGIN_METAMASK, + rawTransaction: expect.stringContaining('0x'), + time: expect.any(Number), + transaction: { + from: ACCOUNT_MOCK, + nonce: '0xc', + to: ACCOUNT_MOCK, + value: '0x0', + }, + }, + { + chainId: CHAIN_IDS.LINEA_MAINNET, + }, + ]); + }); + + it('removes last entry if reached maximum size', async () => { + const existingSubmitHistory = Array(100); + + existingSubmitHistory[99] = { + chainId: CHAIN_IDS.LINEA_MAINNET, + } as unknown as SubmitHistoryEntry; + + const { controller } = setupController({ + messengerOptions: { + addTransactionApprovalRequest: { + state: 'approved', + }, + }, + options: { + state: { + submitHistory: existingSubmitHistory, + }, + }, + }); + + const { result } = await controller.addTransaction( + { + from: ACCOUNT_MOCK, + to: ACCOUNT_MOCK, + }, + { + origin: ORIGIN_METAMASK, + }, + ); + + await result; + + expect(controller.state.submitHistory).toHaveLength(100); + expect(controller.state.submitHistory[0]).toStrictEqual( + expect.objectContaining({ + chainId: MOCK_NETWORK.chainId, + origin: ORIGIN_METAMASK, + }), + ); + expect(controller.state.submitHistory[99]).toBeUndefined(); + }); + }); }); describe('wipeTransactions', () => { @@ -2734,6 +2831,59 @@ describe('TransactionController', () => { expect(finishedEventListener).toHaveBeenCalledTimes(2); expect(finishedEventListener).toHaveBeenCalledWith(cancelTransaction); }); + + it('updates submit history', async () => { + const { controller } = setupController({ + messengerOptions: { + addTransactionApprovalRequest: { + state: 'approved', + }, + }, + }); + + const { result } = await controller.addTransaction( + { + from: ACCOUNT_MOCK, + gas: '0xFF', + gasPrice: '0xEE', + to: ACCOUNT_MOCK, + value: '0x0', + }, + { + origin: ORIGIN_METAMASK, + }, + ); + + await result; + + await controller.stopTransaction(controller.state.transactions[0].id); + + const { submitHistory } = controller.state; + + expect(submitHistory).toStrictEqual([ + { + chainId: MOCK_NETWORK.chainId, + hash: TRANSACTION_HASH_MOCK, + networkType: NetworkType.goerli, + networkUrl: expect.stringContaining('goerli.infura.io'), + origin: 'cancel', + rawTransaction: expect.stringContaining('0x'), + time: expect.any(Number), + transaction: { + from: ACCOUNT_MOCK, + gas: '0xFF', + gasLimit: '0xFF', + gasPrice: '0x105', + nonce: '0xc', + to: ACCOUNT_MOCK, + value: '0x0', + }, + }, + expect.objectContaining({ + origin: ORIGIN_METAMASK, + }), + ]); + }); }); describe('speedUpTransaction', () => { @@ -3096,6 +3246,59 @@ describe('TransactionController', () => { expect(speedupEventListener).toHaveBeenCalledTimes(1); expect(speedupEventListener).toHaveBeenCalledWith(speedUpTransaction); }); + + it('updates submit history', async () => { + const { controller } = setupController({ + messengerOptions: { + addTransactionApprovalRequest: { + state: 'approved', + }, + }, + }); + + const { result } = await controller.addTransaction( + { + from: ACCOUNT_MOCK, + gas: '0xFF', + gasPrice: '0xEE', + to: ACCOUNT_MOCK, + value: '0x0', + }, + { + origin: ORIGIN_METAMASK, + }, + ); + + await result; + + await controller.speedUpTransaction(controller.state.transactions[0].id); + + const { submitHistory } = controller.state; + + expect(submitHistory).toStrictEqual([ + { + chainId: MOCK_NETWORK.chainId, + hash: TRANSACTION_HASH_MOCK, + networkType: NetworkType.goerli, + networkUrl: expect.stringContaining('goerli.infura.io'), + origin: 'speed up', + rawTransaction: expect.stringContaining('0x'), + time: expect.any(Number), + transaction: { + from: ACCOUNT_MOCK, + gas: '0xFF', + gasLimit: '0xFF', + gasPrice: '0x105', + nonce: '0xc', + to: ACCOUNT_MOCK, + value: '0x0', + }, + }, + expect.objectContaining({ + origin: ORIGIN_METAMASK, + }), + ]); + }); }); describe('confirmExternalTransaction', () => { diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 1684acf0564..0668ff6e7c4 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -1381,7 +1381,7 @@ export class TransactionController extends BaseController< const hash = await this.publishTransactionForRetry(ethQuery, { ...newTransactionMeta, - origin: ORIGIN_METAMASK, + origin: label, }); newTransactionMeta.hash = hash; From 79ac9d6adb0b1da97eca87912e5236073ecffed9 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Mon, 16 Sep 2024 12:46:34 +0100 Subject: [PATCH 6/6] Disable submit history during resubmit --- packages/transaction-controller/jest.config.js | 8 ++++---- .../src/TransactionController.ts | 10 ++++++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index c8779f9f119..a7cd29f6724 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: 94.41, - functions: 97.7, - lines: 98.48, - statements: 98.5, + branches: 94.42, + functions: 97.46, + lines: 98.44, + statements: 98.46, }, }, diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 0668ff6e7c4..b44e2e03f3d 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -2672,12 +2672,15 @@ export class TransactionController extends BaseController< private async publishTransaction( ethQuery: EthQuery, transactionMeta: TransactionMeta, + { skipSubmitHistory }: { skipSubmitHistory?: boolean } = {}, ): Promise { const transactionHash = await query(ethQuery, 'sendRawTransaction', [ transactionMeta.rawTx, ]); - this.#updateSubmitHistory(transactionMeta, transactionHash); + if (skipSubmitHistory !== true) { + this.#updateSubmitHistory(transactionMeta, transactionHash); + } return transactionHash; } @@ -3382,7 +3385,10 @@ export class TransactionController extends BaseController< this.#multichainTrackingHelper.acquireNonceLockForChainIdKey({ chainId: getChainId(), }), - publishTransaction: this.publishTransaction.bind(this), + publishTransaction: (_ethQuery, transactionMeta) => + this.publishTransaction(_ethQuery, transactionMeta, { + skipSubmitHistory: true, + }), hooks: { beforeCheckPendingTransaction: this.beforeCheckPendingTransaction.bind(this),