feat: Add `QueuedRequestController` batching #22865

Gudahtt · 2024-02-08T01:25:02Z

Description

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.

Related issues

Fixes:

Manual testing steps

important make sure any dapp used for testing has a permission (eth_accounts for example), as well as the per-domain network feature flag turned on, then refresh the dapp before begining testing.

make sure to also test that everything works unchanged with the feature flag off

Turn on per-domain network feature flag
go to test dapp and connect
refresh the test dapp
go to any other website (msn.com for example)
in the devtools, run this:

await window.ethereum.request({
  "method": "wallet_requestPermissions",
  "params": [
    {
      "eth_accounts": {}
    }
  ]
});

go back to the testdapp tab, make 2 or 3 send transaction calls
ensure that the UI displays the number of transactions being processed, and allows you to go back and forward through the transactions.
before confirming or rejecting any of those transactions, switch to the tab with msn.ca and run this 2-3 times:

await window.ethereum.request({
  "method": "eth_sendTransaction",
  "params": [
    {
      "to": "0x77ff877eE79c3f8C5aD244c037e5580EdCC6eF2c",
      "from": "your address",
      "value": "0x1"
    }
  ]
});

ensure that the you are still only able to scrub through the 3 transactions. Reject each of them.
You should now see the next set of 3 (from the domain msn.com).

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Gudahtt · 2024-02-29T17:33:23Z

@metamaskbot update-policies

metamaskbot · 2024-02-29T17:41:13Z

Policies updated

lavamoat/browserify/main/policy.json

socket-security · 2024-03-12T20:00:52Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/queued-request-controller@0.6.1	Transitive: filesystem	`+5`	2.78 MB	metamaskbot

🚮 Removed packages: npm/@metamask/queued-request-controller@0.3.0

View full report↗︎

Gudahtt · 2024-03-12T20:03:55Z

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",


This is resulting in a bunch of peer dependency warnings

TODO: Validate that they're safe to ignore, or bump the other packages as well in the same PR

Seems like theres only the one warning about queued request controller using a different version of selected network controller:

@metamask/selected-network-controller is listed by your project with version 9.0.0, which doesn't satisfy what @metamask/queued-request-controller (p81a9c) requests (^10.0.0).

the main change in 10.x.x is the update to base controller. The API afaik is the same, the typings have changed a bit here and there, but we arent getting any type errors so I think we are good to go.

https://github.com/MetaMask/core/blob/main/packages/selected-network-controller/CHANGELOG.md#1000

Gudahtt · 2024-03-13T11:40:57Z

test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js

The test description here talks about showing network confirmations, but we don't show confirmations on switch anymore.

Perhaps we should update this test to show that the network is switching, and leave about the "confirmations" part.

The same goes for the other request queuing test suite as well

BelfordZ · 2024-03-19T18:45:01Z

update: Ive re-tested the 2nd issue and it doesn't seem to be a problem anymore. Not sure what was going on before, but it seems to work as expected now. (just make sure the dapp has permissions, otherwise everything is pretty messed up)

github-actions · 2024-03-19T19:08:16Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

BelfordZ · 2024-03-19T19:08:46Z

Rebased to resolve conflicts

BelfordZ · 2024-03-19T19:08:49Z

@metamaskbot update-policies

metamaskbot · 2024-03-19T19:16:42Z

Policies updated

jiexi · 2024-03-19T20:31:06Z

Scenario works for me

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.

BelfordZ · 2024-03-20T18:18:31Z

Rebased again!

metamaskbot · 2024-03-20T20:29:47Z

Builds ready [633db2b]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (474 ± 375 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	64	262	116	51	24
		domContentLoaded	9	70	26	18	9
		load	51	2299	474	781	375
		domInteractive	9	70	26	18	9

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 10.81 KiB (0.29%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-03-20T21:34:40Z

Builds ready [6a238e1]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (877 ± 516 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	61	290	124	54	26
		domContentLoaded	10	91	30	22	10
		load	48	2769	877	1076	516
		domInteractive	10	91	30	22	10

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 10.81 KiB (0.29%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

Gudahtt · 2024-03-21T12:24:06Z

I have re-reviewed the recent changes, LGTM!

Gudahtt force-pushed the test-queue-updates branch 3 times, most recently from 3729bc0 to bb0c1f8 Compare February 14, 2024 06:13