-
-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix SelectedNetworkController default metamask networkClientId #1757
Fix SelectedNetworkController default metamask networkClientId #1757
Conversation
Note that this requires SelectedNetworkController to be instantiated before NetworkController. An alternative solution is to change from
to
|
Another alternative is to instantiate SelectedNetworkController after NetworkController and then to call |
Would this work? Wouldn't we be updating the the metamask domain every time then and skipping setting other domains? Did you mean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sorry, you're correct. The if statement should be negated like you suggested @adonesky1 what approach do you think is preferable? The |
Hey @jiexi sorry for the delay but
I think this ^ approach is probably better |
packages/selected-network-controller/tests/SelectedNetworkController.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit otherwise LGTM
…roller.test.ts Co-authored-by: Alex Donesky <adonesky@gmail.com>
## Explanation Currently, the SelectedNetworkController does not set a value for the default `metamask` domain on initial `NetworkController:stateChange` event because it does not count `selectedNetworkId` as actually changed in the patch yet. This manifests itself in extension with the SelectedNetworkMiddleware setting undefined for the `req.networkClientId` attribute. ~~This PR adds one more check to the event listener to ensure that the first `NetworkController:stateChange` event is used to populate the networkClientId for the default `metamask` domain.~~ See resolution in comments ## References * Fixes MetaMask/MetaMask-planning#1370 ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/selected-network-controller` - **FIXED**: `setNetworkClientIdForDomain()` will now ignore the passed in domain value and set the networkClientId for the `metamask` domain instead when the `state.perDomainNetwork` flag is false (default) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>
## Explanation Currently, the SelectedNetworkController does not set a value for the default `metamask` domain on initial `NetworkController:stateChange` event because it does not count `selectedNetworkId` as actually changed in the patch yet. This manifests itself in extension with the SelectedNetworkMiddleware setting undefined for the `req.networkClientId` attribute. ~~This PR adds one more check to the event listener to ensure that the first `NetworkController:stateChange` event is used to populate the networkClientId for the default `metamask` domain.~~ See resolution in comments ## References * Fixes MetaMask/MetaMask-planning#1370 ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/selected-network-controller` - **FIXED**: `setNetworkClientIdForDomain()` will now ignore the passed in domain value and set the networkClientId for the `metamask` domain instead when the `state.perDomainNetwork` flag is false (default) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>
## Explanation Currently, the SelectedNetworkController does not set a value for the default `metamask` domain on initial `NetworkController:stateChange` event because it does not count `selectedNetworkId` as actually changed in the patch yet. This manifests itself in extension with the SelectedNetworkMiddleware setting undefined for the `req.networkClientId` attribute. ~~This PR adds one more check to the event listener to ensure that the first `NetworkController:stateChange` event is used to populate the networkClientId for the default `metamask` domain.~~ See resolution in comments ## References * Fixes MetaMask/MetaMask-planning#1370 ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/selected-network-controller` - **FIXED**: `setNetworkClientIdForDomain()` will now ignore the passed in domain value and set the networkClientId for the `metamask` domain instead when the `state.perDomainNetwork` flag is false (default) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Alex Donesky <adonesky@gmail.com>
Explanation
Currently, the SelectedNetworkController does not set a value for the default
metamask
domain on initialNetworkController:stateChange
event because it does not countselectedNetworkId
as actually changed in the patch yet. This manifests itself in extension with the SelectedNetworkMiddleware setting undefined for thereq.networkClientId
attribute.This PR adds one more check to the event listener to ensure that the firstSee resolution in commentsNetworkController:stateChange
event is used to populate the networkClientId for the defaultmetamask
domain.References
Changelog
@metamask/selected-network-controller
setNetworkClientIdForDomain()
will now ignore the passed in domain value and set the networkClientId for themetamask
domain instead when thestate.perDomainNetwork
flag is false (default)Checklist