From 5679f2b0ccdc94906bf3522ca0816edd70cc176a Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 28 Mar 2023 14:05:51 -0500 Subject: [PATCH 1/7] add nonceTracker to TransactionsController --- .../src/NetworkController.ts | 2 +- packages/transaction-controller/package.json | 1 + .../src/TransactionController.test.ts | 343 ++++++++++++++---- .../src/TransactionController.ts | 76 +++- .../transaction-controller/src/utils.test.ts | 58 +++ packages/transaction-controller/src/utils.ts | 30 ++ yarn.lock | 59 ++- 7 files changed, 471 insertions(+), 98 deletions(-) diff --git a/packages/network-controller/src/NetworkController.ts b/packages/network-controller/src/NetworkController.ts index b81cb262dc..25fd7cf8cc 100644 --- a/packages/network-controller/src/NetworkController.ts +++ b/packages/network-controller/src/NetworkController.ts @@ -100,7 +100,7 @@ export type ProviderProxy = SwappableProxy; type BlockTracker = any; -type BlockTrackerProxy = SwappableProxy; +export type BlockTrackerProxy = SwappableProxy; export type NetworkControllerStateChangeEvent = { type: `NetworkController:stateChange`; diff --git a/packages/transaction-controller/package.json b/packages/transaction-controller/package.json index 3368fdd4f6..83157551ec 100644 --- a/packages/transaction-controller/package.json +++ b/packages/transaction-controller/package.json @@ -40,6 +40,7 @@ "eth-query": "^2.1.2", "eth-rpc-errors": "^4.0.0", "ethereumjs-util": "^7.0.10", + "nonce-tracker": "^1.1.0", "uuid": "^8.3.2" }, "devDependencies": { diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 13b3d03f2d..97dbc8cbeb 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -1,5 +1,6 @@ import * as sinon from 'sinon'; import HttpProvider from 'ethjs-provider-http'; +// import uuid from 'uuid'; import { NetworksChainId, NetworkType } from '@metamask/controller-utils'; import type { NetworkState } from '@metamask/network-controller'; import { ESTIMATE_GAS_ERROR } from './utils'; @@ -17,10 +18,14 @@ import { txsInStateWithOutdatedStatusAndGasDataMock, } from './mocks/txsMock'; +const v1Stub = jest + .fn() + .mockImplementation(() => '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d'); + jest.mock('uuid', () => { return { ...jest.requireActual('uuid'), - v1: () => '9b1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d', + v1: () => v1Stub(), }; }); @@ -92,6 +97,23 @@ jest.mock('eth-query', () => }), ); +const getNonceLockStub = jest.fn().mockImplementation(() => { + return { + releaseLock: () => Promise.resolve(), + nextNonce: '0', + }; +}); + +jest.mock('nonce-tracker', () => { + return jest.fn().mockImplementation(() => { + return { + getGlobalLock: () => Promise.resolve(), + getNonceLock: getNonceLockStub, + releaseLock: () => Promise.resolve(), + }; + }); +}); + /** * Create a mock implementation of `fetch` that always returns the same data. * @@ -122,14 +144,20 @@ function mockFetchWithDynamicResponse(dataForUrl: any) { } const MOCK_PRFERENCES = { state: { selectedAddress: 'foo' } }; -const PROVIDER = new HttpProvider( +const GOERLI_PROVIDER = new HttpProvider( 'https://goerli.infura.io/v3/341eacb578dd44a1a049cbc5f6fd4035', ); const MAINNET_PROVIDER = new HttpProvider( 'https://mainnet.infura.io/v3/341eacb578dd44a1a049cbc5f6fd4035', ); +// const PALM_PROVIDER = new HttpProvider('https://mainnet.optimism.io'); +const PALM_PROVIDER = new HttpProvider( + 'https://palm-mainnet.infura.io/v3/3a961d6501e54add9a41aa53f15de99b', +); + const MOCK_NETWORK = { - getProvider: () => PROVIDER, + provider: MAINNET_PROVIDER, + blockTracker: { getLatestBlock: () => '0x102833C' }, state: { network: '5', isCustomNetwork: false, @@ -142,22 +170,9 @@ const MOCK_NETWORK = { }, subscribe: () => undefined, }; -const MOCK_NETWORK_CUSTOM = { - getProvider: () => PROVIDER, - state: { - network: '10', - isCustomNetwork: true, - networkDetails: { isEIP1559Compatible: false }, - providerConfig: { - type: NetworkType.rpc, - chainId: '10', - }, - networkConfigurations: {}, - }, - subscribe: () => undefined, -}; const MOCK_NETWORK_WITHOUT_CHAIN_ID = { - getProvider: () => PROVIDER, + provider: GOERLI_PROVIDER, + blockTracker: { getLatestBlock: () => '0x102833C' }, isCustomNetwork: false, state: { network: '5', @@ -167,7 +182,8 @@ const MOCK_NETWORK_WITHOUT_CHAIN_ID = { subscribe: () => undefined, }; const MOCK_MAINNET_NETWORK = { - getProvider: () => MAINNET_PROVIDER, + provider: MAINNET_PROVIDER, + blockTracker: { getLatestBlock: () => '0x102833C' }, state: { network: '1', isCustomNetwork: false, @@ -181,14 +197,15 @@ const MOCK_MAINNET_NETWORK = { subscribe: () => undefined, }; const MOCK_CUSTOM_NETWORK = { - getProvider: () => MAINNET_PROVIDER, + provider: PALM_PROVIDER, + blockTracker: { getLatestBlock: () => '0xA6EDFC' }, state: { - network: '80001', + network: '11297108109', isCustomNetwork: true, networkDetails: { isEIP1559Compatible: false }, providerConfig: { type: NetworkType.rpc, - chainId: '80001', + chainId: '11297108109', }, networkConfigurations: {}, }, @@ -285,9 +302,15 @@ describe('TransactionController', () => { for (const key in mockFlags) { mockFlags[key] = null; } + // jest + // .spyOn(uuid, 'v1') + // .mockImplementationOnce(() => 'aaaab1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d') + // .mockImplementationOnce(() => 'bbbb1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d'); }); afterEach(() => { + // jest.resetModules(); + jest.clearAllMocks(); sinon.restore(); }); @@ -295,8 +318,10 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); + expect(controller.state).toStrictEqual({ methodData: {}, transactions: [], @@ -307,7 +332,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); expect(controller.config).toStrictEqual({ interval: 15000, @@ -325,7 +351,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { interval: 10 }, ); @@ -344,7 +371,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { interval: 1337 }, ); @@ -363,7 +391,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { interval: 10 }, ); @@ -379,7 +408,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); mockFlags.estimateGasValue = '0x12a05f200'; const from = '0x4579d0ad79bfbdf4539a1ddf5f10b378d724a34c'; @@ -392,7 +422,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); mockFlags.estimateGasValue = '0x12a05f200'; mockFlags.estimateGasError = ESTIMATE_GAS_ERROR; @@ -407,7 +438,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_CUSTOM_NETWORK.state, onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, - getProvider: MOCK_CUSTOM_NETWORK.getProvider, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, }); const from = '0x4579d0ad79bfbdf4539a1ddf5f10b378d724a34c'; const result = await controller.estimateGas({ from, to: from }); @@ -418,7 +450,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_CUSTOM_NETWORK.state, onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, - getProvider: MOCK_CUSTOM_NETWORK.getProvider, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, }); mockFlags.estimateGasError = ESTIMATE_GAS_ERROR; const from = '0x4579d0ad79bfbdf4539a1ddf5f10b378d724a34c'; @@ -430,7 +463,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_CUSTOM_NETWORK.state, onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, - getProvider: MOCK_CUSTOM_NETWORK.getProvider, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, }); mockFlags.getBlockByNumberValue = '0x12a05f200'; @@ -444,7 +478,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_CUSTOM_NETWORK.state, onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, - getProvider: MOCK_CUSTOM_NETWORK.getProvider, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, }); mockFlags.getBlockByNumberValue = '0x12a05f200'; @@ -460,7 +495,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); mockFlags.getBlockByNumberValue = '0x12a05f200'; @@ -475,7 +511,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); mockFlags.getBlockByNumberValue = '0x12a05f200'; @@ -491,7 +528,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); await expect( controller.addTransaction({ from: 'foo' } as any), @@ -502,7 +540,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; await controller.addTransaction({ @@ -530,16 +569,17 @@ describe('TransactionController', () => { const onNetworkStateChange = (listener: (state: NetworkState) => void) => { networkStateChangeListener = listener; }; - const getProvider = sinon.stub().returns(PROVIDER); + const controller = new TransactionController({ getNetworkState, onNetworkStateChange, - getProvider, + provider: GOERLI_PROVIDER, + blockTracker: undefined, }); // switch from Goerli to Mainnet getNetworkState.returns(MOCK_MAINNET_NETWORK.state); - getProvider.returns(MAINNET_PROVIDER); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion networkStateChangeListener!(MOCK_MAINNET_NETWORK.state); @@ -569,18 +609,19 @@ describe('TransactionController', () => { const onNetworkStateChange = (listener: (state: NetworkState) => void) => { networkStateChangeListener = listener; }; - const getProvider = sinon.stub().returns(PROVIDER); + const controller = new TransactionController({ getNetworkState, onNetworkStateChange, - getProvider, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, }); // switch from Goerli to Mainnet - getNetworkState.returns(MOCK_NETWORK_CUSTOM.state); - getProvider.returns(PROVIDER); + getNetworkState.returns(MOCK_CUSTOM_NETWORK.state); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - networkStateChangeListener!(MOCK_NETWORK_CUSTOM.state); + networkStateChangeListener!(MOCK_CUSTOM_NETWORK.state); const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; await controller.addTransaction({ @@ -589,11 +630,11 @@ describe('TransactionController', () => { }); expect(controller.state.transactions[0].transaction.from).toBe(from); expect(controller.state.transactions[0].networkID).toBe( - MOCK_NETWORK_CUSTOM.state.network, + MOCK_CUSTOM_NETWORK.state.network, ); expect(controller.state.transactions[0].chainId).toBe( - MOCK_NETWORK_CUSTOM.state.providerConfig.chainId, + MOCK_CUSTOM_NETWORK.state.providerConfig.chainId, ); expect(controller.state.transactions[0].status).toBe( @@ -605,7 +646,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; const { result } = await controller.addTransaction({ @@ -634,7 +676,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); controller.wipeTransactions(); const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; @@ -651,7 +694,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); controller.wipeTransactions(); controller.state.transactions.push({ @@ -667,11 +711,31 @@ describe('TransactionController', () => { it('should approve custom network transaction', async () => { await new Promise(async (resolve) => { + getNonceLockStub + .mockImplementationOnce(() => { + return { + releaseLock: () => Promise.resolve(), + nextNonce: '0', + }; + }) + .mockImplementationOnce(() => { + return { + releaseLock: () => Promise.resolve(), + nextNonce: '1', + }; + }) + .mockImplementationOnce(() => { + return { + releaseLock: () => Promise.resolve(), + nextNonce: '2', + }; + }); const controller = new TransactionController( { getNetworkState: () => MOCK_CUSTOM_NETWORK.state, onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, - getProvider: MOCK_CUSTOM_NETWORK.getProvider, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -704,7 +768,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: () => { @@ -727,7 +792,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }); mockFlags.estimateGasError = ESTIMATE_GAS_ERROR; const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; @@ -750,7 +816,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_CUSTOM_NETWORK.state, onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, - getProvider: MOCK_CUSTOM_NETWORK.getProvider, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, }); mockFlags.estimateGasError = ESTIMATE_GAS_ERROR; const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; @@ -774,7 +841,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, {}, ); @@ -795,7 +863,8 @@ describe('TransactionController', () => { getNetworkState: () => MOCK_NETWORK_WITHOUT_CHAIN_ID.state as NetworkState, onNetworkStateChange: MOCK_NETWORK_WITHOUT_CHAIN_ID.subscribe, - getProvider: MOCK_NETWORK_WITHOUT_CHAIN_ID.getProvider, + provider: MOCK_NETWORK_WITHOUT_CHAIN_ID.provider, + blockTracker: MOCK_NETWORK_WITHOUT_CHAIN_ID.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -818,7 +887,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -852,7 +922,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -888,7 +959,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -921,7 +993,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -945,7 +1018,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -973,7 +1047,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); controller.wipeTransactions(); expect(controller.state.transactions).toHaveLength(0); @@ -990,7 +1065,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }); controller.wipeTransactions(); expect(controller.state.transactions).toHaveLength(0); @@ -1007,7 +1083,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }); const from = '0x6bf137f335ea1b8f193b8f6ea92561a60d23a207'; controller.wipeTransactions(); @@ -1029,7 +1106,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }); controller.wipeTransactions(); expect(controller.state.transactions).toHaveLength(0); @@ -1046,7 +1124,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }); const from = '0x6bf137f335ea1b8f193b8f6ea92561a60d23a207'; controller.wipeTransactions(); @@ -1072,7 +1151,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }); const from = '0x6bf137f335ea1b8f193b8f6ea92561a60d23a207'; controller.wipeTransactions(); @@ -1099,7 +1179,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }); const from = '0x6bf137f335ea1b8f193b8f6ea92561a60d23a207'; controller.wipeTransactions(); @@ -1128,7 +1209,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); controller.wipeTransactions(); expect(controller.state.transactions).toHaveLength(0); @@ -1143,7 +1225,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }, {}, ); @@ -1163,7 +1246,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_MAINNET_NETWORK.state, onNetworkStateChange: MOCK_MAINNET_NETWORK.subscribe, - getProvider: MOCK_MAINNET_NETWORK.getProvider, + provider: MOCK_MAINNET_NETWORK.provider, + blockTracker: MOCK_MAINNET_NETWORK.blockTracker, }, {}, ); @@ -1182,7 +1266,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -1204,7 +1289,8 @@ describe('TransactionController', () => { const controller = new TransactionController({ getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }); const from = '0xe6509775f3f3614576c0d83f8647752f87cd6659'; const to = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; @@ -1221,7 +1307,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { sign: async (transaction: any) => transaction, @@ -1235,10 +1322,45 @@ describe('TransactionController', () => { to: from, value: '0x0', }); + await controller.approveTransaction(controller.state.transactions[0].id); await controller.speedUpTransaction(controller.state.transactions[0].id); expect(controller.state.transactions).toHaveLength(2); expect(controller.state.transactions[1].transaction.gasPrice).toBe( - '0x5916a6d6', + '0x5916a6d6', // 1.1 * 0x50fd51da + ); + resolve(''); + }); + }); + + it('should use the same nonce when speeding up a transaction', async () => { + await new Promise(async (resolve) => { + const controller = new TransactionController( + { + getNetworkState: () => MOCK_NETWORK.state, + onNetworkStateChange: MOCK_NETWORK.subscribe, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, + }, + { + sign: async (transaction: any) => transaction, + }, + ); + const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; + await controller.addTransaction({ + from, + gas: '0x0', + gasPrice: '0x50fd51da', + to: from, + value: '0x0', + }); + + const originalTransaction = controller.state.transactions[0]; + await controller.approveTransaction(originalTransaction.id); + await controller.speedUpTransaction(originalTransaction.id); + expect(getNonceLockStub).toHaveBeenCalledTimes(1); + expect(controller.state.transactions).toHaveLength(2); + expect(originalTransaction.transaction.nonce).toStrictEqual( + controller.state.transactions[1].transaction.nonce, ); resolve(''); }); @@ -1251,7 +1373,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { interval: 5000, @@ -1283,7 +1406,8 @@ describe('TransactionController', () => { { getNetworkState: () => MOCK_NETWORK.state, onNetworkStateChange: MOCK_NETWORK.subscribe, - getProvider: MOCK_NETWORK.getProvider, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, }, { interval: 5000, @@ -1305,4 +1429,69 @@ describe('TransactionController', () => { resolve(''); }); }); + + it('should increment nonce when adding a new non-cancel non-speedup transaction', async () => { + v1Stub + .mockImplementationOnce(() => 'aaaab1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d') + .mockImplementationOnce(() => 'bbbb1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d'); + await new Promise(async (resolve) => { + getNonceLockStub + .mockImplementationOnce(() => { + return { + releaseLock: () => Promise.resolve(), + nextNonce: '0', + }; + }) + .mockImplementationOnce(() => { + return { + releaseLock: () => Promise.resolve(), + nextNonce: '1', + }; + }) + .mockImplementationOnce(() => { + return { + releaseLock: () => Promise.resolve(), + nextNonce: '2', + }; + }); + + const controller = new TransactionController( + { + getNetworkState: () => MOCK_NETWORK.state, + onNetworkStateChange: MOCK_NETWORK.subscribe, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, + }, + { + sign: async (transaction: any) => transaction, + }, + ); + const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; + await controller.addTransaction({ + from, + gas: '0x0', + gasPrice: '0x50fd51da', + to: from, + value: '0x0', + }); + + const firstTransaction = controller.state.transactions[0]; + await controller.approveTransaction(firstTransaction.id); + expect(getNonceLockStub).toHaveBeenCalledTimes(1); + await controller.addTransaction({ + from, + gas: '0x2', + gasPrice: '0x50fd51da', + to: from, + value: '0x1290', + }); + expect(controller.state.transactions).toHaveLength(2); + const secondTransaction = controller.state.transactions[1]; + await controller.approveTransaction(secondTransaction.id); + + expect(firstTransaction.transaction.nonce).toStrictEqual('0x0'); + expect(secondTransaction.transaction.nonce).toStrictEqual('0x1'); + resolve(''); + }); + }); }); diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 8020fc7605..81db09b38f 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -12,7 +12,11 @@ import { BaseConfig, BaseState, } from '@metamask/base-controller'; -import type { NetworkState, ProviderProxy } from '@metamask/network-controller'; +import type { + NetworkState, + ProviderProxy, + BlockTrackerProxy, +} from '@metamask/network-controller'; import { BNToHex, fractionBN, @@ -23,7 +27,9 @@ import { NetworkType, RPC, } from '@metamask/controller-utils'; +import NonceTracker from 'nonce-tracker'; import { + formatTxForNonceTracker, normalizeTransaction, validateTransaction, handleTransactionFetch, @@ -267,8 +273,12 @@ export class TransactionController extends BaseController< > { private ethQuery: any; + private nonceTracker: NonceTracker; + private registry: any; + private provider: ProviderProxy; + private handle?: ReturnType; private mutex = new Mutex(); @@ -410,7 +420,8 @@ export class TransactionController extends BaseController< * @param options - The controller options. * @param options.getNetworkState - Gets the state of the network controller. * @param options.onNetworkStateChange - Allows subscribing to network controller state changes. - * @param options.getProvider - Returns a provider for the current network. + * @param options.provider - The provider used to create the underlying EthQuery instance. + * @param options.blockTracker - The block tracker used to poll for new blocks data. * @param config - Initial options used to configure this controller. * @param state - Initial state to set on this controller. */ @@ -418,11 +429,13 @@ export class TransactionController extends BaseController< { getNetworkState, onNetworkStateChange, - getProvider, + provider, + blockTracker, }: { getNetworkState: () => NetworkState; onNetworkStateChange: (listener: (state: NetworkState) => void) => void; - getProvider: () => ProviderProxy; + provider: ProviderProxy; + blockTracker: BlockTrackerProxy; }, config?: Partial, state?: Partial, @@ -438,14 +451,34 @@ export class TransactionController extends BaseController< transactions: [], }; this.initialize(); - const provider = getProvider(); + this.provider = provider; this.getNetworkState = getNetworkState; this.ethQuery = new EthQuery(provider); this.registry = new MethodRegistry({ provider }); + this.nonceTracker = new NonceTracker({ + provider, + blockTracker, + getPendingTransactions: (address) => { + return formatTxForNonceTracker( + this.state.transactions.filter( + ({ status, transaction: { from } }) => + status === TransactionStatus.submitted && + (address ? from.toLowerCase() === address.toLowerCase() : true), + ), + ); + }, + getConfirmedTransactions: () => { + return formatTxForNonceTracker( + this.state.transactions.filter( + ({ status }) => status === TransactionStatus.confirmed, + ), + ); + }, + }); + onNetworkStateChange(() => { - const newProvider = getProvider(); - this.ethQuery = new EthQuery(newProvider); - this.registry = new MethodRegistry({ provider: newProvider }); + this.ethQuery = new EthQuery(this.provider); + this.registry = new MethodRegistry({ provider: this.provider }); }); this.poll(); } @@ -625,10 +658,11 @@ export class TransactionController extends BaseController< const { chainId: currentChainId } = providerConfig; const index = transactions.findIndex(({ id }) => transactionID === id); const transactionMeta = transactions[index]; - const { nonce } = transactionMeta.transaction; - + const { + transaction: { nonce, from }, + } = transactionMeta; + let nonceLock; try { - const { from } = transactionMeta.transaction; if (!this.sign) { releaseLock(); this.failTransaction( @@ -644,21 +678,21 @@ export class TransactionController extends BaseController< const chainId = parseInt(currentChainId, undefined); const { approved: status } = TransactionStatus; - - const txNonce = - nonce || - (await query(this.ethQuery, 'getTransactionCount', [from, 'pending'])); + let nonceToUse = nonce; + // if a nonce already exists on the transactionMeta it means this is a speedup or cancel transaction + // so we want to reuse that nonce and hope that it beats the previous attempt to chain. Otherwise use a new locked nonce + if (!nonceToUse) { + nonceLock = await this.nonceTracker.getNonceLock(from); + nonceToUse = nonceLock.nextNonce.toString(16); + } transactionMeta.status = status; - transactionMeta.transaction.nonce = txNonce; + transactionMeta.transaction.nonce = addHexPrefix(nonceToUse); transactionMeta.transaction.chainId = chainId; const baseTxParams = { ...transactionMeta.transaction, gasLimit: transactionMeta.transaction.gas, - chainId, - nonce: txNonce, - status, }; const isEIP1559 = isEIP1559Transaction(transactionMeta.transaction); @@ -698,6 +732,10 @@ export class TransactionController extends BaseController< } catch (error: any) { this.failTransaction(transactionMeta, error); } finally { + // must set transaction to submitted/failed before releasing lock + if (nonceLock) { + nonceLock.releaseLock(); + } releaseLock(); } } diff --git a/packages/transaction-controller/src/utils.test.ts b/packages/transaction-controller/src/utils.test.ts index dca3347f66..eb70e28104 100644 --- a/packages/transaction-controller/src/utils.test.ts +++ b/packages/transaction-controller/src/utils.test.ts @@ -1,9 +1,13 @@ +import type { Transaction as NonceTrackerTransaction } from 'nonce-tracker/dist/NonceTracker'; import { Transaction, GasPriceValue, FeeMarketEIP1559Values, + TransactionStatus, } from './TransactionController'; import * as util from './utils'; +import type { TransactionMeta } from './TransactionController'; +import { formatTxForNonceTracker } from './utils'; const MAX_FEE_PER_GAS = 'maxFeePerGas'; const MAX_PRIORITY_FEE_PER_GAS = 'maxPriorityFeePerGas'; @@ -281,4 +285,58 @@ describe('utils', () => { ).not.toThrow(Error); }); }); + + describe('formatTxForNonceTracker', () => { + it('should return an array of formatted NonceTrackerTransaction objects', () => { + const inputTransactions: TransactionMeta[] = [ + { + id: '1', + time: 123456, + transaction: { + from: '0x123', + gas: '0x100', + value: '0x200', + nonce: '0x1', + }, + status: TransactionStatus.confirmed, + }, + { + id: '2', + time: 123457, + transaction: { + from: '', + gas: '', + value: '', + nonce: '', + }, + status: TransactionStatus.submitted, + }, + ]; + const expectedResult: NonceTrackerTransaction[] = [ + { + status: TransactionStatus.confirmed, + history: [{}], + txParams: { + from: '0x123', + gas: '0x100', + value: '0x200', + nonce: '0x1', + }, + }, + { + status: TransactionStatus.submitted, + history: [{}], + txParams: { + from: '', + gas: '', + value: '', + nonce: '', + }, + }, + ]; + + const result = formatTxForNonceTracker(inputTransactions); + expect(result).toStrictEqual(expectedResult); + }); + }); }); diff --git a/packages/transaction-controller/src/utils.ts b/packages/transaction-controller/src/utils.ts index c6d6560fe7..33aea6ee3d 100644 --- a/packages/transaction-controller/src/utils.ts +++ b/packages/transaction-controller/src/utils.ts @@ -5,12 +5,14 @@ import { handleFetch, isValidHexAddress, } from '@metamask/controller-utils'; +import type { Transaction as NonceTrackerTransaction } from 'nonce-tracker/dist/NonceTracker'; import { Transaction, FetchAllOptions, GasPriceValue, FeeMarketEIP1559Values, } from './TransactionController'; +import type { TransactionMeta } from './TransactionController'; export const ESTIMATE_GAS_ERROR = 'eth_estimateGas rpc method error'; @@ -261,3 +263,31 @@ export function validateMinimumIncrease(proposed: string, min: string) { const errorMsg = `The proposed value: ${proposedDecimal} should meet or exceed the minimum value: ${minDecimal}`; throw new Error(errorMsg); } + +/** + * Helper Method to format transactions for the nonce tracker. + * + * @param transactions - Array of transactionMeta objects that have been prefiltered. + * @returns Array of transactions formatted for the nonce tracker. + */ +export function formatTxForNonceTracker( + transactions: TransactionMeta[], +): NonceTrackerTransaction[] { + return transactions.map( + ({ status, transaction: { from, gas, value, nonce } }) => { + // the only value we care about is the nonce + // but we need to return the other values to satisfy the type + // TODO: refactor nonceTracker to not require this + return { + status, + history: [{}], + txParams: { + from: from ?? '', + gas: gas ?? '', + value: value ?? '', + nonce: nonce ?? '', + }, + }; + }, + ); +} diff --git a/yarn.lock b/yarn.lock index 0d42c75eeb..627be8eda4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2045,6 +2045,7 @@ __metadata: ethereumjs-util: ^7.0.10 ethjs-provider-http: ^0.1.6 jest: ^26.4.2 + nonce-tracker: ^1.1.0 sinon: ^9.2.4 ts-jest: ^26.5.2 typedoc: ^0.22.15 @@ -3189,6 +3190,16 @@ __metadata: languageName: node linkType: hard +"assert@npm:^1.4.1": + version: 1.5.0 + resolution: "assert@npm:1.5.0" + dependencies: + object-assign: ^4.1.1 + util: 0.10.3 + checksum: 9be48435f726029ae7020c5888a3566bf4d617687aab280827f2e4029644b6515a9519ea10d018b342147c02faf73d9e9419e780e8937b3786ee4945a0ca71e5 + languageName: node + linkType: hard + "assert@npm:^2.0.0": version: 2.0.0 resolution: "assert@npm:2.0.0" @@ -3286,6 +3297,13 @@ __metadata: languageName: node linkType: hard +"await-semaphore@npm:^0.1.3": + version: 0.1.3 + resolution: "await-semaphore@npm:0.1.3" + checksum: 334c86541e446378dd832168de431327a77146f70cd80b57c99cd483ce5996e3bfdadea9d795e36f0b4faacb5121f5f7a99d94297ac2bdafbc690e5b0aa5cc32 + languageName: node + linkType: hard + "aws-sign2@npm:~0.7.0": version: 0.7.0 resolution: "aws-sign2@npm:0.7.0" @@ -5541,6 +5559,18 @@ __metadata: languageName: node linkType: hard +"ethjs-query@npm:^0.3.8": + version: 0.3.8 + resolution: "ethjs-query@npm:0.3.8" + dependencies: + babel-runtime: ^6.26.0 + ethjs-format: 0.2.7 + ethjs-rpc: 0.2.0 + promise-to-callback: ^1.0.0 + checksum: 6673167101e793dfdbb212f3ee2e7449a7c1eb7f4d72aba04a1c504eb36d9148f18c42a702839ebce7534709d821f32adcca84c56b7900788b2875cba8a371f1 + languageName: node + linkType: hard + "ethjs-rpc@npm:0.2.0": version: 0.2.0 resolution: "ethjs-rpc@npm:0.2.0" @@ -6742,6 +6772,13 @@ __metadata: languageName: node linkType: hard +"inherits@npm:2.0.1": + version: 2.0.1 + resolution: "inherits@npm:2.0.1" + checksum: 6536b9377296d4ce8ee89c5c543cb75030934e61af42dba98a428e7d026938c5985ea4d1e3b87743a5b834f40ed1187f89c2d7479e9d59e41d2d1051aefba07b + languageName: node + linkType: hard + "internal-slot@npm:^1.0.3": version: 1.0.3 resolution: "internal-slot@npm:1.0.3" @@ -9029,6 +9066,17 @@ __metadata: languageName: node linkType: hard +"nonce-tracker@npm:^1.1.0": + version: 1.1.0 + resolution: "nonce-tracker@npm:1.1.0" + dependencies: + assert: ^1.4.1 + await-semaphore: ^0.1.3 + ethjs-query: ^0.3.8 + checksum: fbed4eac51a5df3922a9ee93f2d40f700d3c86e03ac02acc5c7983e9fd5c9d97278f1f5d8422f2dad0f0108c09a2d880145906cae19604717cc898d385bbdff9 + languageName: node + linkType: hard + "nopt@npm:^5.0.0": version: 5.0.0 resolution: "nopt@npm:5.0.0" @@ -9148,7 +9196,7 @@ __metadata: languageName: node linkType: hard -"object-assign@npm:^4.1.0": +"object-assign@npm:^4.1.0, object-assign@npm:^4.1.1": version: 4.1.1 resolution: "object-assign@npm:4.1.1" checksum: fcc6e4ea8c7fe48abfbb552578b1c53e0d194086e2e6bbbf59e0a536381a292f39943c6e9628af05b5528aa5e3318bb30d6b2e53cadaf5b8fe9e12c4b69af23f @@ -11835,6 +11883,15 @@ __metadata: languageName: node linkType: hard +"util@npm:0.10.3": + version: 0.10.3 + resolution: "util@npm:0.10.3" + dependencies: + inherits: 2.0.1 + checksum: bd800f5d237a82caddb61723a6cbe45297d25dd258651a31335a4d5d981fd033cb4771f82db3d5d59b582b187cb69cfe727dc6f4d8d7826f686ee6c07ce611e0 + languageName: node + linkType: hard + "util@npm:^0.12.0": version: 0.12.4 resolution: "util@npm:0.12.4" From 4abd6312df7c97d908978b0da81462e1507552c3 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 6 Apr 2023 13:29:48 -0500 Subject: [PATCH 2/7] cleanup --- .../src/TransactionController.test.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 97dbc8cbeb..05558f2944 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -1,6 +1,5 @@ import * as sinon from 'sinon'; import HttpProvider from 'ethjs-provider-http'; -// import uuid from 'uuid'; import { NetworksChainId, NetworkType } from '@metamask/controller-utils'; import type { NetworkState } from '@metamask/network-controller'; import { ESTIMATE_GAS_ERROR } from './utils'; @@ -150,7 +149,6 @@ const GOERLI_PROVIDER = new HttpProvider( const MAINNET_PROVIDER = new HttpProvider( 'https://mainnet.infura.io/v3/341eacb578dd44a1a049cbc5f6fd4035', ); -// const PALM_PROVIDER = new HttpProvider('https://mainnet.optimism.io'); const PALM_PROVIDER = new HttpProvider( 'https://palm-mainnet.infura.io/v3/3a961d6501e54add9a41aa53f15de99b', ); @@ -302,14 +300,9 @@ describe('TransactionController', () => { for (const key in mockFlags) { mockFlags[key] = null; } - // jest - // .spyOn(uuid, 'v1') - // .mockImplementationOnce(() => 'aaaab1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d') - // .mockImplementationOnce(() => 'bbbb1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d'); }); afterEach(() => { - // jest.resetModules(); jest.clearAllMocks(); sinon.restore(); }); From 2138d654c19b87a7e9baf2221fad057ed3434975 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 6 Apr 2023 13:53:44 -0500 Subject: [PATCH 3/7] address feedback --- .../src/TransactionController.ts | 30 +++++------- .../transaction-controller/src/utils.test.ts | 47 +++++++++++-------- packages/transaction-controller/src/utils.ts | 19 ++++++-- 3 files changed, 54 insertions(+), 42 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 81db09b38f..aa1ac06823 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -29,7 +29,7 @@ import { } from '@metamask/controller-utils'; import NonceTracker from 'nonce-tracker'; import { - formatTxForNonceTracker, + getAndFormatTransactionsForNonceTracker, normalizeTransaction, validateTransaction, handleTransactionFetch, @@ -458,22 +458,18 @@ export class TransactionController extends BaseController< this.nonceTracker = new NonceTracker({ provider, blockTracker, - getPendingTransactions: (address) => { - return formatTxForNonceTracker( - this.state.transactions.filter( - ({ status, transaction: { from } }) => - status === TransactionStatus.submitted && - (address ? from.toLowerCase() === address.toLowerCase() : true), - ), - ); - }, - getConfirmedTransactions: () => { - return formatTxForNonceTracker( - this.state.transactions.filter( - ({ status }) => status === TransactionStatus.confirmed, - ), - ); - }, + getPendingTransactions: (address) => + getAndFormatTransactionsForNonceTracker( + address, + TransactionStatus.submitted, + this.state.transactions, + ), + getConfirmedTransactions: (address) => + getAndFormatTransactionsForNonceTracker( + address, + TransactionStatus.confirmed, + this.state.transactions, + ), }); onNetworkStateChange(() => { diff --git a/packages/transaction-controller/src/utils.test.ts b/packages/transaction-controller/src/utils.test.ts index eb70e28104..df6a93babb 100644 --- a/packages/transaction-controller/src/utils.test.ts +++ b/packages/transaction-controller/src/utils.test.ts @@ -7,7 +7,7 @@ import { } from './TransactionController'; import * as util from './utils'; import type { TransactionMeta } from './TransactionController'; -import { formatTxForNonceTracker } from './utils'; +import { getAndFormatTransactionsForNonceTracker } from './utils'; const MAX_FEE_PER_GAS = 'maxFeePerGas'; const MAX_PRIORITY_FEE_PER_GAS = 'maxPriorityFeePerGas'; @@ -286,14 +286,15 @@ describe('utils', () => { }); }); - describe('formatTxForNonceTracker', () => { - it('should return an array of formatted NonceTrackerTransaction objects', () => { + describe('getAndFormatTransactionsForNonceTracker', () => { + it('should return an array of formatted NonceTrackerTransaction objects filtered by fromAddress and transactionStatus', () => { + const fromAddress = '0x123'; const inputTransactions: TransactionMeta[] = [ { id: '1', time: 123456, transaction: { - from: '0x123', + from: fromAddress, gas: '0x100', value: '0x200', nonce: '0x1', @@ -304,38 +305,44 @@ describe('utils', () => { id: '2', time: 123457, transaction: { - from: '', - gas: '', - value: '', - nonce: '', + from: '0x124', + gas: '0x101', + value: '0x201', + nonce: '0x2', }, status: TransactionStatus.submitted, }, + { + id: '3', + time: 123458, + transaction: { + from: fromAddress, + gas: '0x102', + value: '0x202', + nonce: '0x3', + }, + status: TransactionStatus.approved, + }, ]; + const expectedResult: NonceTrackerTransaction[] = [ { status: TransactionStatus.confirmed, history: [{}], txParams: { - from: '0x123', + from: fromAddress, gas: '0x100', value: '0x200', nonce: '0x1', }, }, - { - status: TransactionStatus.submitted, - history: [{}], - txParams: { - from: '', - gas: '', - value: '', - nonce: '', - }, - }, ]; - const result = formatTxForNonceTracker(inputTransactions); + const result = getAndFormatTransactionsForNonceTracker( + fromAddress, + TransactionStatus.confirmed, + inputTransactions, + ); expect(result).toStrictEqual(expectedResult); }); }); diff --git a/packages/transaction-controller/src/utils.ts b/packages/transaction-controller/src/utils.ts index 33aea6ee3d..043b182793 100644 --- a/packages/transaction-controller/src/utils.ts +++ b/packages/transaction-controller/src/utils.ts @@ -11,6 +11,7 @@ import { FetchAllOptions, GasPriceValue, FeeMarketEIP1559Values, + TransactionStatus, } from './TransactionController'; import type { TransactionMeta } from './TransactionController'; @@ -267,14 +268,23 @@ export function validateMinimumIncrease(proposed: string, min: string) { /** * Helper Method to format transactions for the nonce tracker. * + * @param fromAddress - Address of the account from which the transactions to filter from are sent. + * @param transactionStatus - Status of the transactions for which to filter. * @param transactions - Array of transactionMeta objects that have been prefiltered. * @returns Array of transactions formatted for the nonce tracker. */ -export function formatTxForNonceTracker( +export function getAndFormatTransactionsForNonceTracker( + fromAddress: string, + transactionStatus: TransactionStatus, transactions: TransactionMeta[], ): NonceTrackerTransaction[] { - return transactions.map( - ({ status, transaction: { from, gas, value, nonce } }) => { + return transactions + .filter( + ({ status, transaction: { from } }) => + status === transactionStatus && + from.toLowerCase() === fromAddress.toLowerCase(), + ) + .map(({ status, transaction: { from, gas, value, nonce } }) => { // the only value we care about is the nonce // but we need to return the other values to satisfy the type // TODO: refactor nonceTracker to not require this @@ -288,6 +298,5 @@ export function formatTxForNonceTracker( nonce: nonce ?? '', }, }; - }, - ); + }); } From c73cf692c082a96ef5c4672361e01481ea27a577 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 6 Apr 2023 13:57:56 -0500 Subject: [PATCH 4/7] update comment --- packages/transaction-controller/src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/transaction-controller/src/utils.ts b/packages/transaction-controller/src/utils.ts index 043b182793..f8dc8e8a5f 100644 --- a/packages/transaction-controller/src/utils.ts +++ b/packages/transaction-controller/src/utils.ts @@ -266,7 +266,7 @@ export function validateMinimumIncrease(proposed: string, min: string) { } /** - * Helper Method to format transactions for the nonce tracker. + * Helper function to filter and format transactions for the nonce tracker. * * @param fromAddress - Address of the account from which the transactions to filter from are sent. * @param transactionStatus - Status of the transactions for which to filter. From 9f2662e80198d97a56bed9e82a1933b614d0c4ac Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 10 Apr 2023 13:33:46 -0500 Subject: [PATCH 5/7] modify mocking strategy --- .../src/TransactionController.test.ts | 220 ++++++++---------- 1 file changed, 94 insertions(+), 126 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 05558f2944..b53c601d4c 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -1,5 +1,6 @@ import * as sinon from 'sinon'; import HttpProvider from 'ethjs-provider-http'; +import NonceTracker from 'nonce-tracker'; import { NetworksChainId, NetworkType } from '@metamask/controller-utils'; import type { NetworkState } from '@metamask/network-controller'; import { ESTIMATE_GAS_ERROR } from './utils'; @@ -96,23 +97,6 @@ jest.mock('eth-query', () => }), ); -const getNonceLockStub = jest.fn().mockImplementation(() => { - return { - releaseLock: () => Promise.resolve(), - nextNonce: '0', - }; -}); - -jest.mock('nonce-tracker', () => { - return jest.fn().mockImplementation(() => { - return { - getGlobalLock: () => Promise.resolve(), - getNonceLock: getNonceLockStub, - releaseLock: () => Promise.resolve(), - }; - }); -}); - /** * Create a mock implementation of `fetch` that always returns the same data. * @@ -702,60 +686,6 @@ describe('TransactionController', () => { expect(controller.state.transactions).toHaveLength(0); }); - it('should approve custom network transaction', async () => { - await new Promise(async (resolve) => { - getNonceLockStub - .mockImplementationOnce(() => { - return { - releaseLock: () => Promise.resolve(), - nextNonce: '0', - }; - }) - .mockImplementationOnce(() => { - return { - releaseLock: () => Promise.resolve(), - nextNonce: '1', - }; - }) - .mockImplementationOnce(() => { - return { - releaseLock: () => Promise.resolve(), - nextNonce: '2', - }; - }); - const controller = new TransactionController( - { - getNetworkState: () => MOCK_CUSTOM_NETWORK.state, - onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, - provider: MOCK_CUSTOM_NETWORK.provider, - blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, - }, - { - sign: async (transaction: any) => transaction, - }, - ); - const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; - await controller.addTransaction({ - from, - gas: '0x0', - gasPrice: '0x0', - to: from, - value: '0x0', - }); - - controller.hub.once( - `${controller.state.transactions[0].id}:finished`, - () => { - const { transaction, status } = controller.state.transactions[0]; - expect(transaction.from).toBe(from); - expect(status).toBe(TransactionStatus.submitted); - resolve(''); - }, - ); - controller.approveTransaction(controller.state.transactions[0].id); - }); - }); - it('should fail to approve an invalid transaction', async () => { const controller = new TransactionController( { @@ -1325,40 +1255,6 @@ describe('TransactionController', () => { }); }); - it('should use the same nonce when speeding up a transaction', async () => { - await new Promise(async (resolve) => { - const controller = new TransactionController( - { - getNetworkState: () => MOCK_NETWORK.state, - onNetworkStateChange: MOCK_NETWORK.subscribe, - provider: MOCK_NETWORK.provider, - blockTracker: MOCK_NETWORK.blockTracker, - }, - { - sign: async (transaction: any) => transaction, - }, - ); - const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; - await controller.addTransaction({ - from, - gas: '0x0', - gasPrice: '0x50fd51da', - to: from, - value: '0x0', - }); - - const originalTransaction = controller.state.transactions[0]; - await controller.approveTransaction(originalTransaction.id); - await controller.speedUpTransaction(originalTransaction.id); - expect(getNonceLockStub).toHaveBeenCalledTimes(1); - expect(controller.state.transactions).toHaveLength(2); - expect(originalTransaction.transaction.nonce).toStrictEqual( - controller.state.transactions[1].transaction.nonce, - ); - resolve(''); - }); - }); - it('should limit tx state to a length of 2', async () => { await new Promise(async (resolve) => { mockFetchWithDynamicResponse(MOCK_FETCH_TX_HISTORY_DATA_OK); @@ -1427,27 +1323,8 @@ describe('TransactionController', () => { v1Stub .mockImplementationOnce(() => 'aaaab1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d') .mockImplementationOnce(() => 'bbbb1deb4d-3b7d-4bad-9bdd-2b0d7b3dcb6d'); - await new Promise(async (resolve) => { - getNonceLockStub - .mockImplementationOnce(() => { - return { - releaseLock: () => Promise.resolve(), - nextNonce: '0', - }; - }) - .mockImplementationOnce(() => { - return { - releaseLock: () => Promise.resolve(), - nextNonce: '1', - }; - }) - .mockImplementationOnce(() => { - return { - releaseLock: () => Promise.resolve(), - nextNonce: '2', - }; - }); + await new Promise(async (resolve) => { const controller = new TransactionController( { getNetworkState: () => MOCK_NETWORK.state, @@ -1470,7 +1347,6 @@ describe('TransactionController', () => { const firstTransaction = controller.state.transactions[0]; await controller.approveTransaction(firstTransaction.id); - expect(getNonceLockStub).toHaveBeenCalledTimes(1); await controller.addTransaction({ from, gas: '0x2', @@ -1487,4 +1363,96 @@ describe('TransactionController', () => { resolve(''); }); }); + + describe('mocked nonce-tracker tests', () => { + let getNonceLockSpy: jest.Mock; + let originalGetNonceLock: any; + + beforeEach(async () => { + originalGetNonceLock = NonceTracker.prototype.getNonceLock; + + getNonceLockSpy = jest.fn().mockResolvedValue({ + nextNonce: '0', + releaseLock: () => Promise.resolve(), + }); + + NonceTracker.prototype.getNonceLock = getNonceLockSpy; + }); + + afterEach(() => { + NonceTracker.prototype.getNonceLock = originalGetNonceLock; + }); + + it('should approve custom network transaction', async () => { + await new Promise(async (resolve) => { + const controller = new TransactionController( + { + getNetworkState: () => MOCK_CUSTOM_NETWORK.state, + onNetworkStateChange: MOCK_CUSTOM_NETWORK.subscribe, + provider: MOCK_CUSTOM_NETWORK.provider, + blockTracker: MOCK_CUSTOM_NETWORK.blockTracker, + }, + { + sign: async (transaction: any) => transaction, + }, + ); + const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; + await controller.addTransaction({ + from, + gas: '0x0', + gasPrice: '0x0', + to: from, + value: '0x0', + }); + + controller.hub.once( + `${controller.state.transactions[0].id}:finished`, + () => { + const { transaction, status } = controller.state.transactions[0]; + expect(transaction.from).toBe(from); + expect(getNonceLockSpy).toHaveBeenCalledTimes(1); + expect(status).toBe(TransactionStatus.submitted); + resolve(''); + }, + ); + await controller.approveTransaction( + controller.state.transactions[0].id, + ); + }); + }); + + it('should use the same nonce when speeding up a transaction', async () => { + await new Promise(async (resolve) => { + const controller = new TransactionController( + { + getNetworkState: () => MOCK_NETWORK.state, + onNetworkStateChange: MOCK_NETWORK.subscribe, + provider: MOCK_NETWORK.provider, + blockTracker: MOCK_NETWORK.blockTracker, + }, + { + sign: async (transaction: any) => transaction, + }, + ); + const from = '0xc38bf1ad06ef69f0c04e29dbeb4152b4175f0a8d'; + await controller.addTransaction({ + from, + gas: '0x0', + gasPrice: '0x50fd51da', + to: from, + value: '0x0', + }); + + const originalTransaction = controller.state.transactions[0]; + await controller.approveTransaction(originalTransaction.id); + await controller.speedUpTransaction(originalTransaction.id); + expect(getNonceLockSpy).toHaveBeenCalledTimes(1); + expect(controller.state.transactions).toHaveLength(2); + expect(originalTransaction.transaction.nonce).toStrictEqual( + controller.state.transactions[1].transaction.nonce, + ); + resolve(''); + }); + }); + }); }); From 96c406dc6da948c6718bf2f4b011386f2b140e66 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 10 Apr 2023 14:47:33 -0500 Subject: [PATCH 6/7] cleanup --- .../src/TransactionController.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index b53c601d4c..a05d5b6d2f 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -1364,15 +1364,16 @@ describe('TransactionController', () => { }); }); - describe('mocked nonce-tracker tests', () => { + describe('NonceTracker integration', () => { let getNonceLockSpy: jest.Mock; let originalGetNonceLock: any; + const testNonce = 12; beforeEach(async () => { originalGetNonceLock = NonceTracker.prototype.getNonceLock; getNonceLockSpy = jest.fn().mockResolvedValue({ - nextNonce: '0', + nextNonce: testNonce, releaseLock: () => Promise.resolve(), }); @@ -1383,7 +1384,7 @@ describe('TransactionController', () => { NonceTracker.prototype.getNonceLock = originalGetNonceLock; }); - it('should approve custom network transaction', async () => { + it('should submit transaction with nonce from NonceTracker', async () => { await new Promise(async (resolve) => { const controller = new TransactionController( { @@ -1410,14 +1411,13 @@ describe('TransactionController', () => { () => { const { transaction, status } = controller.state.transactions[0]; expect(transaction.from).toBe(from); + expect(transaction.nonce).toBe(`0x${testNonce.toString(16)}`); expect(getNonceLockSpy).toHaveBeenCalledTimes(1); expect(status).toBe(TransactionStatus.submitted); resolve(''); }, ); - await controller.approveTransaction( - controller.state.transactions[0].id, - ); + controller.approveTransaction(controller.state.transactions[0].id); }); }); From c2961a47c07292fdfb546a455f2791f839d72cea Mon Sep 17 00:00:00 2001 From: Alex Date: Tue, 11 Apr 2023 10:32:57 -0500 Subject: [PATCH 7/7] address feedback --- packages/transaction-controller/src/TransactionController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index aa1ac06823..dfa17643fc 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -679,11 +679,11 @@ export class TransactionController extends BaseController< // so we want to reuse that nonce and hope that it beats the previous attempt to chain. Otherwise use a new locked nonce if (!nonceToUse) { nonceLock = await this.nonceTracker.getNonceLock(from); - nonceToUse = nonceLock.nextNonce.toString(16); + nonceToUse = addHexPrefix(nonceLock.nextNonce.toString(16)); } transactionMeta.status = status; - transactionMeta.transaction.nonce = addHexPrefix(nonceToUse); + transactionMeta.transaction.nonce = nonceToUse; transactionMeta.transaction.chainId = chainId; const baseTxParams = {