From b87c1b85262e5faa101fe8d0562523b8fe2c1c00 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 17 Apr 2023 15:45:01 -0230 Subject: [PATCH 1/2] Make `setProviderType` async (#18604) The network controller method `setProviderType` is now async, and the async operation `_setProviderConfig` called at the end of the method is now awaited. Because the only async operation was the last step, this should have no impact upon the flow of execution. The only functional change is that now any callers have the option of waiting until the network switch operation has completed. One such change was made, in the `switch-ethereum-chain` middleware. As a result, an error thrown while the network is switching will now be thrown in this middleware and returned to the dapp as an internal error. Relates to https://github.com/MetaMask/metamask-extension/issues/18587 --- app/scripts/controllers/detect-tokens.test.js | 12 +- .../network/network-controller.test.ts | 197 ++++-------------- .../controllers/network/network-controller.ts | 4 +- .../handlers/switch-ethereum-chain.js | 2 +- 4 files changed, 49 insertions(+), 166 deletions(-) diff --git a/app/scripts/controllers/detect-tokens.test.js b/app/scripts/controllers/detect-tokens.test.js index 10db93359403..d5f6eb7bda6a 100644 --- a/app/scripts/controllers/detect-tokens.test.js +++ b/app/scripts/controllers/detect-tokens.test.js @@ -278,7 +278,7 @@ describe('DetectTokensController', function () { it('should be called on every polling period', async function () { const clock = sandbox.useFakeTimers(); - network.setProviderType(NETWORK_TYPES.MAINNET); + await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ preferences, network, @@ -304,7 +304,7 @@ describe('DetectTokensController', function () { it('should not check and add tokens while on unsupported networks', async function () { sandbox.useFakeTimers(); - network.setProviderType(NETWORK_TYPES.SEPOLIA); + await network.setProviderType(NETWORK_TYPES.SEPOLIA); const tokenListMessengerSepolia = new ControllerMessenger().getRestricted({ name: 'TokenListController', }); @@ -337,7 +337,7 @@ describe('DetectTokensController', function () { it('should skip adding tokens listed in ignoredTokens array', async function () { sandbox.useFakeTimers(); - network.setProviderType(NETWORK_TYPES.MAINNET); + await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ preferences, network, @@ -388,7 +388,7 @@ describe('DetectTokensController', function () { it('should check and add tokens while on supported networks', async function () { sandbox.useFakeTimers(); - network.setProviderType(NETWORK_TYPES.MAINNET); + await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ preferences, network, @@ -483,7 +483,7 @@ describe('DetectTokensController', function () { it('should not trigger detect new tokens when not unlocked', async function () { const clock = sandbox.useFakeTimers(); - network.setProviderType(NETWORK_TYPES.MAINNET); + await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ preferences, network, @@ -504,7 +504,7 @@ describe('DetectTokensController', function () { it('should not trigger detect new tokens when not open', async function () { const clock = sandbox.useFakeTimers(); - network.setProviderType(NETWORK_TYPES.MAINNET); + await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ preferences, network, diff --git a/app/scripts/controllers/network/network-controller.test.ts b/app/scripts/controllers/network/network-controller.test.ts index 068a85859e88..c5d5975e1e00 100644 --- a/app/scripts/controllers/network/network-controller.test.ts +++ b/app/scripts/controllers/network/network-controller.test.ts @@ -1135,7 +1135,7 @@ describe('NetworkController', () => { }); expect(oldChainIdResult).toBe('0x1337'); - controller.setProviderType(networkType); + await controller.setProviderType(networkType); const promisifiedSendAsync2 = promisify(provider.sendAsync).bind( provider, ); @@ -2652,21 +2652,9 @@ describe('NetworkController', () => { network1.mockEssentialRpcCalls({ eth_getBlockByNumber: { beforeCompleting: async () => { - await waitForPublishedEvents({ - messenger: unrestrictedMessenger, - eventType: NetworkControllerEventType.NetworkDidChange, - operation: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setProviderType( - anotherNetwork.networkType, - ); - }, - }); - }, - }); + await controller.setProviderType( + anotherNetwork.networkType, + ); }, }, }); @@ -3513,13 +3501,7 @@ describe('NetworkController', () => { { response: SUCCESSFUL_NET_VERSION_RESPONSE, beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, ], @@ -3618,13 +3600,7 @@ describe('NetworkController', () => { result: '111', }, beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, }); @@ -3675,13 +3651,7 @@ describe('NetworkController', () => { net_version: { response: SUCCESSFUL_NET_VERSION_RESPONSE, beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, }); @@ -3738,13 +3708,7 @@ describe('NetworkController', () => { network1.mockEssentialRpcCalls({ net_version: { beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, }); @@ -3841,13 +3805,7 @@ describe('NetworkController', () => { }, }, beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, ], @@ -3904,13 +3862,7 @@ describe('NetworkController', () => { network1.mockEssentialRpcCalls({ eth_getBlockByNumber: { beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, net_version: { @@ -3965,13 +3917,7 @@ describe('NetworkController', () => { latestBlock: POST_1559_BLOCK, eth_getBlockByNumber: { beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, }); @@ -4028,13 +3974,7 @@ describe('NetworkController', () => { network1.mockEssentialRpcCalls({ eth_getBlockByNumber: { beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); }, }, }); @@ -4629,7 +4569,7 @@ describe('NetworkController', () => { }); network.mockEssentialRpcCalls(); - controller.setProviderType(networkType); + await controller.setProviderType(networkType); expect(controller.store.getState().provider).toStrictEqual({ type: networkType, @@ -4662,6 +4602,8 @@ describe('NetworkController', () => { messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkWillChange, operation: () => { + // Intentionally not awaited because we're capturing an event + // emitted partway through the operation controller.setProviderType(networkType); }, }); @@ -4714,6 +4656,8 @@ describe('NetworkController', () => { // happens before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we're checking the state + // partway through the operation controller.setProviderType(networkType); }, }); @@ -4767,6 +4711,8 @@ describe('NetworkController', () => { // happens before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we're checking the state + // partway through the operation controller.setProviderType(networkType); }, }); @@ -4786,7 +4732,7 @@ describe('NetworkController', () => { }); network.mockEssentialRpcCalls(); - controller.setProviderType(networkType); + await controller.setProviderType(networkType); const { provider } = controller.getProviderAndBlockTracker(); assert(provider, 'Provider is somehow unset'); @@ -4813,7 +4759,7 @@ describe('NetworkController', () => { const { provider: providerBefore } = controller.getProviderAndBlockTracker(); - controller.setProviderType(networkType); + await controller.setProviderType(networkType); const { provider: providerAfter } = controller.getProviderAndBlockTracker(); @@ -4836,8 +4782,8 @@ describe('NetworkController', () => { const networkDidChange = await waitForPublishedEvents({ messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkDidChange, - operation: () => { - controller.setProviderType(networkType); + operation: async () => { + await controller.setProviderType(networkType); }, }); @@ -4872,7 +4818,7 @@ describe('NetworkController', () => { eventType: NetworkControllerEventType.InfuraIsBlocked, }); - controller.setProviderType(networkType); + await controller.setProviderType(networkType); expect(await promiseForNoInfuraIsUnblockedEvents).toBeTruthy(); expect(await promiseForInfuraIsBlocked).toBeTruthy(); @@ -4891,13 +4837,7 @@ describe('NetworkController', () => { latestBlock: BLOCK, }); - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setProviderType(networkType); - }, - }); + await controller.setProviderType(networkType); expect(controller.store.getState().networkStatus).toBe('available'); }); @@ -4921,16 +4861,7 @@ describe('NetworkController', () => { latestBlock: POST_1559_BLOCK, }); - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - // setProviderType clears networkDetails first, and then updates - // it to what we expect it to be - count: 2, - operation: () => { - controller.setProviderType(networkType); - }, - }); + await controller.setProviderType(networkType); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { @@ -4946,7 +4877,7 @@ describe('NetworkController', () => { describe('given a type of "rpc"', () => { it('throws', async () => { await withController(async ({ controller }) => { - expect(() => controller.setProviderType('rpc')).toThrow( + await expect(() => controller.setProviderType('rpc')).rejects.toThrow( new Error( 'NetworkController - cannot call "setProviderType" with type "rpc". Use "setActiveNetwork"', ), @@ -4958,7 +4889,9 @@ describe('NetworkController', () => { describe('given an invalid Infura network name', () => { it('throws', async () => { await withController(async ({ controller }) => { - expect(() => controller.setProviderType('sadlflaksdj')).toThrow( + await expect(() => + controller.setProviderType('sadlflaksdj'), + ).rejects.toThrow( new Error('Unknown Infura provider type "sadlflaksdj".'), ); }); @@ -6393,12 +6326,7 @@ describe('NetworkController', () => { currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); expect(controller.store.getState().provider).toStrictEqual({ type: 'goerli', rpcUrl: '', @@ -6466,12 +6394,7 @@ describe('NetworkController', () => { }); currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); await waitForLookupNetworkToComplete({ controller, @@ -6518,12 +6441,7 @@ describe('NetworkController', () => { currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); expect(controller.store.getState().networkStatus).toBe('available'); await waitForLookupNetworkToComplete({ @@ -6579,12 +6497,7 @@ describe('NetworkController', () => { }); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { 1559: true, @@ -6645,12 +6558,7 @@ describe('NetworkController', () => { }); currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); await waitForLookupNetworkToComplete({ controller, @@ -6700,12 +6608,7 @@ describe('NetworkController', () => { }); currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); const { provider: providerBefore } = controller.getProviderAndBlockTracker(); @@ -6754,12 +6657,7 @@ describe('NetworkController', () => { currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); await waitForLookupNetworkToComplete({ controller, @@ -6809,12 +6707,7 @@ describe('NetworkController', () => { currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); await waitForLookupNetworkToComplete({ controller, @@ -6869,12 +6762,7 @@ describe('NetworkController', () => { }, }); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); expect(controller.store.getState().networkStatus).toBe('available'); await waitForLookupNetworkToComplete({ @@ -6919,12 +6807,7 @@ describe('NetworkController', () => { latestBlock: POST_1559_BLOCK, }); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setProviderType('goerli'); - }, - }); + await controller.setProviderType('goerli'); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { 1559: false, diff --git a/app/scripts/controllers/network/network-controller.ts b/app/scripts/controllers/network/network-controller.ts index 2cfdadd2e40a..783e368a164a 100644 --- a/app/scripts/controllers/network/network-controller.ts +++ b/app/scripts/controllers/network/network-controller.ts @@ -729,7 +729,7 @@ export class NetworkController extends EventEmitter { * @throws if the `type` is "rpc" or if it is not a known Infura-supported * network. */ - setProviderType(type: string): void { + async setProviderType(type: string) { assert.notStrictEqual( type, NETWORK_TYPES.RPC, @@ -740,7 +740,7 @@ export class NetworkController extends EventEmitter { `Unknown Infura provider type "${type}".`, ); const network = BUILT_IN_INFURA_NETWORKS[type]; - this.#setProviderConfig({ + await this.#setProviderConfig({ type, rpcUrl: '', chainId: network.chainId, diff --git a/app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js b/app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js index 81c6887e048d..faeded0727b4 100644 --- a/app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js +++ b/app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js @@ -113,7 +113,7 @@ async function switchEthereumChainHandler( approvedRequestData.type !== NETWORK_TYPES.LOCALHOST && approvedRequestData.type !== NETWORK_TYPES.LINEA_TESTNET ) { - setProviderType(approvedRequestData.type); + await setProviderType(approvedRequestData.type); } else { await setActiveNetwork(approvedRequestData.id); } From e5835ac45900d00bb8b619dad79695c9559e0a35 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 17 Apr 2023 15:45:22 -0230 Subject: [PATCH 2/2] Make `resetConnection` async (#18601) The network controller method `resetConnection` is now async. This has no functional impact. Relates to https://github.com/MetaMask/metamask-extension/issues/18587 --- .../network/network-controller.test.ts | 71 +++++++------------ .../controllers/network/network-controller.ts | 4 +- 2 files changed, 29 insertions(+), 46 deletions(-) diff --git a/app/scripts/controllers/network/network-controller.test.ts b/app/scripts/controllers/network/network-controller.test.ts index c5d5975e1e00..503f23a72579 100644 --- a/app/scripts/controllers/network/network-controller.test.ts +++ b/app/scripts/controllers/network/network-controller.test.ts @@ -4925,6 +4925,8 @@ describe('NetworkController', () => { messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkWillChange, operation: () => { + // Intentionally not awaited because we want to capture an + // event emitted partway throught this operation controller.resetConnection(); }, }); @@ -4965,6 +4967,8 @@ describe('NetworkController', () => { // happens before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we want to capture a + // state change made partway through the operation controller.resetConnection(); }, }); @@ -5007,6 +5011,8 @@ describe('NetworkController', () => { // happens before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we want to check state + // partway through the operation controller.resetConnection(); }, }); @@ -5034,7 +5040,7 @@ describe('NetworkController', () => { async ({ controller, network }) => { network.mockEssentialRpcCalls(); - controller.resetConnection(); + await controller.resetConnection(); const { provider } = controller.getProviderAndBlockTracker(); assert(provider, 'Provider is somehow unset'); @@ -5073,7 +5079,7 @@ describe('NetworkController', () => { const { provider: providerBefore } = controller.getProviderAndBlockTracker(); - controller.resetConnection(); + await controller.resetConnection(); const { provider: providerAfter } = controller.getProviderAndBlockTracker(); @@ -5104,8 +5110,8 @@ describe('NetworkController', () => { const networkDidChange = await waitForPublishedEvents({ messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkDidChange, - operation: () => { - controller.resetConnection(); + operation: async () => { + await controller.resetConnection(); }, }); @@ -5147,7 +5153,7 @@ describe('NetworkController', () => { eventType: NetworkControllerEventType.InfuraIsBlocked, }); - controller.resetConnection(); + await controller.resetConnection(); expect(await promiseForNoInfuraIsUnblockedEvents).toBeTruthy(); expect(await promiseForInfuraIsBlocked).toBeTruthy(); @@ -5170,13 +5176,7 @@ describe('NetworkController', () => { async ({ controller, network }) => { network.mockEssentialRpcCalls(); - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.resetConnection(); - }, - }); + await controller.resetConnection(); expect(controller.store.getState().networkStatus).toBe( 'available', @@ -5208,13 +5208,7 @@ describe('NetworkController', () => { }, }); - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - operation: () => { - controller.resetConnection(); - }, - }); + await controller.resetConnection(); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { @@ -5259,6 +5253,8 @@ describe('NetworkController', () => { messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkWillChange, operation: () => { + // Intentionally not awaited because we're capturing an event + // emitted partway through the operation controller.resetConnection(); }, }); @@ -5309,6 +5305,8 @@ describe('NetworkController', () => { // before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we want to check state + // partway through the operation controller.resetConnection(); }, }); @@ -5359,6 +5357,8 @@ describe('NetworkController', () => { // before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we want to check state + // partway through the operation controller.resetConnection(); }, }); @@ -5394,7 +5394,7 @@ describe('NetworkController', () => { async ({ controller, network }) => { network.mockEssentialRpcCalls(); - controller.resetConnection(); + await controller.resetConnection(); const { provider } = controller.getProviderAndBlockTracker(); assert(provider, 'Provider is somehow unset'); @@ -5441,12 +5441,7 @@ describe('NetworkController', () => { const { provider: providerBefore } = controller.getProviderAndBlockTracker(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.resetConnection(); - }, - }); + await controller.resetConnection(); const { provider: providerAfter } = controller.getProviderAndBlockTracker(); @@ -5485,8 +5480,8 @@ describe('NetworkController', () => { const networkDidChange = await waitForPublishedEvents({ messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkDidChange, - operation: () => { - controller.resetConnection(); + operation: async () => { + await controller.resetConnection(); }, }); @@ -5525,8 +5520,8 @@ describe('NetworkController', () => { const infuraIsUnblocked = await waitForPublishedEvents({ messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.InfuraIsUnblocked, - operation: () => { - controller.resetConnection(); + operation: async () => { + await controller.resetConnection(); }, }); @@ -5563,13 +5558,7 @@ describe('NetworkController', () => { }); expect(controller.store.getState().networkStatus).toBe('unknown'); - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.resetConnection(); - }, - }); + await controller.resetConnection(); expect(controller.store.getState().networkStatus).toBe('available'); }, @@ -5607,13 +5596,7 @@ describe('NetworkController', () => { }, }); - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - operation: () => { - controller.resetConnection(); - }, - }); + await controller.resetConnection(); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { diff --git a/app/scripts/controllers/network/network-controller.ts b/app/scripts/controllers/network/network-controller.ts index 783e368a164a..51653ab4a209 100644 --- a/app/scripts/controllers/network/network-controller.ts +++ b/app/scripts/controllers/network/network-controller.ts @@ -753,8 +753,8 @@ export class NetworkController extends EventEmitter { /** * Re-initializes the provider and block tracker for the current network. */ - resetConnection(): void { - this.#setProviderConfig(this.store.getState().provider); + async resetConnection() { + await this.#setProviderConfig(this.store.getState().provider); } /**