QueuedRequestController: Fix list of methods that should have requests enqueued and/or switch the globally selected network

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

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();

packages/queued-request-controller/src/QueuedRequestController.ts

@@ -11,6 +11,7 @@ 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';
@@ -282,7 +283,7 @@ export class QueuedRequestController extends BaseController<
         this.#updateQueuedRequestCount();
 
         await waitForDequeue;
-      } else if (request.method !== 'eth_requestAccounts') {
+      } 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)

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,49 +140,30 @@ 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();
-    const middleware = createQueuedRequestMiddleware({
-      enqueueRequest: mockEnqueueRequest,
-      useRequestQueue: () => true,
-    });
-    const request = {
-      ...getRequestDefaults(),
-      origin: 'exampleorigin.com',
-      method: 'eth_sendTransaction',
-    };
+  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,
+      };
 
-    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({

packages/queued-request-controller/src/QueuedRequestMiddleware.ts

@@ -3,24 +3,12 @@ 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) => {
-  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);
 };
 
 /**

packages/queued-request-controller/src/constants.ts

@@ -0,0 +1,34 @@
+/**
+ * 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',
+];