Skip to content
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 permission state change handler #4368

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jun 4, 2024

Explanation

Fixes a bug with SelectedNetworkController where it incorrectly sets the networkClientId for a newly permitted domain when the useRequestQueue flag is set to false.

References

See: MetaMask/metamask-extension#25046

Changelog

@metamask/selected-network-controller

  • FIXED: No longer sets the networkClientId for a newly permitted domain unless the useRequestQueuePreference flag is true

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

@jiexi jiexi marked this pull request as ready for review June 5, 2024 00:58
@jiexi jiexi requested a review from a team as a code owner June 5, 2024 00:58
Gudahtt
Gudahtt previously approved these changes Jun 5, 2024
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM

danjm pushed a commit to MetaMask/metamask-extension that referenced this pull request Jun 5, 2024
…25046)

Hotfix for v11.6.6 that patches @metamask/selected-network-controller
12.0.1 to include a check for if the useRequestQueue flag is set before
setting the networkClientId for a domain on permission controller state
change additions

* Core PR: MetaMask/core#4368
* Core patch branch:
https://github.com/MetaMask/core/tree/jl/patch-selected-network-controller-12.0.1-permission-state-change-guard

---------

Co-authored-by: Mark Stacey <[email protected]>
@adonesky1 adonesky1 merged commit ab6bf37 into main Jun 5, 2024
113 checks passed
@adonesky1 adonesky1 deleted the jl/fix-selected-network-controller-permission-state-change-handler branch June 5, 2024 15:36
jiexi added a commit that referenced this pull request Jun 10, 2024
…uePreference guard (#4388)

## Explanation

Our previous SelectedNetworkController fix that [added a guard to
permission state changes](#4368)
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: MetaMask/metamask-extension#25097

## 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**: `setNetworkClientId()` will now result in a noop if
`useRequestQueuePreference` is false


## 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 <[email protected]>
Sign up for free to 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.

3 participants