fix: removes extra domains from `SelectedNetworkController` domain state by adonesky1 · Pull Request #23130 · MetaMask/metamask-extension

adonesky1 · 2024-02-22T18:39:38Z

Bumps to the the latest version of @metamask/selected-network-controller which fixes issue where the SelectedNetworkController adds any and all domains the user visits to its domains state whether or not the user has connected to these sites.
Adds a migration to clean up the undesired state caused by this bug.
Adds logic to listen for updates to permissions state and add newly connected/permissioned dapps to domains state in the SelectedNetworkController. I've created a ticket here to move this logic into the SelectedNetworkController itself.
Adds logic to couple the useRequestQueue feature flag with the perDomainNetwork feature flag. I've created a ticket here to remove the latter state since it is redundant and requires keeping these two booleans in sync.
Removes calls to setNetworkClientIdForMetamask since the latest version of @metamask/selected-network-controller removes this method (and the metamask domain from the domains state entirely).
Adds logic to use provider and blockTracker from the SelectedNetworkController when feature flag is on and connected origin is in domains state (which can only happen when the dapp is connected) in setupProviderEngine for RPC middleware stack.
Fixes a bug where if a dapp prompts an addEthereumChain request and the user then accepts the subsequent switchEthereumChain call that is chained on, the networkClientID set for that dapp in the SelectedNetworkController domains state will not be updated.

Manual Testing Steps

well first you need to comment out some code that is causing a separate bug (found on develop) but one that will make manually testing this a nightmare.

Go to settings > experimental > Select networks for each site and toggle this setting on
Put the globally selected network to ethereum mainnet
Connect to uniswap
Connect to pancakeswap
Vist Aave but don't connect the wallet
Open devtools in the wallet UI and run the follow command in the console:

const { metamask } = await window.stateHooks.getCleanAppState();
console.log(metamask.domains)

You should see the following log:

{https://app.uniswap.org: 'mainnet', https://pancakeswap.finance: 'mainnet'}

Now call switchEthereumChain on one of the connected dapps (they each have UI elements that do this)
Now go back to the other dapp (where the chain hasn't been switched) and initiate a transaction
You should receive a switchEthereumChain confirmation back to Ethereum Mainnet before the transaction confirmation is show.

github-actions · 2024-02-22T18:39:51Z

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.

adonesky1 · 2024-02-22T18:41:29Z

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

this wasn't being used?

ui/components/ui/new-network-info/new-network-info.js

adonesky1 · 2024-02-22T20:19:08Z

app/scripts/metamask-controller.js

this is no longer necessary because SelectedNetworkController no longer keeps track of the globally selectedNetworkClientId

adonesky1 · 2024-02-22T20:19:16Z

app/scripts/metamask-controller.js

this is no longer necessary because SelectedNetworkController no longer keeps track of the globally selectedNetworkClientId

adonesky1 · 2024-02-22T20:31:11Z

@metamaskbot update-policies

metamaskbot · 2024-02-22T20:38:39Z

No policy changes

metamaskbot · 2024-02-22T21:24:02Z

Builds ready [90af9d5]

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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1760 ± 88 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	134	326	195	53	25
		domContentLoaded	9	79	37	21	10
		load	1508	2401	1760	184	88
		domInteractive	9	79	37	21	10

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

background: 1.88 KiB (0.05%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

app/scripts/metamask-controller.js

adonesky1 · 2024-02-22T23:08:59Z

app/scripts/metamask-controller.js

we should probably just remove this redundant perDomainNetwork state from the SelectedNetworkController and just query for the useRequestQueue state on the PreferencesController wherever we need it. But this will require another update to the SelectedNetworkController and should be done in a separate PR.

This dedupe is happening here btw: MetaMask/core#3989

app/scripts/metamask-controller.js

adonesky1 · 2024-02-23T16:35:43Z

app/scripts/lib/setupSentry.js

    useRequestQueue: true,
  },
-  SelectedNetworkController: { domains: true, perDomainNetwork: false },
+  SelectedNetworkController: { domains: false, perDomainNetwork: true },


We do not want to include domains in our sentry reports since it is potentially sensitive and violates our goal to preserve privacy, but we do want to include the perDomainNetwork toggle since it is not sensitive and potentially useful for diagnosing bugs.

adonesky1 · 2024-02-26T17:41:24Z

app/scripts/metamask-controller.js

+          // When permissions are added for a given origin we need to
+          // set the network client id for that origin to the current
+          // globally selected network client id.
+          if (accounts.length > 0) {
+            // TODO add this logic directly in the SelectedNetworkController
+            this.selectedNetworkController.setNetworkClientIdForDomain(
+              origin,
+              this.networkController.state.selectedNetworkClientId,
+            );
+          } else {
+            // TODO remove domain from selectedNetworkController when permissions are removed
+            // Add this logic directly in the SelectedNetworkController
+          }


There is a WIP PR against the SelectedNetworkController to pull this logic into that controller instead: MetaMask/core#3969

codecov · 2024-02-26T23:43:45Z

Codecov Report

Attention: Patch coverage is 71.42857% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 68.59%. Comparing base (63fb3d4) to head (3e67f01).
Report is 1 commits behind head on develop.

Files	Patch %	Lines
app/scripts/metamask-controller.js	72.22%	10 Missing ⚠️
...ethod-middleware/handlers/switch-ethereum-chain.js	16.67%	5 Missing ⚠️
...c-method-middleware/handlers/add-ethereum-chain.js	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #23130      +/-   ##
===========================================
- Coverage    68.61%   68.59%   -0.02%     
===========================================
  Files         1098     1099       +1     
  Lines        43101    43141      +40     
  Branches     11513    11539      +26     
===========================================
+ Hits         29571    29589      +18     
- Misses       13530    13552      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-02-26T23:48:54Z

Builds ready [a72e1c7]

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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1254 ± 468 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	78	422	161	71	34
		domContentLoaded	15	198	50	43	21
		load	62	3241	1254	975	468
		domInteractive	15	197	50	43	21

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

background: 1.97 KiB (0.05%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

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

Gudahtt · 2024-03-01T14:09:11Z

The diff looks good! I can re-review when CI passes. It looks like at least one of the CI failures is related to these changes, the e2e test failure.

metamaskbot · 2024-03-01T17:10:11Z

Builds ready [a9885a5]

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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (889 ± 433 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	223	119	45	21
		domContentLoaded	10	78	29	19	9
		load	52	2381	889	903	433
		domInteractive	10	78	29	19	9

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

background: 2.95 KiB (0.08%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

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

metamaskbot · 2024-03-01T19:38:37Z

Builds ready [bbc2f17]

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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (886 ± 393 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	59	201	109	36	17
		domContentLoaded	9	63	26	17	8
		load	47	1864	886	819	393
		domInteractive	9	63	26	17	8

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

background: 2.95 KiB (0.08%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

Gudahtt

LGTM!

metamaskbot · 2024-03-01T20:18:37Z

Builds ready [3e67f01]

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, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1340 ± 305 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	87	235	141	44	21
		domContentLoaded	13	85	39	24	11
		load	83	1931	1340	635	305
		domInteractive	13	85	39	24	11

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

background: 2.95 KiB (0.08%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

adonesky1 force-pushed the ad/bump-selected-network-controller-v8.0.0 branch from 7041cee to f9d191b Compare February 22, 2024 18:40

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 22, 2024

adonesky1 commented Feb 22, 2024

View reviewed changes

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

Copy link
Copy Markdown

Contributor Author

adonesky1 Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this wasn't being used?

adonesky1 commented Feb 22, 2024

View reviewed changes

ui/components/ui/new-network-info/new-network-info.js Outdated Show resolved Hide resolved

adonesky1 changed the title ~~Bump @metamask/selected-network-controller to v8.0.0, fix issue where…~~ Bump @metamask/selected-network-controller to v8.0.0 Feb 22, 2024

adonesky1 changed the title ~~Bump @metamask/selected-network-controller to v8.0.0~~ Fix: bump @metamask/selected-network-controller to v8.0.0 Feb 22, 2024

adonesky1 force-pushed the ad/bump-selected-network-controller-v8.0.0 branch from b527a23 to 495f622 Compare February 22, 2024 19:54

adonesky1 commented Feb 22, 2024

View reviewed changes

adonesky1 added team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead team-core-extension-ux Core Extension UX team and removed INVALID-PR-TEMPLATE PR's body doesn't match template labels Feb 22, 2024

adonesky1 commented Feb 22, 2024

View reviewed changes

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

adonesky1 commented Feb 22, 2024

View reviewed changes

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 22, 2024

jiexi reviewed Feb 22, 2024

View reviewed changes

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

adonesky1 force-pushed the ad/bump-selected-network-controller-v8.0.0 branch from 01ef225 to a44307e Compare February 23, 2024 16:33

adonesky1 commented Feb 23, 2024

View reviewed changes

adonesky1 commented Feb 26, 2024

View reviewed changes

adonesky1 marked this pull request as ready for review February 27, 2024 01:48

adonesky1 requested a review from a team as a code owner February 27, 2024 01:48

adonesky1 changed the title ~~Fix: bump @metamask/selected-network-controller to v8.0.0~~ Chore: bump @metamask/selected-network-controller to v8.0.0 Feb 27, 2024

adonesky1 changed the title ~~Chore: bump @metamask/selected-network-controller to v8.0.0~~ fix: removes extra domains from SelectedNetworkController domain state Feb 27, 2024

adonesky1 requested a review from jiexi February 27, 2024 01:52

adonesky1 added 3 commits February 29, 2024 14:44

fix: hasPermission -> hasPermissions

2228ece

Merge branch 'develop' into ad/bump-selected-network-controller-v8.0.0

cd1cab4

fix e2e test

85a1d28

tmashuang reviewed Feb 29, 2024

View reviewed changes

test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js Outdated Show resolved Hide resolved

tmashuang reviewed Feb 29, 2024

View reviewed changes

test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js Outdated Show resolved Hide resolved

cleanup

399971c

adonesky1 added 2 commits March 1, 2024 08:15

Merge branch 'develop' into ad/bump-selected-network-controller-v8.0.0

1b64aee

Merge branch 'develop' into ad/bump-selected-network-controller-v8.0.0

a9885a5

adonesky1 requested review from Gudahtt, jiexi and tmashuang March 1, 2024 17:06

jiexi reviewed Mar 1, 2024

View reviewed changes

test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js Outdated Show resolved Hide resolved

jiexi reviewed Mar 1, 2024

View reviewed changes

test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js Outdated Show resolved Hide resolved

remove unnecessary comments

11cb981

jiexi previously approved these changes Mar 1, 2024

View reviewed changes

adonesky1 dismissed jiexi’s stale review via 11cb981 March 1, 2024 18:35

tmashuang added 2 commits March 1, 2024 11:03

Test improvements

eda1f88

Remove debugging

bbc2f17

Merge branch 'develop' into ad/bump-selected-network-controller-v8.0.0

3e67f01

adonesky1 requested a review from jiexi March 1, 2024 20:14

Gudahtt approved these changes Mar 1, 2024

View reviewed changes

jiexi approved these changes Mar 1, 2024

View reviewed changes

adonesky1 merged commit 53b4c5d into develop Mar 1, 2024

adonesky1 deleted the ad/bump-selected-network-controller-v8.0.0 branch March 1, 2024 20:53

github-actions bot locked and limited conversation to collaborators Mar 1, 2024

metamaskbot added the release-11.13.0 Issue or pull request that will be included in release 11.13.0 label Mar 1, 2024

Conversation

adonesky1 commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 22, 2024

Uh oh!

adonesky1 Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adonesky1 Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

adonesky1 commented Feb 22, 2024

Uh oh!

metamaskbot commented Feb 22, 2024

Uh oh!

metamaskbot commented Feb 22, 2024

Uh oh!

Uh oh!

adonesky1 Feb 22, 2024

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adonesky1 Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Feb 26, 2024

Uh oh!

Uh oh!

Uh oh!

Gudahtt commented Mar 1, 2024

Uh oh!

metamaskbot commented Mar 1, 2024

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Mar 1, 2024

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adonesky1 commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading