fix: issue where provider engine needs to rerun after permissions are granted to ensure correct proxies are assigned #23525

adonesky1 · 2024-03-15T16:02:46Z

Description

When the "Selected networks for each site" toggle is enabled, the wallet API should use an origin-specific chain for all interactions. However this doesn't work correctly if the connection status has changed since the last refresh. If you grant permissions to a site or revoke them (i.e. connect to or disconnect from a site), the site does not work as expected until after the site is refreshed.

Expected behavior
The wallet API should always use the dapp-selected chain for sites with permissions, and should use the globally-selected chain for sites without permissions. This should apply even if the permissions have changed since the page loaded.

Solution

This PR introduces changes made to @metamask/selected-network-controller to resolve the issue described above:

Previously the SelectedNetworkController only constructed proxies for domains that had permissions. Other domains have no associated proxy and the getProviderAndBlockTracker method would throw an error. This was problematic because we grab the network client for an origin a single time when constructing an RPC pipeline for that origin in the MetaMask extension. We don't re-create the RPC pipeline when permissions change. That means that the pipeline is setup with the wrong network client and cannot be updated. The following changes ensure seamlessly proxying calls during sessions where a dapp connects/disconnects and provides a path for clients to prune inactive proxies.

This domainProxyMap param now passed into SelectedNetworkController allows the client to handle pruning inactive proxies while the SelectedNetworkController handles adding entries - it can't handle removal, as it doesn't know which connections are active.

The extension passes in an instance of a newly created WeakStringMap class which allows for garbage collection of its data once no longer referenced.

Related issues

Fixes: #23509

Manual testing steps

The problem presented itself in two scenarios: when granting site permissions, and when revoking all site permissions.

When granting permissions:

Enable the "Selected networks for each site" toggle
Open a site with no permissions in one tab, and the fullscreen wallet UI in another
If the site was open already, hard-refresh before proceeding
Call eth_requestAccounts on the site, asking for the eth_accounts permissions
Approve the request
Call eth_chainId from the site, and see that it matches the wallet selected chain
Switch the selected chain in the fullscreen UI
Call eth_chainId from the site, and see that it is still on the same chain the wallet was connected to before switching

When revoking permissions:

Redo all the same steps for When granting permissions:
Open the browser-action popup, click on the three-vertical-dots menu, click "Connected sites", then disconnect from the site
Call eth_chainId from the site. See that the sites connected chain is now the same as the "globally selected chain" in the wallet

Screenshots/Recordings

Before

This recording shows both reproduction use cases (granting permissions, then revoking)

queue-selected-chain-bug-2024-03-14_19.49.19.webm

After

Screen.Recording.2024-04-08.at.11.33.40.AM.mov

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.

github-actions · 2024-03-15T16:02:59Z

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-03-27T21:12:41Z

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

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/queued-request-controller@0.7.0	None	`+2`	368 kB	metamaskbot
npm/@metamask/selected-network-controller@11.0.0	None	`+1`	177 kB	metamaskbot

🚮 Removed packages: npm/@metamask/network-controller@18.0.1, npm/@metamask/queued-request-controller@0.6.1, npm/@metamask/selected-network-controller@9.0.0

View full report↗︎

adonesky1 · 2024-03-28T13:34:50Z

@metamaskbot update-policies

metamaskbot · 2024-03-28T13:44:18Z

Policies updated

socket-security · 2024-03-28T13:45:46Z

👍 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↗︎

adonesky1 · 2024-03-28T14:21:35Z

@metamaskbot update-policies

metamaskbot · 2024-03-28T14:29:29Z

Policies updated

adonesky1 · 2024-04-08T18:04:32Z

@metamaskbot update-policies

metamaskbot · 2024-04-08T18:13:17Z

No policy changes

adonesky1 · 2024-04-08T21:04:20Z

@metamaskbot update-policies

metamaskbot · 2024-04-08T21:12:27Z

No policy changes

metamaskbot · 2024-04-08T21:31:28Z

Builds ready [a45f7e8]

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

Page Load Metrics (580 ± 419 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	64	328	114	59	28
		domContentLoaded	10	60	24	13	6
		load	53	2212	580	873	419
		domInteractive	10	60	24	13	6

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

background: 12.83 KiB (0.44%)
ui: 0 Bytes (0.00%)
common: 326 Bytes (0.01%)

adonesky1 · 2024-04-12T21:28:36Z

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

@@ -109,15 +109,13 @@ describe('Request Queuing for Multiple Dapps and Txs on different networks.', fu

        // Dapp one send tx
        await driver.switchToWindowWithUrl(DAPP_URL);
-        await driver.executeScript(`window.location.reload()`);


its no longer required to reload the page after connecting to ensure that the proxies are correctly synced

metamaskbot · 2024-04-12T21:52:31Z

Builds ready [292a4fe]

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

Page Load Metrics (726 ± 542 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	320	129	71	34
		domContentLoaded	11	112	28	23	11
		load	52	3165	726	1129	542
		domInteractive	10	112	27	23	11

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

background: 13.49 KiB (0.39%)
ui: 0 Bytes (0.00%)
common: 326 Bytes (0.01%)

app/scripts/lib/WeakRefObjectMap.test.ts

metamaskbot · 2024-04-12T22:44:38Z

Builds ready [2577090]

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

Page Load Metrics (1281 ± 573 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	69	379	149	74	36
		domContentLoaded	10	99	38	25	12
		load	55	3115	1281	1194	573
		domInteractive	10	99	38	25	12

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

background: 13.49 KiB (0.39%)
ui: 0 Bytes (0.00%)
common: 326 Bytes (0.01%)

…s-bug

metamaskbot · 2024-04-15T15:02:41Z

Builds ready [61586c9]

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

Page Load Metrics (1318 ± 648 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	81	377	159	85	41
		domContentLoaded	14	145	34	30	14
		load	59	3308	1318	1350	648
		domInteractive	14	145	34	30	14

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

background: 13.49 KiB (0.39%)
ui: 0 Bytes (0.00%)
common: 326 Bytes (0.01%)

app/scripts/lib/WeakRefObjectMap.ts

Gudahtt

LGTM! One more pending suggestion (re: improving types), but overall this looks great

metamaskbot · 2024-04-15T19:14:30Z

Builds ready [9592db0]

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

Page Load Metrics (1286 ± 641 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	76	381	139	71	34
		domContentLoaded	11	138	35	28	13
		load	63	3125	1286	1335	641
		domInteractive	11	138	35	28	13

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

background: 13.49 KiB (0.39%)
ui: 0 Bytes (0.00%)
common: 326 Bytes (0.01%)

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

metamaskbot · 2024-04-15T20:06:27Z

Builds ready [77f8100]

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

Page Load Metrics (954 ± 571 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	73	418	150	100	48
		domContentLoaded	9	68	24	16	8
		load	60	2830	954	1189	571
		domInteractive	9	68	24	16	8

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

background: 13.49 KiB (0.39%)
ui: 0 Bytes (0.00%)
common: 326 Bytes (0.01%)

adonesky1 force-pushed the ad/fix-selected-network-controller-proxies-bug branch from d1d17de to 8c41fcd Compare March 15, 2024 16:27

adonesky1 force-pushed the ad/fix-selected-network-controller-proxies-bug branch from dd8e36a to 389e639 Compare March 27, 2024 21:11

adonesky1 force-pushed the ad/fix-selected-network-controller-proxies-bug branch from 0e89e61 to 265e11f Compare March 28, 2024 14:02

adonesky1 force-pushed the ad/fix-selected-network-controller-proxies-bug branch 3 times, most recently from 1d15a0c to e39883f Compare April 4, 2024 19:01

adonesky1 force-pushed the ad/fix-selected-network-controller-proxies-bug branch from e39883f to c12992a Compare April 8, 2024 17:58

adonesky1 force-pushed the ad/fix-selected-network-controller-proxies-bug branch from c12992a to 90ee937 Compare April 8, 2024 20:15

adonesky1 marked this pull request as ready for review April 8, 2024 21:30

adonesky1 requested review from a team as code owners April 8, 2024 21:30

adonesky1 added team-wallet-api-platform area-multichain amon-hen-v1 Represents blocking issues for the release of Amon Hen labels Apr 8, 2024

adonesky1 assigned shanejonas, jiexi and BelfordZ Apr 8, 2024

adonesky1 commented Apr 12, 2024

View reviewed changes

add iterator tests

b1165e1

jiexi reviewed Apr 12, 2024

View reviewed changes

app/scripts/lib/WeakRefObjectMap.test.ts Show resolved Hide resolved

jiexi previously approved these changes Apr 12, 2024

View reviewed changes

Update app/scripts/lib/WeakRefObjectMap.test.ts

2577090

jiexi dismissed their stale review via 2577090 April 12, 2024 22:10

jiexi previously approved these changes Apr 12, 2024

View reviewed changes

Merge branch 'develop' into ad/fix-selected-network-controller-proxie…

61586c9

…s-bug

Gudahtt reviewed Apr 15, 2024

View reviewed changes

app/scripts/lib/WeakRefObjectMap.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 15, 2024

View reviewed changes

app/scripts/lib/WeakRefObjectMap.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 15, 2024

View reviewed changes

app/scripts/lib/WeakRefObjectMap.ts Outdated Show resolved Hide resolved

address feedback: update types, throw error instead of console warning

9592db0

adonesky1 dismissed jiexi’s stale review via 9592db0 April 15, 2024 18:49

Gudahtt reviewed Apr 15, 2024

View reviewed changes

app/scripts/lib/WeakRefObjectMap.ts Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Apr 15, 2024

View reviewed changes

Update app/scripts/lib/WeakRefObjectMap.ts

77f8100

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

adonesky1 dismissed Gudahtt’s stale review via 77f8100 April 15, 2024 19:44

adonesky1 requested review from jiexi and Gudahtt April 15, 2024 19:44

Gudahtt approved these changes Apr 15, 2024

View reviewed changes

jiexi approved these changes Apr 15, 2024

View reviewed changes

adonesky1 merged commit b1b8c08 into develop Apr 15, 2024
66 of 68 checks passed

adonesky1 deleted the ad/fix-selected-network-controller-proxies-bug branch April 15, 2024 20:05

github-actions bot locked and limited conversation to collaborators Apr 15, 2024

metamaskbot added the release-11.16.0 Issue or pull request that will be included in release 11.16.0 label Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: issue where provider engine needs to rerun after permissions are granted to ensure correct proxies are assigned #23525

fix: issue where provider engine needs to rerun after permissions are granted to ensure correct proxies are assigned #23525

adonesky1 commented Mar 15, 2024 •

edited

github-actions bot commented Mar 15, 2024

socket-security bot commented Mar 27, 2024 •

edited

adonesky1 commented Mar 28, 2024

metamaskbot commented Mar 28, 2024

socket-security bot commented Mar 28, 2024 •

edited

adonesky1 commented Mar 28, 2024

metamaskbot commented Mar 28, 2024

adonesky1 commented Apr 8, 2024

metamaskbot commented Apr 8, 2024

adonesky1 commented Apr 8, 2024

metamaskbot commented Apr 8, 2024

metamaskbot commented Apr 8, 2024

adonesky1 Apr 12, 2024

metamaskbot commented Apr 12, 2024

metamaskbot commented Apr 12, 2024

metamaskbot commented Apr 15, 2024

Gudahtt left a comment

metamaskbot commented Apr 15, 2024

metamaskbot commented Apr 15, 2024

fix: issue where provider engine needs to rerun after permissions are granted to ensure correct proxies are assigned #23525

fix: issue where provider engine needs to rerun after permissions are granted to ensure correct proxies are assigned #23525

Conversation

adonesky1 commented Mar 15, 2024 • edited

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Mar 15, 2024

socket-security bot commented Mar 27, 2024 • edited

adonesky1 commented Mar 28, 2024

metamaskbot commented Mar 28, 2024

socket-security bot commented Mar 28, 2024 • edited

adonesky1 commented Mar 28, 2024

metamaskbot commented Mar 28, 2024

adonesky1 commented Apr 8, 2024

metamaskbot commented Apr 8, 2024

adonesky1 commented Apr 8, 2024

metamaskbot commented Apr 8, 2024

metamaskbot commented Apr 8, 2024

adonesky1 Apr 12, 2024

Choose a reason for hiding this comment

metamaskbot commented Apr 12, 2024

metamaskbot commented Apr 12, 2024

metamaskbot commented Apr 15, 2024

Gudahtt left a comment

Choose a reason for hiding this comment

metamaskbot commented Apr 15, 2024

metamaskbot commented Apr 15, 2024

adonesky1 commented Mar 15, 2024 •

edited

socket-security bot commented Mar 27, 2024 •

edited

socket-security bot commented Mar 28, 2024 •

edited