Skip to content

Fix SelectedNetworkController state corruption when networkClients are removed - #3926

Merged
adonesky1 merged 1 commit into
mainfrom
ad/fix-removing-networks
Feb 22, 2024
Merged

Fix SelectedNetworkController state corruption when networkClients are removed#3926
adonesky1 merged 1 commit into
mainfrom
ad/fix-removing-networks

Conversation

@adonesky1

@adonesky1adonesky1 commented Feb 16, 2024

Copy link
Copy Markdown
Contributor

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1964

@metamask/selected-network-controller

CHANGED

  • The SelectedNetworkController now listens for networkConfiguration removal events on the NetworkController and if a removed networkClientId matches the set networkClientId for any domains in its state, it updates them to the globally selected networkClientId and repoints the proxies accordingly.

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

@adonesky1
adonesky1 marked this pull request as ready for review February 16, 2024 21:27
@adonesky1
adonesky1 requested a review from a team as a code ownerFebruary 16, 2024 21:27
@adonesky1adonesky1 changed the title Ad/fix removing networksFix SelectedNetworkController state corruption when networkClients are removedFeb 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting idea to use patches for this. Might be more performant this way? I worry about the potential for a network configuration to be replaced by a different operation though, e.g. move or replace.

At the moment I think remove is the only possibility, but later with data syncing, I am not so sure.

Comparing each selected network client ID with each existing network might be safer.

@GudahttGudahttFeb 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could ensure this only runs for config changes by using a selector as well, e.g.

this.messagingSystem.subscribe(
'NetworkController:stateChange',
(networkConfigurations) => {
...
},
(state) => state.networkConfigurations,
)

Gudahtt
Gudahtt previously approved these changes Feb 16, 2024

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, good catch!

jiexi
jiexi previously approved these changes Feb 20, 2024
@adonesky1
adonesky1force-pushed the ad/fix-selected-network-middleware branch from a209931 to a71ecddCompareFebruary 20, 2024 19:29
@adonesky1
adonesky1force-pushed the ad/fix-removing-networks branch from 2baf760 to b6c560fCompareFebruary 20, 2024 20:10
Base automatically changed from ad/fix-selected-network-middleware to mainFebruary 22, 2024 00:14
@adonesky1
adonesky1 dismissed stale reviews from jiexi and GudahttFebruary 22, 2024 00:14

The base branch was changed.

…mains that were pointed at that networkClient are repointed to the globally selected network
@adonesky1
adonesky1force-pushed the ad/fix-removing-networks branch from b6c560f to 5652d07CompareFebruary 22, 2024 15:12
@adonesky1
adonesky1 merged commit 7af430d into mainFeb 22, 2024
@adonesky1
adonesky1 deleted the ad/fix-removing-networks branch February 22, 2024 15:41
@adonesky1adonesky1 mentioned this pull request Feb 22, 2024
adonesky1 added a commit that referenced this pull request Feb 22, 2024
# @metamask/selected-network-controller
## [8.0.0]
### Changed
- **BREAKING:** `setNetworkClientIdForDomain` now throws an error if
passed `metamask` as its first (`domain`) argument
([#3908](#3908)).
- **BREAKING:** `setNetworkClientIdForDomain` now includes a check that
the requesting `domain` has already been granted permissions in the
`PermissionsController` before adding it to `domains` state and throws
an error if the domain does not have permissions
([#3908](#3908)).
- **BREAKING:** the `domains` state now no longer contains a `metamask`
domain key Consumers should instead use the `selectedNetworkClientId`
from the `NetworkController` to get the selected network for the
`metamask` domain ([#3908](#3908)).
- **BREAKING:** `getProviderAndBlockTracker` now throws an error if
called with any domain while the `perDomainNetwork` flag is false.
Consumers should instead use the `provider` and `blockTracker` from the
`NetworkController` when the `perDomainNetwork` flag is false
([#3908](#3908)).
- **BREAKING:** `getProviderAndBlockTracker` now throws an error if
called with a domain that is not in domains state
([#3908](#3908)).
- **BREAKING:** `getNetworkClientIdForDomain` now returns the
`selectedNetworkClientId` for the globally selected network if the
`perDomainNetwork` flag is false and if the domain is not in the
`domains` state ([#3908](#3908)).
### Removed
- **BREAKING:** Remove logic in `selectedNetworkMiddleware` to set a
default `networkClientId` for the requesting origin when not already
set. Now if no `networkClientId` is already set for the requesting
origin, the middleware will not add the origin to `domains` state but
will add the `networkClientId` currently set for the
`selectedNetworkClient` from the `NetworkController` to the request
object ([#3908](#3908)).
### Fixed
- The `SelectedNetworkController` now listens for `networkConfiguration`
removal events on the `NetworkController` and if a removed
`networkClientId` matches the set `networkClientId` for any domains in
its state, it updates them to the globally selected `networkClientId`
and repoints the proxies accordingly.
([#3926](#3926))
---------
Co-authored-by: jiexi <jiexiluan@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@adonesky1@shanejonas@jiexi@Gudahtt