feat: Add QueuedRequestController batching

The `QueuedRequestController` will now batch requests by origin. Requests from the same UI now get shown together, just as they were before this queuing feature was added. The only remaining difference is that we no longer show requests from different origins in the same batch, which we view as a security/usability improvement.
MetaMask · Mar 13, 2024 · daa17b5 · daa17b5
1 parent 9c3835e
commit daa17b5
Show file tree

Hide file tree

Showing 6 changed files with 82 additions and 80 deletions.
diff --git a/app/scripts/background.js b/app/scripts/background.js
@@ -797,6 +797,11 @@ export function setupController(
     updateBadge,
   );
 
+  controller.controllerMessenger.subscribe(
+    METAMASK_CONTROLLER_EVENTS.QUEUED_REQUEST_STATE_CHANGE,
+    updateBadge,
+  );
+
   controller.txController.initApprovals();
 
   /**
@@ -820,35 +825,19 @@ export function setupController(
   }
 
   function getUnapprovedTransactionCount() {
-    let count = controller.appStateController.waitingForUnlock.length;
+    let count =
+      controller.appStateController.waitingForUnlock.length +
+      controller.approvalController.getTotalApprovalCount();
+
     if (controller.preferencesController.getUseRequestQueue()) {
-      count += controller.queuedRequestController.length();
-    } else {
-      count += controller.approvalController.getTotalApprovalCount();
+      count += controller.queuedRequestController.state.queuedRequestCount;
     }
     return count;
   }
 
-  controller.controllerMessenger.subscribe(
-    'QueuedRequestController:countChanged',
-    (count) => {
-      updateBadge();
-      if (count > 0) {
-        triggerUi();
-      }
-    },
-  );
-
   notificationManager.on(
     NOTIFICATION_MANAGER_EVENTS.POPUP_CLOSED,
     ({ automaticallyClosed }) => {
-      if (controller.preferencesController.getUseRequestQueue()) {
-        // when the feature flag is on, rejecting unnapproved notifications in this way does nothing (since the controllers havent seen the requests yet)
-        // Also, the updating of badge / triggering of UI happens from the countChanged event when the feature flag is on, so we dont need that here either.
-        // The only thing that we might want to add here is possibly calling a method to empty the queue / do the same thing as rejecting all confirmed?
-        return;
-      }
-
       if (!automaticallyClosed) {
         rejectUnapprovedNotifications();
       } else if (getUnapprovedTransactionCount() > 0) {

diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
@@ -306,6 +306,7 @@ export const METAMASK_CONTROLLER_EVENTS = {
   UPDATE_BADGE: 'updateBadge',
   // TODO: Add this and similar enums to the `controllers` repo and export them
   APPROVAL_STATE_CHANGE: 'ApprovalController:stateChange',
+  QUEUED_REQUEST_STATE_CHANGE: 'QueuedRequestController:stateChange',
 };
 
 // stream channels
@@ -398,6 +399,11 @@ export default class MetamaskController extends EventEmitter {
     this.queuedRequestController = new QueuedRequestController({
       messenger: this.controllerMessenger.getRestricted({
         name: 'QueuedRequestController',
+        allowedActions: [
+          'NetworkController:getState',
+          'NetworkController:setActiveNetwork',
+          'SelectedNetworkController:getNetworkClientIdForDomain',
+        ],
       }),
     });
 
@@ -4783,7 +4789,9 @@ export default class MetamaskController extends EventEmitter {
     }
 
     const requestQueueMiddleware = createQueuedRequestMiddleware({
-      messenger: this.controllerMessenger,
+      enqueueRequest: this.queuedRequestController.enqueueRequest.bind(
+        this.queuedRequestController,
+      ),
       useRequestQueue: this.preferencesController.getUseRequestQueue.bind(
         this.preferencesController,
       ),

diff --git a/package.json b/package.json
@@ -286,7 +286,7 @@
     "@metamask/post-message-stream": "^8.0.0",
     "@metamask/ppom-validator": "^0.27.0",
     "@metamask/providers": "^14.0.2",
-    "@metamask/queued-request-controller": "^0.3.0",
+    "@metamask/queued-request-controller": "^0.6.0",
     "@metamask/rate-limit-controller": "^3.0.0",
     "@metamask/safe-event-emitter": "^2.0.0",
     "@metamask/scure-bip39": "^2.0.3",

diff --git a/test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js b/test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js
@@ -115,15 +115,8 @@ describe('Request Queuing for Multiple Dapps and Txs on different networks.', fu
         await driver.switchToWindowWithUrl(DAPP_ONE_URL);
         await driver.clickElement('#sendButton');
 
-        await driver.delay(largeDelayMs);
-
-        // First switch network confirmation
+        // First switch network
         await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
-        await driver.findClickableElement({
-          text: 'Switch network',
-          tag: 'button',
-        });
-        await driver.clickElement({ text: 'Switch network', tag: 'button' });
 
         // Wait for confirm tx after switch network confirmation.
         await driver.delay(largeDelayMs);
@@ -151,24 +144,11 @@ describe('Request Queuing for Multiple Dapps and Txs on different networks.', fu
         // TODO: Reload fix to have the confirmations show
         await driver.executeScript(`window.location.reload()`);
 
-        // Second Switch Network Confirmation
+        // Second Switch Network
         await driver.switchToWindowWithTitle(
           WINDOW_TITLES.ExtensionInFullScreenView,
         );
 
-        await driver.findElement({
-          css: '[data-testid="network-switch-from-network"]',
-          text: 'Localhost 8545',
-        });
-
-        await driver.findElement({
-          css: '[data-testid="network-switch-to-network"]',
-          text: 'Localhost 8546',
-        });
-
-        // Switch Network
-        await driver.clickElement({ text: 'Switch network', tag: 'button' });
-
         // Check for unconfirmed transaction in tx list
         await driver.wait(async () => {
           const unconfirmedTxes = await driver.findElements(

diff --git a/test/e2e/tests/request-queuing/switch-network.spec.js b/test/e2e/tests/request-queuing/switch-network.spec.js
@@ -81,27 +81,6 @@ describe('Request Queuing Switch Network on Dapp Send Tx while on different netw
         // Queue confirm tx should show switch chain first when on different network
         await driver.clickElement('#sendButton');
 
-        await switchToNotificationWindow(driver);
-
-        // Switch Chain Confirmation
-        await driver.findElement({
-          css: '[data-testid="network-switch-from-network"]',
-          text: 'Localhost 8546',
-        });
-
-        await driver.findElement({
-          css: '[data-testid="network-switch-to-network"]',
-          text: 'Localhost 8545',
-        });
-
-        // Confirm Switch Chain
-        await driver.findClickableElement({
-          text: 'Switch network',
-          tag: 'button',
-        });
-        await driver.clickElement({ text: 'Switch network', tag: 'button' });
-
-        // Wait for confirm tx after switch network confirmation.
         await driver.delay(regularDelayMs);
 
         await driver.waitUntilXWindowHandles(3);

diff --git a/yarn.lock b/yarn.lock
@@ -4012,6 +4012,16 @@ __metadata:
   languageName: node
   linkType: hard
 
+"@metamask/base-controller@npm:^5.0.0":
+  version: 5.0.0
+  resolution: "@metamask/base-controller@npm:5.0.0"
+  dependencies:
+    "@metamask/utils": "npm:^8.3.0"
+    immer: "npm:^9.0.6"
+  checksum: 84e1ab05cf759892ad54e0e9f758dae5771853f1100bd82a43f4aed5c3f711d72ad1a4d8ccf7a6f275fb7388f51652cee4df6299e5452bdeece693effa4de06f
+  languageName: node
+  linkType: hard
+
 "@metamask/browser-passworder@npm:^4.3.0":
   version: 4.3.0
   resolution: "@metamask/browser-passworder@npm:4.3.0"
@@ -4084,6 +4094,23 @@ __metadata:
   languageName: node
   linkType: hard
 
+"@metamask/controller-utils@npm:^9.0.0":
+  version: 9.0.0
+  resolution: "@metamask/controller-utils@npm:9.0.0"
+  dependencies:
+    "@ethereumjs/util": "npm:^8.1.0"
+    "@metamask/eth-query": "npm:^4.0.0"
+    "@metamask/ethjs-unit": "npm:^0.3.0"
+    "@metamask/utils": "npm:^8.3.0"
+    "@spruceid/siwe-parser": "npm:1.1.3"
+    "@types/bn.js": "npm:^5.1.5"
+    bn.js: "npm:^5.2.1"
+    eth-ens-namehash: "npm:^2.0.8"
+    fast-deep-equal: "npm:^3.1.3"
+  checksum: 960af91b47b8006a82c0ee456041c2543d9ea4acfb5acc1b470a32436ea251380f00f840b108dcbf4705c7a41159c21dca4348e4100a319c1174fe5111b96037
+  languageName: node
+  linkType: hard
+
 "@metamask/design-tokens@npm:^1.12.0":
   version: 1.13.0
   resolution: "@metamask/design-tokens@npm:1.13.0"
@@ -4574,6 +4601,17 @@ __metadata:
   languageName: node
   linkType: hard
 
+"@metamask/json-rpc-engine@npm:^8.0.0":
+  version: 8.0.0
+  resolution: "@metamask/json-rpc-engine@npm:8.0.0"
+  dependencies:
+    "@metamask/rpc-errors": "npm:^6.2.1"
+    "@metamask/safe-event-emitter": "npm:^3.0.0"
+    "@metamask/utils": "npm:^8.3.0"
+  checksum: a1fe17c1443b14b777a4b31d61fad61530675252b21fc3bec5626625428f6e4796fdb1ce82540414e3569df9fb8884cd872e90aa3adf2d8efb4f7632766f9ce3
+  languageName: node
+  linkType: hard
+
 "@metamask/json-rpc-middleware-stream@npm:^6.0.2":
   version: 6.0.2
   resolution: "@metamask/json-rpc-middleware-stream@npm:6.0.2"
@@ -5037,21 +5075,20 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@metamask/queued-request-controller@npm:^0.3.0":
-  version: 0.3.0
-  resolution: "@metamask/queued-request-controller@npm:0.3.0"
+"@metamask/queued-request-controller@npm:^0.6.0":
+  version: 0.6.0
+  resolution: "@metamask/queued-request-controller@npm:0.6.0"
   dependencies:
-    "@metamask/base-controller": "npm:^4.0.1"
-    "@metamask/controller-utils": "npm:^8.0.1"
-    "@metamask/json-rpc-engine": "npm:^7.3.1"
-    "@metamask/rpc-errors": "npm:^6.1.0"
-    "@metamask/swappable-obj-proxy": "npm:^2.1.0"
-    "@metamask/utils": "npm:^8.2.0"
+    "@metamask/base-controller": "npm:^5.0.0"
+    "@metamask/controller-utils": "npm:^9.0.0"
+    "@metamask/json-rpc-engine": "npm:^8.0.0"
+    "@metamask/rpc-errors": "npm:^6.2.1"
+    "@metamask/swappable-obj-proxy": "npm:^2.2.0"
+    "@metamask/utils": "npm:^8.3.0"
   peerDependencies:
-    "@metamask/approval-controller": ^5.1.1
-    "@metamask/network-controller": ^17.1.0
-    "@metamask/selected-network-controller": ^6.0.0
-  checksum: 47988f3999f8ab45dfbbc8fde796ce4c8ebc405abb79bc165dc1faf777db6190942e220df76e0a7d0355d8de5a635cc41089fc75825eb4b6047ebf1c85b44e6a
+    "@metamask/network-controller": ^18.0.0
+    "@metamask/selected-network-controller": ^10.0.0
+  checksum: afd1df8fc7264ede3fc26cfedefaa75eb4cc40a3617b1597d4c202aecdcbf3189d656ad22bd389fe9bc7322d856f819a39b7aa708003ccd0e8951da4e844f9d3
   languageName: node
   linkType: hard
 
@@ -8952,6 +8989,15 @@ __metadata:
   languageName: node
   linkType: hard
 
+"@types/bn.js@npm:^5.1.5":
+  version: 5.1.5
+  resolution: "@types/bn.js@npm:5.1.5"
+  dependencies:
+    "@types/node": "npm:*"
+  checksum: 9719330c86aeae0a6a447c974cf0f853ba3660ede20de61f435b03d699e30e6d8b35bf71a8dc9fdc8317784438e83177644ba068ed653d0ae0106e1ecbfe289e
+  languageName: node
+  linkType: hard
+
 "@types/body-parser@npm:*":
   version: 1.19.2
   resolution: "@types/body-parser@npm:1.19.2"
@@ -24879,7 +24925,7 @@ __metadata:
     "@metamask/post-message-stream": "npm:^8.0.0"
     "@metamask/ppom-validator": "npm:^0.27.0"
     "@metamask/providers": "npm:^14.0.2"
-    "@metamask/queued-request-controller": "npm:^0.3.0"
+    "@metamask/queued-request-controller": "npm:^0.6.0"
     "@metamask/rate-limit-controller": "npm:^3.0.0"
     "@metamask/safe-event-emitter": "npm:^2.0.0"
     "@metamask/scure-bip39": "npm:^2.0.3"