Fix issue with selectedNetworkMiddlware where all domains are added to state regardless of whether they have permissions
#3908
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
1067774 to
10d6e66
Compare
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
103055a to
99e2006
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
f7736f1 to
08978bf
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
3 tasks
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
067fd49 to
3905f51
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
MajorLift
reviewed
Feb 9, 2024
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
adonesky1
commented
Feb 9, 2024
packages/selected-network-controller/tests/SelectedNetworkMiddleware.test.ts
Show resolved
Hide resolved
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
57aad5e to
b790270
Compare
3 tasks
MajorLift
reviewed
Feb 15, 2024
packages/selected-network-controller/src/SelectedNetworkController.ts
Outdated
Show resolved
Hide resolved
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
1a0f393 to
3b3808f
Compare
Gudahtt
reviewed
Feb 19, 2024
packages/selected-network-controller/src/SelectedNetworkController.ts
Outdated
Show resolved
Hide resolved
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
88b679e to
ecb5e72
Compare
Gudahtt
reviewed
Feb 20, 2024
…electedNetworkMiddleware. Fallback to metamask domain networkClientId to add to the req object.
Cleans up and simplifies
adonesky1
force-pushed
the
ad/fix-selected-network-middleware
branch
from
a209931 to
a71ecdd
Compare
adonesky1
commented
Feb 20, 2024
Comment on lines
-232
to
-238
| Object.keys(this.state.domains).forEach((domain) => { | ||
| // when perDomainNetwork is false, getNetworkClientIdForDomain always returns the networkClientId for the domain 'metamask' | ||
| this.setNetworkClientIdForDomain( | ||
| domain, | ||
| this.getNetworkClientIdForDomain(domain), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
No need to update the domain state when the perDomainNetwork flag is toggled on or off, we can just reference this flag in the relevant methods to determine which networkClient to access in either case.
adonesky1
requested review from
jiexi,
Gudahtt,
MajorLift,
shanejonas and
BelfordZ
mcmire
reviewed
Feb 21, 2024
packages/selected-network-controller/src/SelectedNetworkController.ts
Outdated
Show resolved
Hide resolved
|
Changes make sense. |
jiexi
approved these changes
Feb 21, 2024
mcmire
approved these changes
Feb 21, 2024
Merged
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@metamask/selected-network-controller
Changed
selectedNetworkMiddlewareto set a defaultnetworkClientIdfor the requesting origin when not already set. Now if nonetworkClientIdis already set for the requesting origin, the middleware will not add the origin todomainsstate but will add thenetworkClientIdcurrently set for theselectedNetworkClientfrom theNetworkControllerto the request object.setNetworkClientIdForDomainnow throws an error if passedmetamaskas its first (domain) argumentsetNetworkClientIdForDomainnow includes a check that the requestingdomainhas already been granted permissions in thePermissionsControllerbefore adding it todomainsstate and throws an error if the domain does not have permissions.domainsstate now no longer contains ametamaskkey.getProviderAndBlockTrackernow throws an error if called with any domain while theperDomainNetworkflag is false.(These changes help fix an issue where the
SelectedNetworkControlleradds any and all domains the user visits to its domains state whether or not the user has connected to these sites.)Currently can be e2e tested on this WIP integration branch: MetaMask/metamask-extension#22860