feat: Bump `QueuedRequestController` from `^2.0.0` to `^7.0.0` by jiexi · Pull Request #28090 · MetaMask/metamask-extension

jiexi · 2024-10-24T20:55:49Z

Description

Bumps @metamask/queued-request-controller to fix queueing issue with Chain Permission wallet_switchEthereumChain and wallet_addEthereumChain when switching to a previously permitted chain and with wallet_addEthereumChain not being enqueued when it still should be.

Related issues

Related: MetaMask/core#4846
Fixes: #28101
Fixes: #27977
Fixes: #28102

Manual testing steps

The easiest way to test this would be a combination of using the test dapp and the following request to switch chains

await window.ethereum.request({
 "method": "wallet_switchEthereumChain",
 "params": [
  {
    chainId: "0x1"
  }
],
});

The behaviors you should see include:
One dapp:

On a dapp permissioned for chain A and B, on chain A, queue up several send transactions, then use wallet_switchEthereumChain to switch to chain B. The send transactions should NOT get cleared immediately after requesting the chain switch. Chain switch should NOT happen until the previous approvals are approved/rejected.
On a dapp permissioned for chain A and B, on chain A, queue up one send transaction, then use wallet_switchEthereumChain to switch to chain B, then queue up several more send transactions. Reject/approve the first transaction. Afterwards, you should see chain B as the active chain for the dapp, and all subsequent approvals cleared/rejected automatically.
On a dapp permissioned for ONLY chain A, on chain A, queue up one send transaction, then use wallet_switchEthereumChain to switch to chain B, then queue up several more send transactions. Reject/approve the first transaction. Afterwards, you should an approval prompt for adding chain B. If you approve it, the dapp should then be on chain B, with all subsequent approvals cleared/rejected. If you disapprove it, you should be prompted with the subsequent approvals.
On a dapp permissioned for ONLY chain A, on chain A, wallet_switchEthereumChain to switch to chain B, then queue up several more send transactions. Reject/approve the first transaction. Afterwards, you should an approval prompt for adding chain B. If you approve it, the dapp should then be on chain B, with all subsequent approvals cleared/rejected. If you disapprove it, you should be prompted with the subsequent approvals.

Two dapps:

On a dapp permissioned for chain A, on chain A, queue up several send transactions, On a separate dapp permissioned for chain A and B, on chain A, use wallet_switchEthereumChain to switch to chain B. The send transactions should NOT get cleared immediately after requesting the chain switch. Chain switch should NOT happen until the previous approvals are approved/rejected.
On a dapp permissioned for chain A and B, on chain A, queue up one send transaction. On a separate dapp permissioned for chain A and B, on chain A, use wallet_switchEthereumChain to switch to chain B. Then on the first dapp queue up several more send transactions. Reject/approve the first transaction. Afterwards, you should see chain B as the active chain for the second dapp, and then you should still be prompted with the subsequent approvals for the first dapp.
One one dapp, start a wallet_addEthereumChain for a chain that does not exist in the wallet and leave the approval alone. On a different dapp, do the same thing. Only the request from the first dapp should be accessible (i.e. no scrubbing between both of them). After rejecting the first request, the second request should then appear (which will look exactly the same of course). Wallet should not lock up if you repeat this and accept either of the requests

Screenshots/Recordings

Before

After

Screen.Recording.2024-10-24.at.1.57.19.PM.mov

Screen.Recording.2024-10-30.at.8.44.54.AM.mov

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

…ch-chain

github-actions · 2024-10-24T20:56:00Z

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.

socket-security · 2024-10-24T20:56:54Z

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

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/queued-request-controller@7.0.0	None	`0`	0 B

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

View full report↗︎

jiexi · 2024-10-24T21:04:25Z

@metamaskbot update-policies

socket-security · 2024-10-24T21:05:38Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js

metamaskbot · 2024-10-24T21:16:13Z

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

…ovalCount()

…-add-switch-chain' into jl/fix-queued-request-controller-add-switch-chain

…ch-chain

…stController.canRequestSwitchNetworkWithoutApproval

…ch-chain

metamaskbot · 2024-10-28T23:59:35Z

Builds ready [fbc055b]

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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1725 ± 58 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	307	2110	1647	328	157
		domContentLoaded	1508	2009	1702	106	51
		load	1517	2118	1725	121	58
		domInteractive	23	92	49	19	9
		backgroundConnect	9	109	23	23	11
		firstReactRender	45	185	87	31	15
		getState	5	51	14	16	8
		initialActions	0	0	0	0	0
		loadScripts	1084	1451	1256	87	42
		setupStore	10	64	22	18	9
		uiStartup	1664	2451	1921	169	81

Bundle size diffs [🚀 Bundle size reduced!]

background: -117.39 KiB (-2.65%)
ui: 0 Bytes (0.00%)
common: 37.09 KiB (0.47%)

… switch the network without prompting user approval (#4846) ## Explanation The release of Chain Permissions in Extension have changed the behavior of `wallet_switchEthereumChain` and `wallet_addEthereumChain` which breaks previous assumptions that the `QueuedRequestController` operated from. Specifically, it was previously assumed that the above methods would ALWAYS generate an approval that the user must accept or reject. This is important because the side effect of this behavior was that it was not possible for those two methods to immediately switch the chain if there happened to be existing pending approvals. The result of this new behavior was that pending approvals were being immediately cleared if one of the method calls above were to a chain that was already permitted which is the case where a network switch happens immediately without user interaction. This PR fixes this new case by: * explicitly waiting for any batch of processing requests to finish before proceeding with processing a request that has the potential to switch the globally selected network * processing a request that has the potential to switch the globally selected network by itself (rather than releasing it within a batch of several requests for the same origin) * flushing the request queue for the origin IF a request that has the potential to switch the globally selected network actually does change the globally selected network ## References Related: MetaMask/metamask-extension#28090 ## Changelog  ### `@metamask/queued-request-controller` - **BREAKING**: `QueuedRequestController` now requires the `canRequestSwitchNetworkWithoutApproval` callback in its constructor params. - **CHANGED**: `QueuedRequestController` now ensures that any queued requests for a origin are failed if a request that can switch the globally selected network without approval actually does change the globally selected network for that origin. - **CHANGED**: `QueuedRequestController` now ensures that a request that can switch the globally selected network without approval is queued behind any existing pending requests. ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Mark Stacey <markjstacey@gmail.com>

…ch-chain

metamaskbot · 2024-10-29T22:23:41Z

Builds ready [31d8bdc]

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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2011 ± 64 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1805	2277	2024	128	61
		domContentLoaded	1775	2201	1972	119	57
		load	1808	2277	2011	133	64
		domInteractive	21	91	49	20	9
		backgroundConnect	10	85	38	24	12
		firstReactRender	53	130	106	15	7
		getState	7	53	14	13	6
		initialActions	0	1	0	0	0
		loadScripts	1236	1685	1447	106	51
		setupStore	11	70	25	21	10
		uiStartup	2034	2484	2227	131	63

Bundle size diffs [🚀 Bundle size reduced!]

background: -79.84 KiB (-1.80%)
ui: 0 Bytes (0.00%)
common: 74.83 KiB (0.95%)

Gudahtt

LGTM!

app/scripts/metamask-controller.js

…ch-chain

… enqueued

…-add-switch-chain' into jl/fix-queued-request-controller-add-switch-chain

Gudahtt

LGTM!

…ch-chain

metamaskbot · 2024-10-30T16:55:18Z

Builds ready [f8ca8b5]

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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2196 ± 157 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	558	3006	2123	488	234
		domContentLoaded	1724	2947	2160	321	154
		load	1777	3016	2196	327	157
		domInteractive	29	109	57	22	10
		backgroundConnect	9	84	33	26	12
		firstReactRender	62	202	112	32	16
		getState	5	61	20	17	8
		initialActions	0	1	0	0	0
		loadScripts	1272	2383	1619	281	135
		setupStore	11	174	50	38	18
		uiStartup	1973	3231	2451	347	166

Bundle size diffs [🚀 Bundle size reduced!]

background: -79.76 KiB (-1.75%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

…ch-chain

metamaskbot · 2024-10-30T17:31:40Z

Builds ready [951bd80]

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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1939 ± 78 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	312	2059	1758	453	218
		domContentLoaded	1648	2462	1911	164	79
		load	1662	2468	1939	162	78
		domInteractive	21	173	48	33	16
		backgroundConnect	9	75	30	21	10
		firstReactRender	57	200	111	39	19
		getState	5	90	28	26	13
		initialActions	0	1	0	0	0
		loadScripts	1143	1919	1399	156	75
		setupStore	12	74	33	23	11
		uiStartup	1858	2759	2192	205	99

Bundle size diffs [🚀 Bundle size reduced!]

background: -79.76 KiB (-1.75%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

…ch-chain

Gudahtt · 2024-10-30T20:08:25Z

I have manually tested and confirmed that this fixes #28102 as well. I have added it to the description.

metamaskbot · 2024-10-30T20:19:33Z

Builds ready [ae936a0]

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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1963 ± 133 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1662	2893	1962	278	134
		domContentLoaded	1652	2672	1923	238	114
		load	1663	2900	1963	278	133
		domInteractive	16	80	49	16	8
		backgroundConnect	8	225	40	48	23
		firstReactRender	53	208	100	31	15
		getState	5	96	17	23	11
		initialActions	0	1	0	0	0
		loadScripts	1138	2126	1417	219	105
		setupStore	11	88	34	25	12
		uiStartup	1822	3651	2196	382	183

Bundle size diffs [🚀 Bundle size reduced!]

background: -79.76 KiB (-1.75%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

mcmire

Followed the manual testing steps and they all worked for me.

jiexi added 4 commits October 24, 2024 09:37

use preview build

ceb1596

use preview build

01ea856

commented out delay in switchChain as example

ab674ec

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

ba2f08c

…ch-chain

github-actions bot added the team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead label Oct 24, 2024

yarn dedupe

9ed1e11

jiexi commented Oct 24, 2024

View reviewed changes

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js Outdated Show resolved Hide resolved

Update LavaMoat policies

5ca61e4

jiexi added 3 commits October 24, 2024 14:26

bring in new preview build

d49a419

guard showApprovalRequest behind this.approvalController.getTotalAppr…

aa47ffc

…ovalCount()

Merge remote-tracking branch 'origin/jl/fix-queued-request-controller…

a8a7e87

…-add-switch-chain' into jl/fix-queued-request-controller-add-switch-chain

jiexi mentioned this pull request Oct 25, 2024

feat: Add handling to QueuedRequestController for requests that can switch the network without prompting user approval MetaMask/core#4846

Merged

4 tasks

jiexi added 10 commits October 25, 2024 10:29

use preview build f9d64175

f504449

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

f4a5e18

…ch-chain

bring in preview build (f504f91d)

00f3e66

cleanup comments

22b669f

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

2eb2366

…ch-chain

lint

7bca0ca

add methodsThatCanSwitchNetworkWithoutApproval and use in QueuedReque…

51f19af

…stController.canRequestSwitchNetworkWithoutApproval

lint

405d156

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

e2e2aaf

…ch-chain

preview build (6bb6f553)

fbc055b

jiexi mentioned this pull request Oct 29, 2024

Release/235.0.0 MetaMask/core#4867

Merged

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

31d8bdc

…ch-chain

Gudahtt previously approved these changes Oct 29, 2024

View reviewed changes

Gudahtt reviewed Oct 29, 2024

View reviewed changes

app/scripts/metamask-controller.js Show resolved Hide resolved

NidhiKJha previously approved these changes Oct 30, 2024

View reviewed changes

jiexi added 5 commits October 30, 2024 08:04

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

42a7d84

…ch-chain

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

6a33265

…ch-chain

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

9bafc7d

…ch-chain

Add wallet_addEthereumChain back into list of methods of that must be…

e8cd367

… enqueued

Merge remote-tracking branch 'origin/jl/fix-queued-request-controller…

7a85b89

…-add-switch-chain' into jl/fix-queued-request-controller-add-switch-chain

jiexi dismissed stale reviews from NidhiKJha and Gudahtt via 7a85b89 October 30, 2024 15:41

Gudahtt approved these changes Oct 30, 2024

View reviewed changes

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

f8ca8b5

…ch-chain

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

951bd80

…ch-chain

DDDDDanica approved these changes Oct 30, 2024

View reviewed changes

Merge branch 'develop' into jl/fix-queued-request-controller-add-swit…

ae936a0

…ch-chain

jiexi enabled auto-merge October 30, 2024 19:46

Gudahtt mentioned this pull request Oct 30, 2024

[Bug]: API Requests Unordered - Whenever I perform different wallet API requests from different dapps, those don't appear ordered sequentially based on the order of myactions #28102

Closed

jiexi added this pull request to the merge queue Oct 30, 2024

mcmire approved these changes Oct 30, 2024

View reviewed changes

Merged via the queue into develop with commit 956f99a Oct 30, 2024

jiexi deleted the jl/fix-queued-request-controller-add-switch-chain branch October 30, 2024 20:32

github-actions bot locked and limited conversation to collaborators Oct 30, 2024

metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 30, 2024

Conversation

jiexi commented Oct 24, 2024 • edited by Gudahtt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Oct 24, 2024

Uh oh!

socket-security bot commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiexi commented Oct 24, 2024

Uh oh!

socket-security bot commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Oct 24, 2024

Uh oh!

metamaskbot commented Oct 28, 2024

Uh oh!

metamaskbot commented Oct 29, 2024

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Oct 30, 2024

Uh oh!

metamaskbot commented Oct 30, 2024

Uh oh!

Gudahtt commented Oct 30, 2024

Uh oh!

metamaskbot commented Oct 30, 2024

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jiexi commented Oct 24, 2024 •

edited by Gudahtt

Loading

socket-security bot commented Oct 24, 2024 •

edited

Loading

socket-security bot commented Oct 24, 2024 •

edited

Loading