From 0f08f8926059b1718c99d49ed89a4e33a396c486 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Thu, 14 Mar 2024 16:08:35 -0700 Subject: [PATCH 01/14] wip --- .../src/QueuedRequestController.ts | 3 ++- .../src/QueuedRequestMiddleware.ts | 16 ++-------------- .../queued-request-controller/src/constants.ts | 16 ++++++++++++++++ 3 files changed, 20 insertions(+), 15 deletions(-) create mode 100644 packages/queued-request-controller/src/constants.ts diff --git a/packages/queued-request-controller/src/QueuedRequestController.ts b/packages/queued-request-controller/src/QueuedRequestController.ts index 63c591be4b..b45bf65e4e 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.ts @@ -13,6 +13,7 @@ import { createDeferredPromise } from '@metamask/utils'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; +import {methodsRequiringNetwork} from './constants'; export const controllerName = 'QueuedRequestController'; export type QueuedRequestControllerState = { @@ -282,7 +283,7 @@ export class QueuedRequestController extends BaseController< this.#updateQueuedRequestCount(); await waitForDequeue; - } else if (request.method !== 'eth_requestAccounts') { + } else if (methodsRequiringNetwork.includes(request.method) === false) { // Process request immediately // Requires switching network now if necessary // Note: we dont need to switch chain before processing eth_requestAccounts because accounts are not network-specific (at the time of writing) diff --git a/packages/queued-request-controller/src/QueuedRequestMiddleware.ts b/packages/queued-request-controller/src/QueuedRequestMiddleware.ts index e0a1f988fa..bc5528b5a6 100644 --- a/packages/queued-request-controller/src/QueuedRequestMiddleware.ts +++ b/packages/queued-request-controller/src/QueuedRequestMiddleware.ts @@ -5,22 +5,10 @@ import type { Json, JsonRpcParams, JsonRpcRequest } from '@metamask/utils'; import type { QueuedRequestController } from './QueuedRequestController'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; +import {methodsWithConfirmation} from './constants'; const isConfirmationMethod = (method: string) => { - const confirmationMethods = [ - 'eth_sendTransaction', - 'wallet_watchAsset', - 'wallet_switchEthereumChain', - 'eth_signTypedData_v4', - 'wallet_addEthereumChain', - 'wallet_requestPermissions', - 'wallet_requestSnaps', - 'personal_sign', - 'eth_sign', - 'eth_requestAccounts', - ]; - - return confirmationMethods.includes(method); + return methodsWithConfirmation.includes(method); }; /** diff --git a/packages/queued-request-controller/src/constants.ts b/packages/queued-request-controller/src/constants.ts new file mode 100644 index 0000000000..1e6b4bc8a3 --- /dev/null +++ b/packages/queued-request-controller/src/constants.ts @@ -0,0 +1,16 @@ +export const methodsRequiringNetwork = [ + 'eth_sendTransaction', + 'wallet_watchAsset', + 'wallet_switchEthereumChain', + 'wallet_addEthereumChain', +] + +export const methodsWithConfirmation = [ + ...methodsRequiringNetwork, + 'eth_signTypedData_v4', + 'wallet_requestPermissions', + 'wallet_requestSnaps', + 'personal_sign', + 'eth_sign', + 'eth_requestAccounts', +]; From cd7d8d8f83dc771e62c4425396b97c1060da57a0 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 15 Mar 2024 14:36:25 -0700 Subject: [PATCH 02/14] update method list --- .../src/QueuedRequestController.ts | 2 +- packages/queued-request-controller/src/constants.ts | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestController.ts b/packages/queued-request-controller/src/QueuedRequestController.ts index b45bf65e4e..4647f9369e 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.ts @@ -283,7 +283,7 @@ export class QueuedRequestController extends BaseController< this.#updateQueuedRequestCount(); await waitForDequeue; - } else if (methodsRequiringNetwork.includes(request.method) === false) { + } else if (methodsRequiringNetwork.includes(request.method)) { // Process request immediately // Requires switching network now if necessary // Note: we dont need to switch chain before processing eth_requestAccounts because accounts are not network-specific (at the time of writing) diff --git a/packages/queued-request-controller/src/constants.ts b/packages/queued-request-controller/src/constants.ts index 1e6b4bc8a3..dee80fc190 100644 --- a/packages/queued-request-controller/src/constants.ts +++ b/packages/queued-request-controller/src/constants.ts @@ -1,16 +1,21 @@ export const methodsRequiringNetwork = [ 'eth_sendTransaction', - 'wallet_watchAsset', + 'eth_sendRawTransaction', 'wallet_switchEthereumChain', 'wallet_addEthereumChain', + 'wallet_watchAsset', + 'eth_signTypedData_v4', + 'personal_sign', ] export const methodsWithConfirmation = [ ...methodsRequiringNetwork, - 'eth_signTypedData_v4', 'wallet_requestPermissions', 'wallet_requestSnaps', - 'personal_sign', + 'wallet_snap', + 'wallet_invokeSnap', + 'eth_decrypt', 'eth_sign', 'eth_requestAccounts', + 'eth_getEncryptionPublicKey', ]; From 16a618f5ffcdbdefe1ce5c0e32ed03f3b4f61277 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 15 Mar 2024 14:40:44 -0700 Subject: [PATCH 03/14] lint --- .../src/QueuedRequestController.ts | 4 ++-- .../src/QueuedRequestMiddleware.ts | 2 +- packages/queued-request-controller/src/constants.ts | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestController.ts b/packages/queued-request-controller/src/QueuedRequestController.ts index 4647f9369e..2f2202b1a4 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.ts @@ -11,9 +11,9 @@ import type { import type { SelectedNetworkControllerGetNetworkClientIdForDomainAction } from '@metamask/selected-network-controller'; import { createDeferredPromise } from '@metamask/utils'; +import { methodsRequiringNetworkSwitch } from './constants'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; -import {methodsRequiringNetwork} from './constants'; export const controllerName = 'QueuedRequestController'; export type QueuedRequestControllerState = { @@ -283,7 +283,7 @@ export class QueuedRequestController extends BaseController< this.#updateQueuedRequestCount(); await waitForDequeue; - } else if (methodsRequiringNetwork.includes(request.method)) { + } else if (methodsRequiringNetworkSwitch.includes(request.method)) { // Process request immediately // Requires switching network now if necessary // Note: we dont need to switch chain before processing eth_requestAccounts because accounts are not network-specific (at the time of writing) diff --git a/packages/queued-request-controller/src/QueuedRequestMiddleware.ts b/packages/queued-request-controller/src/QueuedRequestMiddleware.ts index bc5528b5a6..f564ba5227 100644 --- a/packages/queued-request-controller/src/QueuedRequestMiddleware.ts +++ b/packages/queued-request-controller/src/QueuedRequestMiddleware.ts @@ -3,9 +3,9 @@ import { createAsyncMiddleware } from '@metamask/json-rpc-engine'; import { serializeError } from '@metamask/rpc-errors'; import type { Json, JsonRpcParams, JsonRpcRequest } from '@metamask/utils'; +import { methodsWithConfirmation } from './constants'; import type { QueuedRequestController } from './QueuedRequestController'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; -import {methodsWithConfirmation} from './constants'; const isConfirmationMethod = (method: string) => { return methodsWithConfirmation.includes(method); diff --git a/packages/queued-request-controller/src/constants.ts b/packages/queued-request-controller/src/constants.ts index dee80fc190..4b48f27e36 100644 --- a/packages/queued-request-controller/src/constants.ts +++ b/packages/queued-request-controller/src/constants.ts @@ -1,4 +1,4 @@ -export const methodsRequiringNetwork = [ +export const methodsRequiringNetworkSwitch = [ 'eth_sendTransaction', 'eth_sendRawTransaction', 'wallet_switchEthereumChain', @@ -6,10 +6,10 @@ export const methodsRequiringNetwork = [ 'wallet_watchAsset', 'eth_signTypedData_v4', 'personal_sign', -] +]; export const methodsWithConfirmation = [ - ...methodsRequiringNetwork, + ...methodsRequiringNetworkSwitch, 'wallet_requestPermissions', 'wallet_requestSnaps', 'wallet_snap', From 1e6355f1c2f8d8f26c46cdfdf69556225009d3c3 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 15 Mar 2024 14:55:30 -0700 Subject: [PATCH 04/14] delete duplicate scenario --- .../src/QueuedRequestMiddleware.test.ts | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts b/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts index f8b1b5dead..43aa65c1f0 100644 --- a/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts +++ b/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts @@ -139,27 +139,6 @@ describe('createQueuedRequestMiddleware', () => { expect(mockEnqueueRequest).not.toHaveBeenCalled(); }); - it('enqueues request that has a confirmation', async () => { - const mockEnqueueRequest = getMockEnqueueRequest(); - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: mockEnqueueRequest, - useRequestQueue: () => true, - }); - const request = { - ...getRequestDefaults(), - origin: 'exampleorigin.com', - method: 'eth_sendTransaction', - }; - - await new Promise((resolve, reject) => - middleware(request, getPendingResponseDefault(), resolve, reject), - ); - - expect(mockEnqueueRequest).toHaveBeenCalledWith( - request, - expect.any(Function), - ); - }); it('enqueues request that have a confirmation', async () => { const mockEnqueueRequest = getMockEnqueueRequest(); From 2eb0ff263430a628f866fc76e9de7393c42d8a4d Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 15 Mar 2024 14:55:41 -0700 Subject: [PATCH 05/14] Add specs for new constants --- .../src/QueuedRequestController.test.ts | 60 ++++++++++--------- .../src/QueuedRequestMiddleware.test.ts | 43 ++++++------- 2 files changed, 55 insertions(+), 48 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestController.test.ts b/packages/queued-request-controller/src/QueuedRequestController.test.ts index e00f44311a..0d03f9f41b 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.test.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.test.ts @@ -8,6 +8,7 @@ import type { SelectedNetworkControllerGetNetworkClientIdForDomainAction } from import { createDeferredPromise } from '@metamask/utils'; import { cloneDeep } from 'lodash'; +import { methodsRequiringNetworkSwitch } from './constants'; import type { AllowedActions, QueuedRequestControllerActions, @@ -100,36 +101,39 @@ describe('QueuedRequestController', () => { ); }); - it('does not switch networks if the method is `eth_requestAccounts`', async () => { - const mockSetActiveNetwork = jest.fn(); - const { messenger } = buildControllerMessenger({ - networkControllerGetState: jest.fn().mockReturnValue({ - ...cloneDeep(defaultNetworkState), - selectedNetworkClientId: 'selectedNetworkClientId', - }), - networkControllerSetActiveNetwork: mockSetActiveNetwork, - selectedNetworkControllerGetNetworkClientIdForDomain: jest - .fn() - .mockImplementation((_origin) => 'differentNetworkClientId'), - }); - const onNetworkSwitched = jest.fn(); - messenger.subscribe( - 'QueuedRequestController:networkSwitched', - onNetworkSwitched, - ); - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + it.each(methodsRequiringNetworkSwitch)( + 'does not switch networks if the method is `%s`', + async (method) => { + const mockSetActiveNetwork = jest.fn(); + const { messenger } = buildControllerMessenger({ + networkControllerGetState: jest.fn().mockReturnValue({ + ...cloneDeep(defaultNetworkState), + selectedNetworkClientId: 'selectedNetworkClientId', + }), + networkControllerSetActiveNetwork: mockSetActiveNetwork, + selectedNetworkControllerGetNetworkClientIdForDomain: jest + .fn() + .mockImplementation((_origin) => 'differentNetworkClientId'), + }); + const onNetworkSwitched = jest.fn(); + messenger.subscribe( + 'QueuedRequestController:networkSwitched', + onNetworkSwitched, + ); + const options: QueuedRequestControllerOptions = { + messenger: buildQueuedRequestControllerMessenger(messenger), + }; + const controller = new QueuedRequestController(options); - await controller.enqueueRequest( - { ...buildRequest(), method: 'eth_requestAccounts' }, - () => new Promise((resolve) => setTimeout(resolve, 10)), - ); + await controller.enqueueRequest( + { ...buildRequest(), method }, + () => new Promise((resolve) => setTimeout(resolve, 10)), + ); - expect(mockSetActiveNetwork).not.toHaveBeenCalled(); - expect(onNetworkSwitched).not.toHaveBeenCalled(); - }); + expect(mockSetActiveNetwork).not.toHaveBeenCalled(); + expect(onNetworkSwitched).not.toHaveBeenCalled(); + }, + ); it('does not switch networks if a request comes in for the same network client', async () => { const mockSetActiveNetwork = jest.fn(); diff --git a/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts b/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts index 43aa65c1f0..3b6f059d3f 100644 --- a/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts +++ b/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts @@ -1,6 +1,7 @@ import { errorCodes } from '@metamask/rpc-errors'; import type { Json, PendingJsonRpcResponse } from '@metamask/utils'; +import { methodsWithConfirmation } from './constants'; import type { QueuedRequestControllerEnqueueRequestAction } from './QueuedRequestController'; import { createQueuedRequestMiddleware } from './QueuedRequestMiddleware'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; @@ -139,28 +140,30 @@ describe('createQueuedRequestMiddleware', () => { expect(mockEnqueueRequest).not.toHaveBeenCalled(); }); + it.each(methodsWithConfirmation)( + 'enqueues the `%s` request that has a confirmation', + async (method) => { + const mockEnqueueRequest = getMockEnqueueRequest(); + const middleware = createQueuedRequestMiddleware({ + enqueueRequest: mockEnqueueRequest, + useRequestQueue: () => true, + }); + const request = { + ...getRequestDefaults(), + origin: 'exampleorigin.com', + method, + }; - it('enqueues request that have a confirmation', async () => { - const mockEnqueueRequest = getMockEnqueueRequest(); - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: mockEnqueueRequest, - useRequestQueue: () => true, - }); - const request = { - ...getRequestDefaults(), - origin: 'exampleorigin.com', - method: 'eth_sendTransaction', - }; - - await new Promise((resolve, reject) => - middleware(request, getPendingResponseDefault(), resolve, reject), - ); + await new Promise((resolve, reject) => + middleware(request, getPendingResponseDefault(), resolve, reject), + ); - expect(mockEnqueueRequest).toHaveBeenCalledWith( - request, - expect.any(Function), - ); - }); + expect(mockEnqueueRequest).toHaveBeenCalledWith( + request, + expect.any(Function), + ); + }, + ); it('calls next when a request is not queued', async () => { const middleware = createQueuedRequestMiddleware({ From 908a3a65aee7af4f147c850294d03a15bc169235 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 15 Mar 2024 15:15:11 -0700 Subject: [PATCH 06/14] add comments to constants --- .../queued-request-controller/CHANGELOG.md | 27 +++++++++++++++++++ .../src/constants.ts | 13 +++++++++ 2 files changed, 40 insertions(+) diff --git a/packages/queued-request-controller/CHANGELOG.md b/packages/queued-request-controller/CHANGELOG.md index c56025f033..2846a00ac2 100644 --- a/packages/queued-request-controller/CHANGELOG.md +++ b/packages/queued-request-controller/CHANGELOG.md @@ -7,6 +7,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## Fixed + +- `QueuedRequestMiddleware` now enqueues the following methods that can trigger confirmations: ([#4066](https://github.com/MetaMask/core/pull/4066)) + - `eth_sendTransaction` + - `eth_sendRawTransaction` + - `wallet_switchEthereumChain` + - `wallet_addEthereumChain` + - `wallet_watchAsset` + - `eth_signTypedData_v4` + - `personal_sign` + - `wallet_requestPermissions` + - `wallet_requestSnaps` + - `wallet_snap` + - `wallet_invokeSnap` + - `eth_decrypt` + - `eth_sign` + - `eth_requestAccounts` + - `eth_getEncryptionPublicKey` +- `QueuedRequestController.enqueueRequest()` now ensures the globally selected network matches the dapp selected network before processing the following methods: ([#4066](https://github.com/MetaMask/core/pull/4066)) + - `eth_sendTransaction` + - `eth_sendRawTransaction` + - `wallet_switchEthereumChain` + - `wallet_addEthereumChain` + - `wallet_watchAsset` + - `eth_signTypedData_v4` + - `personal_sign` + ## [0.6.1] ### Fixed diff --git a/packages/queued-request-controller/src/constants.ts b/packages/queued-request-controller/src/constants.ts index 4b48f27e36..1b23ef8b4f 100644 --- a/packages/queued-request-controller/src/constants.ts +++ b/packages/queued-request-controller/src/constants.ts @@ -1,3 +1,11 @@ +/** + * This is a list of methods that require the globally selected network + * to match the dapp selected network before being processed. These can + * be for UI/UX reasons where the currently selected network is displayed + * in the confirmation even though it will be submitted on the correct + * network for the dapp. It could also be that a method expects the + * globally selected network to match some value in the request params itself. + */ export const methodsRequiringNetworkSwitch = [ 'eth_sendTransaction', 'eth_sendRawTransaction', @@ -8,6 +16,11 @@ export const methodsRequiringNetworkSwitch = [ 'personal_sign', ]; +/** + * This is a list of methods that can cause a confirmation to be + * presented to the user. Note that some of these methods may + * only sometimes cause a confirmation to appear. + */ export const methodsWithConfirmation = [ ...methodsRequiringNetworkSwitch, 'wallet_requestPermissions', From 3705d8de95313c9e3a89aadc991fdb46364b512d Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 15 Mar 2024 15:21:15 -0700 Subject: [PATCH 07/14] lint --- packages/queued-request-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/queued-request-controller/CHANGELOG.md b/packages/queued-request-controller/CHANGELOG.md index 2846a00ac2..eb211bf421 100644 --- a/packages/queued-request-controller/CHANGELOG.md +++ b/packages/queued-request-controller/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## Fixed +### Fixed - `QueuedRequestMiddleware` now enqueues the following methods that can trigger confirmations: ([#4066](https://github.com/MetaMask/core/pull/4066)) - `eth_sendTransaction` From 9decce609a23d2bb10ff21ef31a05b2498cbb350 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 29 Mar 2024 09:28:52 -0700 Subject: [PATCH 08/14] fix conflict --- packages/queued-request-controller/CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/queued-request-controller/CHANGELOG.md b/packages/queued-request-controller/CHANGELOG.md index fa594cf7bc..5c172521b3 100644 --- a/packages/queued-request-controller/CHANGELOG.md +++ b/packages/queued-request-controller/CHANGELOG.md @@ -7,7 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -<<<<<<< HEAD ### Fixed - `QueuedRequestMiddleware` now enqueues the following methods that can trigger confirmations: ([#4066](https://github.com/MetaMask/core/pull/4066)) @@ -35,8 +34,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `eth_signTypedData_v4` - `personal_sign` -||||||| 7da8ef70 -======= ## [0.7.0] ### Changed @@ -44,7 +41,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **BREAKING:** Bump peer dependency `@metamask/selected-network-controller` to `^11.0.0` ([#4121](https://github.com/MetaMask/core/pull/4121)) - Bump `@metamask/controller-utils` to `^9.0.2` ([#4065](https://github.com/MetaMask/core/pull/4065)) ->>>>>>> main ## [0.6.1] ### Fixed From d75b97cce03ddcdd62f3d94ef106f404960a619f Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 29 Mar 2024 14:12:19 -0700 Subject: [PATCH 09/14] Add methodsRequiringNetworkSwitch to QueuedRequestController --- .../src/QueuedRequestController.ts | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestController.ts b/packages/queued-request-controller/src/QueuedRequestController.ts index 2f2202b1a4..a371c13ac3 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.ts @@ -11,7 +11,6 @@ import type { import type { SelectedNetworkControllerGetNetworkClientIdForDomainAction } from '@metamask/selected-network-controller'; import { createDeferredPromise } from '@metamask/utils'; -import { methodsRequiringNetworkSwitch } from './constants'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; export const controllerName = 'QueuedRequestController'; @@ -74,6 +73,7 @@ export type QueuedRequestControllerMessenger = RestrictedControllerMessenger< export type QueuedRequestControllerOptions = { messenger: QueuedRequestControllerMessenger; + methodsRequiringNetworkSwitch: string[]; }; /** @@ -128,13 +128,27 @@ export class QueuedRequestController extends BaseController< */ #processingRequestCount = 0; + /** + * This is a list of methods that require the globally selected network + * to match the dapp selected network before being processed. These can + * be for UI/UX reasons where the currently selected network is displayed + * in the confirmation even though it will be submitted on the correct + * network for the dapp. It could also be that a method expects the + * globally selected network to match some value in the request params itself. + */ + readonly #methodsRequiringNetworkSwitch: string[]; + /** * Construct a QueuedRequestController. * * @param options - Controller options. * @param options.messenger - The restricted controller messenger that facilitates communication with other controllers. + * @param options.methodsRequiringNetworkSwitch - A list of methods that require the globally selected network to match the dapp selected network. */ - constructor({ messenger }: QueuedRequestControllerOptions) { + constructor({ + messenger, + methodsRequiringNetworkSwitch, + }: QueuedRequestControllerOptions) { super({ name: controllerName, metadata: { @@ -146,6 +160,7 @@ export class QueuedRequestController extends BaseController< messenger, state: { queuedRequestCount: 0 }, }); + this.#methodsRequiringNetworkSwitch = methodsRequiringNetworkSwitch; this.#registerMessageHandlers(); } @@ -283,10 +298,9 @@ export class QueuedRequestController extends BaseController< this.#updateQueuedRequestCount(); await waitForDequeue; - } else if (methodsRequiringNetworkSwitch.includes(request.method)) { + } else if (this.#methodsRequiringNetworkSwitch.includes(request.method)) { // Process request immediately // Requires switching network now if necessary - // Note: we dont need to switch chain before processing eth_requestAccounts because accounts are not network-specific (at the time of writing) await this.#switchNetworkIfNecessary(); } this.#processingRequestCount += 1; From ad8dc07dc439d00aca6dc3878cff0e2fdbcbd33c Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 29 Mar 2024 14:21:49 -0700 Subject: [PATCH 10/14] fix QueuedRequestController specs. DRY builder --- .../src/QueuedRequestController.test.ts | 233 +++++++++--------- 1 file changed, 121 insertions(+), 112 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestController.test.ts b/packages/queued-request-controller/src/QueuedRequestController.test.ts index 0d03f9f41b..80bcc9f8cf 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.test.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.test.ts @@ -8,7 +8,6 @@ import type { SelectedNetworkControllerGetNetworkClientIdForDomainAction } from import { createDeferredPromise } from '@metamask/utils'; import { cloneDeep } from 'lodash'; -import { methodsRequiringNetworkSwitch } from './constants'; import type { AllowedActions, QueuedRequestControllerActions, @@ -26,6 +25,7 @@ describe('QueuedRequestController', () => { it('can be instantiated with default values', () => { const options: QueuedRequestControllerOptions = { messenger: buildQueuedRequestControllerMessenger(), + methodsRequiringNetworkSwitch: [], }; const controller = new QueuedRequestController(options); @@ -34,10 +34,7 @@ describe('QueuedRequestController', () => { describe('enqueueRequest', () => { it('skips the queue if the queue is empty and no request is being processed', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); await controller.enqueueRequest(buildRequest(), async () => { expect(controller.state.queuedRequestCount).toBe(0); @@ -46,10 +43,7 @@ describe('QueuedRequestController', () => { }); it('skips the queue if the queue is empty and the request being processed has the same origin', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); // Trigger first request const firstRequest = controller.enqueueRequest( buildRequest(), @@ -66,7 +60,7 @@ describe('QueuedRequestController', () => { await firstRequest; }); - it('switches network if a request comes in for a different network client', async () => { + it('switches network if a request comes in for a different network client and the method is in the methodsRequiringNetworkSwitch param', async () => { const mockSetActiveNetwork = jest.fn(); const { messenger } = buildControllerMessenger({ networkControllerGetState: jest.fn().mockReturnValue({ @@ -83,13 +77,13 @@ describe('QueuedRequestController', () => { 'QueuedRequestController:networkSwitched', onNetworkSwitched, ); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + methodsRequiringNetworkSwitch: ['method_requiring_network_switch'], + }); await controller.enqueueRequest( - buildRequest(), + { ...buildRequest(), method: 'method_requiring_network_switch' }, () => new Promise((resolve) => setTimeout(resolve, 10)), ); @@ -101,39 +95,36 @@ describe('QueuedRequestController', () => { ); }); - it.each(methodsRequiringNetworkSwitch)( - 'does not switch networks if the method is `%s`', - async (method) => { - const mockSetActiveNetwork = jest.fn(); - const { messenger } = buildControllerMessenger({ - networkControllerGetState: jest.fn().mockReturnValue({ - ...cloneDeep(defaultNetworkState), - selectedNetworkClientId: 'selectedNetworkClientId', - }), - networkControllerSetActiveNetwork: mockSetActiveNetwork, - selectedNetworkControllerGetNetworkClientIdForDomain: jest - .fn() - .mockImplementation((_origin) => 'differentNetworkClientId'), - }); - const onNetworkSwitched = jest.fn(); - messenger.subscribe( - 'QueuedRequestController:networkSwitched', - onNetworkSwitched, - ); - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + it('does not switch networks if the method is not in the methodsRequiringNetworkSwitch param', async () => { + const mockSetActiveNetwork = jest.fn(); + const { messenger } = buildControllerMessenger({ + networkControllerGetState: jest.fn().mockReturnValue({ + ...cloneDeep(defaultNetworkState), + selectedNetworkClientId: 'selectedNetworkClientId', + }), + networkControllerSetActiveNetwork: mockSetActiveNetwork, + selectedNetworkControllerGetNetworkClientIdForDomain: jest + .fn() + .mockImplementation((_origin) => 'differentNetworkClientId'), + }); + const onNetworkSwitched = jest.fn(); + messenger.subscribe( + 'QueuedRequestController:networkSwitched', + onNetworkSwitched, + ); + const controller = buildQueuedRequestController({ + messenger: buildQueuedRequestControllerMessenger(messenger), + methodsRequiringNetworkSwitch: [], + }); - await controller.enqueueRequest( - { ...buildRequest(), method }, - () => new Promise((resolve) => setTimeout(resolve, 10)), - ); + await controller.enqueueRequest( + { ...buildRequest(), method: 'not_in_methodsRequiringNetworkSwitch' }, + () => new Promise((resolve) => setTimeout(resolve, 10)), + ); - expect(mockSetActiveNetwork).not.toHaveBeenCalled(); - expect(onNetworkSwitched).not.toHaveBeenCalled(); - }, - ); + expect(mockSetActiveNetwork).not.toHaveBeenCalled(); + expect(onNetworkSwitched).not.toHaveBeenCalled(); + }); it('does not switch networks if a request comes in for the same network client', async () => { const mockSetActiveNetwork = jest.fn(); @@ -152,10 +143,9 @@ describe('QueuedRequestController', () => { 'QueuedRequestController:networkSwitched', onNetworkSwitched, ); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + }); await controller.enqueueRequest( buildRequest(), @@ -167,10 +157,7 @@ describe('QueuedRequestController', () => { }); it('queues request if a request from another origin is being processed', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); // Trigger first request const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'https://exampleorigin1.metamask.io' }, @@ -193,10 +180,7 @@ describe('QueuedRequestController', () => { }); it('drains batch from queue when current batch finishes', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); // Trigger first batch const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'https://firstbatch.metamask.io' }, @@ -240,10 +224,7 @@ describe('QueuedRequestController', () => { }); it('drains batch from queue when current batch finishes with requests out-of-order', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); // Trigger first batch const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'https://firstbatch.metamask.io' }, @@ -287,10 +268,7 @@ describe('QueuedRequestController', () => { }); it('processes requests from each batch in parallel', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'https://firstorigin.metamask.io' }, async () => { @@ -346,10 +324,7 @@ describe('QueuedRequestController', () => { }); it('preserves request order within each batch', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); const executionOrder: string[] = []; const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'https://firstorigin.metamask.io' }, @@ -402,10 +377,7 @@ describe('QueuedRequestController', () => { }); it('preserves request order even when interlaced with requests from other origins', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); const executionOrder: string[] = []; const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'https://firstorigin.metamask.io' }, @@ -465,10 +437,9 @@ describe('QueuedRequestController', () => { 'QueuedRequestController:networkSwitched', onNetworkSwitched, ); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + }); const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'https://firstorigin.metamask.io' }, () => new Promise((resolve) => setTimeout(resolve, 10)), @@ -517,10 +488,9 @@ describe('QueuedRequestController', () => { 'QueuedRequestController:networkSwitched', onNetworkSwitched, ); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + }); const firstRequest = controller.enqueueRequest( { ...buildRequest(), origin: 'firstorigin.metamask.io' }, () => new Promise((resolve) => setTimeout(resolve, 10)), @@ -562,14 +532,18 @@ describe('QueuedRequestController', () => { .fn() .mockImplementation((_origin) => 'differentNetworkClientId'), }); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + methodsRequiringNetworkSwitch: ['method_requiring_network_switch'], + }); await expect(() => controller.enqueueRequest( - { ...buildRequest(), origin: 'https://example.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://example.metamask.io', + }, jest.fn(), ), ).rejects.toThrow(switchError); @@ -593,12 +567,16 @@ describe('QueuedRequestController', () => { : 'selectedNetworkClientId', ), }); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + methodsRequiringNetworkSwitch: ['method_requiring_network_switch'], + }); const firstRequest = controller.enqueueRequest( - { ...buildRequest(), origin: 'https://firstorigin.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://firstorigin.metamask.io', + }, () => new Promise((resolve) => setTimeout(resolve, 10)), ); // ensure first request skips queue @@ -609,7 +587,11 @@ describe('QueuedRequestController', () => { () => new Promise((resolve) => setTimeout(resolve, 100)), ); const secondRequest = controller.enqueueRequest( - { ...buildRequest(), origin: 'https://secondorigin.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://secondorigin.metamask.io', + }, secondRequestNext, ); @@ -639,12 +621,16 @@ describe('QueuedRequestController', () => { : 'selectedNetworkClientId', ), }); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + methodsRequiringNetworkSwitch: ['method_requiring_network_switch'], + }); const firstRequest = controller.enqueueRequest( - { ...buildRequest(), origin: 'https://firstorigin.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://firstorigin.metamask.io', + }, () => new Promise((resolve) => setTimeout(resolve, 10)), ); // ensure first request skips queue @@ -655,7 +641,11 @@ describe('QueuedRequestController', () => { () => new Promise((resolve) => setTimeout(resolve, 100)), ); const secondRequest = controller.enqueueRequest( - { ...buildRequest(), origin: 'https://secondorigin.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://secondorigin.metamask.io', + }, secondRequestNext, ); // ensure test starts with one request queued up @@ -684,12 +674,16 @@ describe('QueuedRequestController', () => { : 'selectedNetworkClientId', ), }); - const options: QueuedRequestControllerOptions = { + const controller = buildQueuedRequestController({ messenger: buildQueuedRequestControllerMessenger(messenger), - }; - const controller = new QueuedRequestController(options); + methodsRequiringNetworkSwitch: ['method_requiring_network_switch'], + }); const firstRequest = controller.enqueueRequest( - { ...buildRequest(), origin: 'https://firstorigin.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://firstorigin.metamask.io', + }, () => new Promise((resolve) => setTimeout(resolve, 10)), ); // ensure first request skips queue @@ -700,7 +694,11 @@ describe('QueuedRequestController', () => { () => new Promise((resolve) => setTimeout(resolve, 100)), ); const secondRequest = controller.enqueueRequest( - { ...buildRequest(), origin: 'https://secondorigin.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://secondorigin.metamask.io', + }, secondRequestNext, ); const thirdRequestNext = jest @@ -709,7 +707,11 @@ describe('QueuedRequestController', () => { () => new Promise((resolve) => setTimeout(resolve, 100)), ); const thirdRequest = controller.enqueueRequest( - { ...buildRequest(), origin: 'https://thirdorigin.metamask.io' }, + { + ...buildRequest(), + method: 'method_requiring_network_switch', + origin: 'https://thirdorigin.metamask.io', + }, thirdRequestNext, ); // ensure test starts with two requests queued up @@ -726,11 +728,7 @@ describe('QueuedRequestController', () => { describe('when a request fails', () => { it('throws error', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); // Mock a request that throws an error const requestWithError = jest.fn(() => @@ -748,10 +746,7 @@ describe('QueuedRequestController', () => { }); it('correctly updates the request queue count upon failure', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); await expect(() => controller.enqueueRequest( @@ -765,11 +760,7 @@ describe('QueuedRequestController', () => { }); it('correctly processes the next item in the queue', async () => { - const options: QueuedRequestControllerOptions = { - messenger: buildQueuedRequestControllerMessenger(), - }; - - const controller = new QueuedRequestController(options); + const controller = buildQueuedRequestController(); // Mock requests with one request throwing an error const request1 = jest.fn(async () => { @@ -904,6 +895,24 @@ function buildQueuedRequestControllerMessenger( }); } +/** + * Builds a QueuedRequestController + * + * @param overrideOptions - The optional options object. + * @returns The QueuedRequestController. + */ +function buildQueuedRequestController( + overrideOptions?: Partial, +): QueuedRequestController { + const options: QueuedRequestControllerOptions = { + messenger: buildQueuedRequestControllerMessenger(), + methodsRequiringNetworkSwitch: [], + ...overrideOptions, + }; + + return new QueuedRequestController(options); +} + /** * Build a valid JSON-RPC request that includes all required properties * From 7043f7470c6bc6efb6640e5ee5712c53f23d47ef Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 29 Mar 2024 14:22:19 -0700 Subject: [PATCH 11/14] Add methodsWithConfirmation param to QueuedRequestMiddleware --- .../src/QueuedRequestMiddleware.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestMiddleware.ts b/packages/queued-request-controller/src/QueuedRequestMiddleware.ts index f564ba5227..fbafa384bf 100644 --- a/packages/queued-request-controller/src/QueuedRequestMiddleware.ts +++ b/packages/queued-request-controller/src/QueuedRequestMiddleware.ts @@ -3,14 +3,9 @@ import { createAsyncMiddleware } from '@metamask/json-rpc-engine'; import { serializeError } from '@metamask/rpc-errors'; import type { Json, JsonRpcParams, JsonRpcRequest } from '@metamask/utils'; -import { methodsWithConfirmation } from './constants'; import type { QueuedRequestController } from './QueuedRequestController'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; -const isConfirmationMethod = (method: string) => { - return methodsWithConfirmation.includes(method); -}; - /** * Ensure that the incoming request has the additional required request metadata. This metadata * should be attached to the request earlier in the middleware pipeline. @@ -44,21 +39,24 @@ function hasRequiredMetadata( * @param options - Configuration options. * @param options.enqueueRequest - A method for enqueueing a request. * @param options.useRequestQueue - A function that determines if the request queue feature is enabled. + * @param options.methodsWithConfirmation - A list of methods that can cause a confirmation to be presented to the user. * @returns The JSON-RPC middleware that manages queued requests. */ export const createQueuedRequestMiddleware = ({ enqueueRequest, useRequestQueue, + methodsWithConfirmation, }: { enqueueRequest: QueuedRequestController['enqueueRequest']; useRequestQueue: () => boolean; + methodsWithConfirmation: string[]; }): JsonRpcMiddleware => { return createAsyncMiddleware(async (req: JsonRpcRequest, res, next) => { hasRequiredMetadata(req); // if the request queue feature is turned off, or this method is not a confirmation method // bypass the queue completely - if (!useRequestQueue() || !isConfirmationMethod(req.method)) { + if (!useRequestQueue() || !methodsWithConfirmation.includes(req.method)) { return await next(); } From cf389357092ccf03cbf12c87fb3657a26ad41622 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 29 Mar 2024 14:34:45 -0700 Subject: [PATCH 12/14] fix QueuedRequestMiddleware specs. DRY builders --- .../src/QueuedRequestMiddleware.test.ts | 172 ++++++++++-------- 1 file changed, 96 insertions(+), 76 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts b/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts index 3b6f059d3f..c062c70484 100644 --- a/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts +++ b/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts @@ -1,42 +1,13 @@ import { errorCodes } from '@metamask/rpc-errors'; import type { Json, PendingJsonRpcResponse } from '@metamask/utils'; -import { methodsWithConfirmation } from './constants'; import type { QueuedRequestControllerEnqueueRequestAction } from './QueuedRequestController'; import { createQueuedRequestMiddleware } from './QueuedRequestMiddleware'; import type { QueuedRequestMiddlewareJsonRpcRequest } from './types'; -const getRequestDefaults = (): QueuedRequestMiddlewareJsonRpcRequest => { - return { - method: 'doesnt matter', - id: 'doesnt matter', - jsonrpc: '2.0' as const, - origin: 'example.com', - networkClientId: 'mainnet', - }; -}; - -const getPendingResponseDefault = (): PendingJsonRpcResponse => { - return { - id: 'doesnt matter', - jsonrpc: '2.0' as const, - }; -}; - -const getMockEnqueueRequest = () => - jest - .fn< - ReturnType, - Parameters - >() - .mockImplementation((_request, requestNext) => requestNext()); - describe('createQueuedRequestMiddleware', () => { it('throws if not provided an origin', async () => { - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: getMockEnqueueRequest(), - useRequestQueue: () => false, - }); + const middleware = buildQueuedRequestMiddleware(); const request = getRequestDefaults(); // @ts-expect-error Intentionally invalid request delete request.origin; @@ -50,10 +21,7 @@ describe('createQueuedRequestMiddleware', () => { }); it('throws if provided an invalid origin', async () => { - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: getMockEnqueueRequest(), - useRequestQueue: () => false, - }); + const middleware = buildQueuedRequestMiddleware(); const request = getRequestDefaults(); // @ts-expect-error Intentionally invalid request request.origin = 1; @@ -67,10 +35,7 @@ describe('createQueuedRequestMiddleware', () => { }); it('throws if not provided an networkClientId', async () => { - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: getMockEnqueueRequest(), - useRequestQueue: () => false, - }); + const middleware = buildQueuedRequestMiddleware(); const request = getRequestDefaults(); // @ts-expect-error Intentionally invalid request delete request.networkClientId; @@ -84,10 +49,7 @@ describe('createQueuedRequestMiddleware', () => { }); it('throws if provided an invalid networkClientId', async () => { - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: getMockEnqueueRequest(), - useRequestQueue: () => false, - }); + const middleware = buildQueuedRequestMiddleware(); const request = getRequestDefaults(); // @ts-expect-error Intentionally invalid request request.networkClientId = 1; @@ -104,9 +66,8 @@ describe('createQueuedRequestMiddleware', () => { it('does not enqueue the request when useRequestQueue is false', async () => { const mockEnqueueRequest = getMockEnqueueRequest(); - const middleware = createQueuedRequestMiddleware({ + const middleware = buildQueuedRequestMiddleware({ enqueueRequest: mockEnqueueRequest, - useRequestQueue: () => false, }); await new Promise((resolve, reject) => @@ -123,7 +84,7 @@ describe('createQueuedRequestMiddleware', () => { it('does not enqueue request that has no confirmation', async () => { const mockEnqueueRequest = getMockEnqueueRequest(); - const middleware = createQueuedRequestMiddleware({ + const middleware = buildQueuedRequestMiddleware({ enqueueRequest: mockEnqueueRequest, useRequestQueue: () => true, }); @@ -140,36 +101,31 @@ describe('createQueuedRequestMiddleware', () => { expect(mockEnqueueRequest).not.toHaveBeenCalled(); }); - it.each(methodsWithConfirmation)( - 'enqueues the `%s` request that has a confirmation', - async (method) => { - const mockEnqueueRequest = getMockEnqueueRequest(); - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: mockEnqueueRequest, - useRequestQueue: () => true, - }); - const request = { - ...getRequestDefaults(), - origin: 'exampleorigin.com', - method, - }; + it('enqueues the request if the method is in the methodsWithConfirmation param', async () => { + const mockEnqueueRequest = getMockEnqueueRequest(); + const middleware = buildQueuedRequestMiddleware({ + enqueueRequest: mockEnqueueRequest, + useRequestQueue: () => true, + methodsWithConfirmation: ['method_with_confirmation'], + }); + const request = { + ...getRequestDefaults(), + origin: 'exampleorigin.com', + method: 'method_with_confirmation', + }; - await new Promise((resolve, reject) => - middleware(request, getPendingResponseDefault(), resolve, reject), - ); + await new Promise((resolve, reject) => + middleware(request, getPendingResponseDefault(), resolve, reject), + ); - expect(mockEnqueueRequest).toHaveBeenCalledWith( - request, - expect.any(Function), - ); - }, - ); + expect(mockEnqueueRequest).toHaveBeenCalledWith( + request, + expect.any(Function), + ); + }); it('calls next when a request is not queued', async () => { - const middleware = createQueuedRequestMiddleware({ - enqueueRequest: getMockEnqueueRequest(), - useRequestQueue: () => false, - }); + const middleware = buildQueuedRequestMiddleware(); const mockNext = jest.fn(); await new Promise((resolve) => { @@ -186,7 +142,7 @@ describe('createQueuedRequestMiddleware', () => { }); it('calls next after a request is queued and processed', async () => { - const middleware = createQueuedRequestMiddleware({ + const middleware = buildQueuedRequestMiddleware({ enqueueRequest: getMockEnqueueRequest(), useRequestQueue: () => true, }); @@ -206,15 +162,16 @@ describe('createQueuedRequestMiddleware', () => { describe('when enqueueRequest throws', () => { it('ends without calling next', async () => { - const middleware = createQueuedRequestMiddleware({ + const middleware = buildQueuedRequestMiddleware({ enqueueRequest: jest .fn() .mockRejectedValue(new Error('enqueuing error')), useRequestQueue: () => true, + methodsWithConfirmation: ['method_should_be_enqueued'], }); const request = { ...getRequestDefaults(), - method: 'eth_sendTransaction', + method: 'method_should_be_enqueued', }; const mockNext = jest.fn(); const mockEnd = jest.fn(); @@ -229,15 +186,16 @@ describe('createQueuedRequestMiddleware', () => { }); it('serializes processing errors and attaches them to the response', async () => { - const middleware = createQueuedRequestMiddleware({ + const middleware = buildQueuedRequestMiddleware({ enqueueRequest: jest .fn() .mockRejectedValue(new Error('enqueuing error')), useRequestQueue: () => true, + methodsWithConfirmation: ['method_should_be_enqueued'], }); const request = { ...getRequestDefaults(), - method: 'eth_sendTransaction', + method: 'method_should_be_enqueued', }; const response = getPendingResponseDefault(); @@ -257,3 +215,65 @@ describe('createQueuedRequestMiddleware', () => { }); }); }); + +/** + * Build a valid JSON-RPC request that includes all required properties + * + * @returns A valid JSON-RPC request with all required properties. + */ +function getRequestDefaults(): QueuedRequestMiddlewareJsonRpcRequest { + return { + method: 'doesnt matter', + id: 'doesnt matter', + jsonrpc: '2.0' as const, + origin: 'example.com', + networkClientId: 'mainnet', + }; +} + +/** + * Build a partial JSON-RPC response + * + * @returns A partial response request + */ +function getPendingResponseDefault(): PendingJsonRpcResponse { + return { + id: 'doesnt matter', + jsonrpc: '2.0' as const, + }; +} + +/** + * Builds a mock QueuedRequestController.enqueueRequest function + * + * @returns A mock function that calls the next request in the middleware chain + */ +function getMockEnqueueRequest() { + return jest + .fn< + ReturnType, + Parameters + >() + .mockImplementation((_request, requestNext) => requestNext()); +} + +/** + * Builds the QueuedRequestMiddleware + * + * @param overrideOptions - The optional options object. + * @returns The QueuedRequestMiddleware. + */ +function buildQueuedRequestMiddleware( + overrideOptions?: Partial< + Parameters[0] + >, +) { + const options = { + enqueueRequest: getMockEnqueueRequest(), + useRequestQueue: () => false, + methodsWithConfirmation: [], + ...overrideOptions, + }; + + return createQueuedRequestMiddleware(options); +} From da8a670274bcd4f9a719eb6f34dd4ef4d1bf5f19 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 29 Mar 2024 14:34:50 -0700 Subject: [PATCH 13/14] remove constants --- .../src/constants.ts | 34 ------------------- 1 file changed, 34 deletions(-) delete mode 100644 packages/queued-request-controller/src/constants.ts diff --git a/packages/queued-request-controller/src/constants.ts b/packages/queued-request-controller/src/constants.ts deleted file mode 100644 index 1b23ef8b4f..0000000000 --- a/packages/queued-request-controller/src/constants.ts +++ /dev/null @@ -1,34 +0,0 @@ -/** - * This is a list of methods that require the globally selected network - * to match the dapp selected network before being processed. These can - * be for UI/UX reasons where the currently selected network is displayed - * in the confirmation even though it will be submitted on the correct - * network for the dapp. It could also be that a method expects the - * globally selected network to match some value in the request params itself. - */ -export const methodsRequiringNetworkSwitch = [ - 'eth_sendTransaction', - 'eth_sendRawTransaction', - 'wallet_switchEthereumChain', - 'wallet_addEthereumChain', - 'wallet_watchAsset', - 'eth_signTypedData_v4', - 'personal_sign', -]; - -/** - * This is a list of methods that can cause a confirmation to be - * presented to the user. Note that some of these methods may - * only sometimes cause a confirmation to appear. - */ -export const methodsWithConfirmation = [ - ...methodsRequiringNetworkSwitch, - 'wallet_requestPermissions', - 'wallet_requestSnaps', - 'wallet_snap', - 'wallet_invokeSnap', - 'eth_decrypt', - 'eth_sign', - 'eth_requestAccounts', - 'eth_getEncryptionPublicKey', -]; From 4215f6505188c0365981d072e1f72ebd78b1c476 Mon Sep 17 00:00:00 2001 From: Jiexi Luan Date: Fri, 29 Mar 2024 14:40:51 -0700 Subject: [PATCH 14/14] update CHANGELOG --- .../queued-request-controller/CHANGELOG.md | 32 ++++--------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/packages/queued-request-controller/CHANGELOG.md b/packages/queued-request-controller/CHANGELOG.md index 5c172521b3..a450470797 100644 --- a/packages/queued-request-controller/CHANGELOG.md +++ b/packages/queued-request-controller/CHANGELOG.md @@ -7,32 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed +### Added + +- **BREAKING**: The `QueuedRequestMiddleware` constructor now requires the `methodsWithConfirmation` param which should be a list of methods that can trigger confirmations ([#4066](https://github.com/MetaMask/core/pull/4066)) +- **BREAKING**: The `QueuedRequestController` constructor now requires the `methodsRequiringNetworkSwitch` param which should be a list of methods that need the globally selected network to switched to the dapp selected network before being processed ([#4066](https://github.com/MetaMask/core/pull/4066)) + +### Changed -- `QueuedRequestMiddleware` now enqueues the following methods that can trigger confirmations: ([#4066](https://github.com/MetaMask/core/pull/4066)) - - `eth_sendTransaction` - - `eth_sendRawTransaction` - - `wallet_switchEthereumChain` - - `wallet_addEthereumChain` - - `wallet_watchAsset` - - `eth_signTypedData_v4` - - `personal_sign` - - `wallet_requestPermissions` - - `wallet_requestSnaps` - - `wallet_snap` - - `wallet_invokeSnap` - - `eth_decrypt` - - `eth_sign` - - `eth_requestAccounts` - - `eth_getEncryptionPublicKey` -- `QueuedRequestController.enqueueRequest()` now ensures the globally selected network matches the dapp selected network before processing the following methods: ([#4066](https://github.com/MetaMask/core/pull/4066)) - - `eth_sendTransaction` - - `eth_sendRawTransaction` - - `wallet_switchEthereumChain` - - `wallet_addEthereumChain` - - `wallet_watchAsset` - - `eth_signTypedData_v4` - - `personal_sign` +- **BREAKING**: `QueuedRequestController.enqueueRequest()` now ensures the globally selected network matches the dapp selected network before processing methods listed in the `methodsRequiringNetworkSwitch` constructor param. This replaces the previous behavior of switching for all methods except `eth_requestAccounts`. ([#4066](https://github.com/MetaMask/core/pull/4066)) ## [0.7.0]