fix: add `SelectedNetworkController` setNetworkClientId useRequestQueuePreference guard by jiexi · Pull Request #4388 · MetaMask/core

jiexi · 2024-06-06T23:33:44Z

Explanation

Our previous SelectedNetworkController fix that added a guard to permission state changes was not sufficient. This PR properly addresses the underlying issue of setting networkClientId for domains when the useRequestQueuePreference flag is false by moving the guard into the setNetworkClientId() method itself

References

Fixes: [Bug]: Cannot get the latest value after switching the network metamask-extension#25097

Changelog

`@metamask/selected-network-controller`

FIXED: setNetworkClientId() will now result in a noop if useRequestQueuePreference is false

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

…oller:stateChange handler

Gudahtt

LGTM! Left a couple of suggestions for additional tests though

Gudahtt · 2024-06-07T00:22:10Z

packages/selected-network-controller/tests/SelectedNetworkController.test.ts

  });

-  describe('It updates domain state when the network controller state changes', () => {
+  describe('networkController:stateChange and useRequestQueuePreference is true', () => {


Nit: We may want a test for the opposite condition (i.e. when useRequestQueue is false)

done here fd79acf

Gudahtt · 2024-06-07T00:23:13Z

packages/selected-network-controller/tests/SelectedNetworkController.test.ts


  describe('Constructor checks for domains in permissions', () => {
-    it('should set networkClientId for domains not already in state', async () => {
+    it('should set networkClientId for domains not already in state when useRequestQueuePreference is true', async () => {


Nit: Likewise here, we may want to test the opposite case

addressed here c30f5d8

) Core PR: MetaMask/core#4388 Patch Branch: [jl/patch-selected-network-controller-12.0.1-setNetworkClientId-guard](https://github.com/MetaMask/core/tree/jl/patch-selected-network-controller-12.0.1-setNetworkClientId-guard)

adonesky1 · 2024-06-07T14:48:46Z

packages/selected-network-controller/tests/SelectedNetworkController.test.ts

-    });
+
    describe('when the useRequestQueue is false', () => {
      describe('when the requesting domain is not metamask', () => {


nit we can probably get rid of this nested describe block when the requesting domain is not metamask since the test applies whether or not the request domain is MetaMask

done here 7d594d9

packages/selected-network-controller/src/SelectedNetworkController.ts

…kClientIdForDomain-guard

adonesky1

Both of my comments are non blocking

…spec

adonesky1 · 2024-06-07T18:41:03Z

packages/selected-network-controller/tests/SelectedNetworkController.test.ts

+        it('does not update the networkClientId for domains which were previously set to the deleted networkClientId', () => {
+          const { controller, messenger } = setup({
+            state: {
+              // normally there would not be any domains in state if useRequestQueuePreference is false


Lets remove this whole block now since this state shouldn't be possible?

adonesky1 · 2024-06-07T18:57:23Z

packages/selected-network-controller/tests/SelectedNetworkController.test.ts

-          domains: {
-            'existingdomain.com': 'initialNetworkId',
+    describe('when useRequestQueuePreference is false', () => {
+      it('should set networkClientId for domains not already in state', async () => {


So this seems like another one where there shouldn't be domains in state so maybe we don't need this test? But if we're going to have it the networkClientId for the existing domain should probably change?

But it seems confusing to have this test at all.

Sorry I misphrased my initial comment. This implication of this test is actually backwards. It should be should **not** set networkClientId for domains not already in state

This is the only one that I think is kindof egregious enough to block merging

addressed here d16bc7f

adonesky1 · 2024-06-07T18:58:29Z

packages/selected-network-controller/tests/SelectedNetworkController.test.ts

-      expect(controller.state.domains['existingdomain.com']).toBe(
-        'initialNetworkId',
-      );
+      it('should not modify domains already in state', async () => {


This seems like one where there shouldn't be domains in state so maybe we don't need this test

adonesky1 · 2024-06-07T19:01:03Z

packages/selected-network-controller/tests/SelectedNetworkController.test.ts


-      // Now, 'newdomain.com' should have the selectedNetworkClientId set
-      expect(controller.state.domains['newdomain.com']).toBe('mainnet');
+      it('should not modify domains already in state', async () => {


this test seems redundant since the previous one tests this condition too?

Seems like the previous test could be called "should set networkClientId for domains not already in state and not modify domains already in state"?

adonesky1 · 2024-06-07T19:46:31Z

packages/selected-network-controller/src/SelectedNetworkController.ts

+    if (!this.#useRequestQueuePreference) {
+      return;
+    }


nit this could be bundled with the snaps check below which returns

if (snapsPrefixes.some((prefix) => domain.startsWith(prefix)) || !this.#useRequestQueuePreference) { return; }

…kClientIdForDomain-guard

jiexi · 2024-06-10T19:34:06Z

apologies, accidentally comingled some changes intended for another branch

adonesky1

LGTM! I'll do some of the clean up I suggested here in follow up!

…work domain is incorrectly set (#25106) ## **Description** Pulls [this fix](MetaMask/core#4388) which adds a guard to the `setNetworkClientIdForDomain` method on the `SelectedNetworkController` to no longer add domains to domains state (nor create a selected network proxy for them) unless the `useRequestQueuePreference` flag is true Bumping the `@metamask/selected-network-controller` version required [bumping peer dependencies](https://github.com/MetaMask/core/blob/main/packages/selected-network-controller/CHANGELOG.md#changed): - `@metamask/network-controller` to v19.0.0 - An existing patch was repointed to this version. - `@metamask/permission-controller` to v10.0.0 -[ This required bumping its own peerDep](https://github.com/MetaMask/core/blob/main/packages/selected-network-controller/CHANGELOG.md#changed) `@metamask/approval-controller` to v7.0.0 [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25106?quickstart=1) ## **Related issues** See: https://consensys.slack.com/archives/C1L7H42BT/p1717512150509719 ## **Manual testing steps** 1. Open settings -> experimental and toggle of the `Select networks for each site` setting 2. Go to any site 3. Open the console and execute: ``` await window.ethereum.request({ "method": "eth_chainId", "params": [] }); ``` 4. See that it matches the globally selected network 5. Manually change the network with the network switcher 6. Go back to the site and execute the same `eth_chainId` script 7. See that the result has changed to the chainId you switched to 8. Now execute ``` await window.ethereum.request({ "method": "wallet_requestPermissions", "params": [ { "eth_accounts": {} } ] }); ``` and connect an account to the site 9. Repeat steps 1 - 7 10. The `eth_chainId` results should still match the globally selected network ## **Screenshots/Recordings**  ### **Before**  https://github.com/MetaMask/metamask-extension/assets/34557516/ff2f68ff-6ec8-495f-96c1-e938973ce59b ### **After**  https://github.com/MetaMask/metamask-extension/assets/34557516/6061f4e2-faab-48a5-8c55-3b2a4e3aa9f2 ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>

jiexi added 3 commits June 6, 2024 16:32

add useRequestQueuePreference guard to setNetworkClientIdForDomain

7622cf3

remove unnecessary useRequestQueuePreference guard in PermissionContr…

439f42c

…oller:stateChange handler

Fix specs

688ab9b

jiexi added the team-wallet-api-platform label Jun 6, 2024

jiexi marked this pull request as ready for review June 6, 2024 23:42

jiexi requested a review from a team June 6, 2024 23:42

jiexi mentioned this pull request Jun 7, 2024

Version v11.16.8 alternative - patch selected network controller MetaMask/metamask-extension#25127

Merged

Gudahtt previously approved these changes Jun 7, 2024

View reviewed changes

adonesky1 reviewed Jun 7, 2024

View reviewed changes

packages/selected-network-controller/src/SelectedNetworkController.ts Show resolved Hide resolved

Merge branch 'main' into jl/fix-selected-network-controller-setNetwor…

68b34e7

…kClientIdForDomain-guard

adonesky1 previously approved these changes Jun 7, 2024

View reviewed changes

Add networkController:stateChange useRequestQueuePreference is false …

fd79acf

…spec

jiexi dismissed stale reviews from adonesky1 and Gudahtt via fd79acf June 7, 2024 17:38

jiexi requested review from Gudahtt and adonesky1 June 7, 2024 18:07

adonesky1 reviewed Jun 7, 2024

View reviewed changes

jiexi closed this Jun 10, 2024

jiexi force-pushed the jl/fix-selected-network-controller-setNetworkClientIdForDomain-guard branch from 2afe972 to e1a71e5 Compare June 10, 2024 19:26

jiexi added 2 commits June 10, 2024 12:32

Restore test

18f8013

Merge branch 'main' into jl/fix-selected-network-controller-setNetwor…

a96da76

…kClientIdForDomain-guard

jiexi reopened this Jun 10, 2024

update spec description

d16bc7f

adonesky1 approved these changes Jun 10, 2024

View reviewed changes

jiexi merged commit 1900a9d into main Jun 10, 2024

jiexi deleted the jl/fix-selected-network-controller-setNetworkClientIdForDomain-guard branch June 10, 2024 21:21

adonesky1 mentioned this pull request Jun 12, 2024

fix: bump SelectedNetworkController to fix issue where selected network domain is incorrectly set MetaMask/metamask-extension#25106

Merged

7 tasks

Uh oh!

Conversation

jiexi commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Changelog

@metamask/selected-network-controller

Checklist

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiexi commented Jun 10, 2024

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jiexi commented Jun 6, 2024 •

edited

Loading

`@metamask/selected-network-controller`

adonesky1 Jun 7, 2024 •

edited

Loading

adonesky1 Jun 7, 2024 •

edited

Loading