[queued-request-controller] Batch RPC requests (#3781)

## Explanation The `QueuedRequestController` will now batch RPC requests by origin. Each batch of requests will be processed in parallel, allowing existing features relating to groups of requests to function correctly with queuing enabled (e.g. the confirmation navigation buttons, and the "Reject all" button). The batching by origin also ensures that confirmations from different dapps are never displayed together in a group, which will help prevent users from confusing which dapp each confirmation is from. The meaning of `queuedRequestCount` has changed; it now represents the total number of queued requests, i.e. those that are queued but are **not** being processed. Previously it included processing requests as well. Given that the number of confirmations awaiting approval is already tracked elsewhere, it was not useful to double count them by including them in the queued request count as well. Additionally, when the `QueuedRequestController` requires the globally selected network to be changed, we now emit an event rather than triggering a confirmation. This aligns with recent designs for this use case. The actions `NetworkController:getNetworkConfigurationByNetworkClientId` and `ApprovalController:addRequest` were only used for triggering the switch network confirmation, so they are no longer needed now. They have been removed. This removes the last dependency of this controller on the `ApprovalController`, so it has been removed as a dependency as well. ## References Closes #3763 Fixes #3967 Closes #3983 ## Changelog ### `@metamask/queued-request-controller` #### Changed - **BREAKING**: The `QueuedRequestController` will now batch queued requests by origin - All of the requests in a single batch will be processed in parallel. - Requests get processed in order of insertion, even across origins/batches. - All requests get processed even in the event of preceding requests failing. - **BREAKING:** The `queuedRequestCount` state no longer includes requests that are currently being processed. It just counts requests that are queued. - **BREAKING:** The `QueuedRequestController` no longer triggers a confirmation when a network switch is needed - The network switch now happens automatically, with no confirmation. - A new `QueuedRequestController:networkSwitched` event has been added to communicate when this has happened. - The `QueuedRequestController` messenger no longer needs access to the actions `NetworkController:getNetworkConfigurationByNetworkClientId` and `ApprovalController:addRequest`. - The package `@metamask/approval-controller` has been completely removed as a dependency ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Elliot Winkler <[email protected]> Co-authored-by: legobeat <[email protected]> Co-authored-by: Alex Donesky <[email protected]>
MetaMask · Mar 8, 2024 · 8d74b81 · 8d74b81
1 parent 6263cca
commit 8d74b81
Show file tree

Hide file tree

Showing 8 changed files with 761 additions and 264 deletions.
diff --git a/packages/queued-request-controller/package.json b/packages/queued-request-controller/package.json
@@ -49,7 +49,6 @@
     "@metamask/utils": "^8.3.0"
   },
   "devDependencies": {
-    "@metamask/approval-controller": "^5.1.3",
     "@metamask/auto-changelog": "^3.4.4",
     "@metamask/network-controller": "^17.2.1",
     "@metamask/selected-network-controller": "^9.0.0",
@@ -66,7 +65,6 @@
     "typescript": "~4.8.4"
   },
   "peerDependencies": {
-    "@metamask/approval-controller": "^5.1.2",
     "@metamask/network-controller": "^17.2.0",
     "@metamask/selected-network-controller": "^9.0.0"
   },

diff --git a/packages/queued-request-controller/src/QueuedRequestController.test.ts b/packages/queued-request-controller/src/QueuedRequestController.test.ts
diff --git a/packages/queued-request-controller/src/QueuedRequestController.ts b/packages/queued-request-controller/src/QueuedRequestController.ts
@@ -1,16 +1,15 @@
-import type { AddApprovalRequest } from '@metamask/approval-controller';
 import type {
   ControllerGetStateAction,
   ControllerStateChangeEvent,
   RestrictedControllerMessenger,
 } from '@metamask/base-controller';
 import { BaseController } from '@metamask/base-controller';
-import { ApprovalType } from '@metamask/controller-utils';
 import type {
-  NetworkControllerGetNetworkConfigurationByNetworkClientId,
   NetworkControllerGetStateAction,
   NetworkControllerSetActiveNetworkAction,
 } from '@metamask/network-controller';
+import type { SelectedNetworkControllerGetNetworkClientIdForDomainAction } from '@metamask/selected-network-controller';
+import { createDeferredPromise } from '@metamask/utils';
 
 import type { QueuedRequestMiddlewareJsonRpcRequest } from './types';
 
@@ -36,6 +35,7 @@ export type QueuedRequestControllerEnqueueRequestAction = {
 };
 
 export const QueuedRequestControllerEventTypes = {
+  networkSwitched: `${controllerName}:networkSwitched` as const,
   stateChange: `${controllerName}:stateChange` as const,
 };
 
@@ -45,8 +45,14 @@ export type QueuedRequestControllerStateChangeEvent =
     QueuedRequestControllerState
   >;
 
+export type QueuedRequestControllerNetworkSwitched = {
+  type: typeof QueuedRequestControllerEventTypes.networkSwitched;
+  payload: [string];
+};
+
 export type QueuedRequestControllerEvents =
-  QueuedRequestControllerStateChangeEvent;
+  | QueuedRequestControllerStateChangeEvent
+  | QueuedRequestControllerNetworkSwitched;
 
 export type QueuedRequestControllerActions =
   | QueuedRequestControllerGetStateAction
@@ -55,8 +61,7 @@ export type QueuedRequestControllerActions =
 export type AllowedActions =
   | NetworkControllerGetStateAction
   | NetworkControllerSetActiveNetworkAction
-  | NetworkControllerGetNetworkConfigurationByNetworkClientId
-  | AddApprovalRequest;
+  | SelectedNetworkControllerGetNetworkClientIdForDomainAction;
 
 export type QueuedRequestControllerMessenger = RestrictedControllerMessenger<
   typeof controllerName,
@@ -71,28 +76,62 @@ export type QueuedRequestControllerOptions = {
 };
 
 /**
- * Controller for request queueing. The QueuedRequestController manages the orderly execution of enqueued requests
- * to prevent concurrency issues and ensure proper handling of asynchronous operations.
+ * A queued request.
+ */
+type QueuedRequest = {
+  /**
+   * The origin of the queued request.
+   */
+  origin: string;
+  /**
+   * A callback used to continue processing the request, called when the request is dequeued.
+   */
+  processRequest: (error: unknown) => void;
+};
+
+/**
+ * Queue requests for processing in batches, by request origin.
  *
- * @param options - The controller options, including the restricted controller messenger for the QueuedRequestController.
- * @param options.messenger - The restricted controller messenger that facilitates communication with the QueuedRequestController.
+ * Processing requests in batches allows us to completely separate sets of requests that originate
+ * from different origins. This ensures that our UI will not display those requests as a set, which
+ * could mislead users into thinking they are related.
  *
- * The QueuedRequestController maintains a count of enqueued requests, allowing you to monitor the queue's workload.
- * It processes requests sequentially, ensuring that each request is executed one after the other. The class offers
- * an `enqueueRequest` method for adding requests to the queue. The controller initializes with a count of zero and
- * registers message handlers for request enqueuing. It also publishes count changes to inform external observers.
+ * Queuing requests in batches also allows us to ensure the globally selected network matches the
+ * dapp-selected network, before the confirmation UI is rendered. This is important because the
+ * data shown on some confirmation screens is only collected for the globally selected network.
+ *
+ * Requests get processed in order of insertion, even across batches. All requests get processed
+ * even in the event of preceding requests failing.
  */
 export class QueuedRequestController extends BaseController<
   typeof controllerName,
   QueuedRequestControllerState,
   QueuedRequestControllerMessenger
 > {
-  private currentRequest: Promise<unknown> = Promise.resolve();
+  /**
+   * The origin of the current batch of requests being processed, or `undefined` if there are no
+   * requests currently being processed.
+   */
+  #originOfCurrentBatch: string | undefined;
 
   /**
-   * Constructs a QueuedRequestController, responsible for managing and processing enqueued requests sequentially.
-   * @param options - The controller options, including the restricted controller messenger for the QueuedRequestController.
-   * @param options.messenger - The restricted controller messenger that facilitates communication with the QueuedRequestController.
+   * The list of all queued requests, in chronological order.
+   */
+  #requestQueue: QueuedRequest[] = [];
+
+  /**
+   * The number of requests currently being processed.
+   *
+   * Note that this does not include queued requests, just those being actively processed (i.e.
+   * those in the "current batch").
+   */
+  #processingRequestCount = 0;
+
+  /**
+   * Construct a QueuedRequestController.
+   *
+   * @param options - Controller options.
+   * @param options.messenger - The restricted controller messenger that facilitates communication with other controllers.
    */
   constructor({ messenger }: QueuedRequestControllerOptions) {
     super({
@@ -111,118 +150,160 @@ export class QueuedRequestController extends BaseController<
 
   #registerMessageHandlers(): void {
     this.messagingSystem.registerActionHandler(
-      QueuedRequestControllerActionTypes.enqueueRequest,
+      `${controllerName}:enqueueRequest`,
       this.enqueueRequest.bind(this),
     );
   }
 
   /**
-   * Switch the current globally selected network if necessary for processing the given
-   * request.
+   * Process the next batch of requests.
+   *
+   * This will trigger the next batch of requests with matching origins to be processed. Each
+   * request in the batch is dequeued one at a time, in chronological order, but they all get
+   * processed in parallel.
+   *
+   * This should be called after a batch of requests has finished processing, if the queue is non-
+   * empty.
+   */
+  async #processNextBatch() {
+    const firstRequest = this.#requestQueue.shift() as QueuedRequest;
+    this.#originOfCurrentBatch = firstRequest.origin;
+    const batch = [firstRequest.processRequest];
+    while (this.#requestQueue[0]?.origin === this.#originOfCurrentBatch) {
+      const nextEntry = this.#requestQueue.shift() as QueuedRequest;
+      batch.push(nextEntry.processRequest);
+    }
+
+    // If globally selected network is different from origin selected network,
+    // switch network before processing batch
+    let networkSwitchError: unknown;
+    try {
+      await this.#switchNetworkIfNecessary();
+    } catch (error: unknown) {
+      networkSwitchError = error;
+    }
+
+    for (const processRequest of batch) {
+      processRequest(networkSwitchError);
+    }
+    this.#updateQueuedRequestCount();
+  }
+
+  /**
+   * Switch the globally selected network client to match the network
+   * client of the current batch.
    *
-   * @param request - The request currently being processed.
    * @throws Throws an error if the current selected `networkClientId` or the
    * `networkClientId` on the request are invalid.
    */
-  async #switchNetworkIfNecessary(
-    request: QueuedRequestMiddlewareJsonRpcRequest,
-  ) {
+  async #switchNetworkIfNecessary() {
+    // This branch is unreachable; it's just here for type reasons.
+    /* istanbul ignore next */
+    if (!this.#originOfCurrentBatch) {
+      throw new Error('Current batch origin must be initialized first');
+    }
+    const originNetworkClientId = this.messagingSystem.call(
+      'SelectedNetworkController:getNetworkClientIdForDomain',
+      this.#originOfCurrentBatch,
+    );
     const { selectedNetworkClientId } = this.messagingSystem.call(
       'NetworkController:getState',
     );
-    if (request.networkClientId === selectedNetworkClientId) {
+    if (originNetworkClientId === selectedNetworkClientId) {
       return;
     }
 
-    const toNetworkConfiguration = this.messagingSystem.call(
-      'NetworkController:getNetworkConfigurationByNetworkClientId',
-      request.networkClientId,
-    );
-    const fromNetworkConfiguration = this.messagingSystem.call(
-      'NetworkController:getNetworkConfigurationByNetworkClientId',
-      selectedNetworkClientId,
-    );
-    if (!toNetworkConfiguration) {
-      throw new Error(
-        `Missing network configuration for ${request.networkClientId}`,
-      );
-    } else if (!fromNetworkConfiguration) {
-      throw new Error(
-        `Missing network configuration for ${selectedNetworkClientId}`,
-      );
-    }
-
-    const requestData = {
-      toNetworkConfiguration,
-      fromNetworkConfiguration,
-    };
     await this.messagingSystem.call(
-      'ApprovalController:addRequest',
-      {
-        origin: request.origin,
-        type: ApprovalType.SwitchEthereumChain,
-        requestData,
-      },
-      true,
+      'NetworkController:setActiveNetwork',
+      originNetworkClientId,
     );
 
-    await this.messagingSystem.call(
-      'NetworkController:setActiveNetwork',
-      request.networkClientId,
+    this.messagingSystem.publish(
+      'QueuedRequestController:networkSwitched',
+      originNetworkClientId,
     );
   }
 
-  #updateCount(change: -1 | 1) {
+  /**
+   * Update the queued request count.
+   */
+  #updateQueuedRequestCount() {
     this.update((state) => {
-      state.queuedRequestCount += change;
+      state.queuedRequestCount = this.#requestQueue.length;
     });
   }
 
   /**
-   * Enqueues a new request for sequential processing in the request queue. This function manages the order of
-   * requests, ensuring they are executed one after the other to prevent concurrency issues and maintain proper
-   * execution flow.
+   * Enqueue a request to be processed in a batch with other requests from the same origin.
+   *
+   * We process requests one origin at a time, so that requests from different origins do not get
+   * interwoven, and so that we can ensure that the globally selected network matches the dapp-
+   * selected network.
+   *
+   * Requests get processed in order of insertion, even across origins/batches. All requests get
+   * processed even in the event of preceding requests failing.
    *
    * @param request - The JSON-RPC request to process.
-   * @param requestNext - A function representing the next steps for processing this request. It returns a promise that
-   * resolves when the request is complete.
-   * @returns A promise that resolves when the enqueued request and any subsequent asynchronous
-   * operations are fully processed. This allows you to await the completion of the enqueued request before continuing
-   * with additional actions. If there are multiple enqueued requests, this function ensures they are processed in
-   * the order they were enqueued, guaranteeing sequential execution.
+   * @param requestNext - A function representing the next steps for processing this request.
+   * @returns A promise that resolves when the given request has been fully processed.
    */
   async enqueueRequest(
     request: QueuedRequestMiddlewareJsonRpcRequest,
     requestNext: () => Promise<void>,
-  ) {
-    this.#updateCount(1);
-    if (this.state.queuedRequestCount > 1) {
-      try {
-        await this.currentRequest;
-      } catch (_error) {
-        // error ignored - this is handled in the middleware instead
-        this.#updateCount(-1);
-      }
+  ): Promise<void> {
+    if (this.#originOfCurrentBatch === undefined) {
+      this.#originOfCurrentBatch = request.origin;
     }
 
-    const processCurrentRequest = async () => {
-      try {
-        if (
-          request.method !== 'wallet_switchEthereumChain' &&
-          request.method !== 'wallet_addEthereumChain'
-        ) {
-          await this.#switchNetworkIfNecessary(request);
-        }
+    try {
+      // Queue request for later processing
+      // Network switch is handled when this batch is processed
+      if (
+        this.state.queuedRequestCount > 0 ||
+        this.#originOfCurrentBatch !== request.origin
+      ) {
+        const {
+          promise: waitForDequeue,
+          reject,
+          resolve,
+        } = createDeferredPromise({
+          suppressUnhandledRejection: true,
+        });
+        this.#requestQueue.push({
+          origin: request.origin,
+          processRequest: (error: unknown) => {
+            if (error) {
+              reject(error);
+            } else {
+              resolve();
+            }
+          },
+        });
+        this.#updateQueuedRequestCount();
 
+        await waitForDequeue;
+      } else {
+        // Process request immediately
+        // Requires switching network now if necessary
+        await this.#switchNetworkIfNecessary();
+      }
+      this.#processingRequestCount += 1;
+      try {
         await requestNext();
       } finally {
-        // The count is updated as part of the request processing to ensure
-        // that it has been updated before the next request is run.
-        this.#updateCount(-1);
+        this.#processingRequestCount -= 1;
       }
-    };
-
-    this.currentRequest = processCurrentRequest();
-    await this.currentRequest;
+      return undefined;
+    } finally {
+      if (this.#processingRequestCount === 0) {
+        this.#originOfCurrentBatch = undefined;
+        if (this.#requestQueue.length > 0) {
+          // The next batch is triggered here. We intentionally omit the `await` because we don't
+          // want the next batch to block resolution of the current request.
+          // eslint-disable-next-line @typescript-eslint/no-floating-promises
+          this.#processNextBatch();
+        }
+      }
+    }
   }
 }
diff --git a/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts b/packages/queued-request-controller/src/QueuedRequestMiddleware.test.ts
@@ -28,7 +28,7 @@ const getMockEnqueueRequest = () =>
       ReturnType<QueuedRequestControllerEnqueueRequestAction['handler']>,
       Parameters<QueuedRequestControllerEnqueueRequestAction['handler']>
     >()
-    .mockImplementation((_origin, requestNext) => requestNext());
+    .mockImplementation((_request, requestNext) => requestNext());
 
 describe('createQueuedRequestMiddleware', () => {
   it('throws if not provided an origin', async () => {

diff --git a/packages/queued-request-controller/src/index.ts b/packages/queued-request-controller/src/index.ts
@@ -3,6 +3,7 @@ export type {
   QueuedRequestControllerEnqueueRequestAction,
   QueuedRequestControllerGetStateAction,
   QueuedRequestControllerStateChangeEvent,
+  QueuedRequestControllerNetworkSwitched,
   QueuedRequestControllerEvents,
   QueuedRequestControllerActions,
   QueuedRequestControllerMessenger,