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

Update metamask/selected-network-controller to version 20.0.0 #12434

Open
metamaskbot opened this issue Nov 26, 2024 · 1 comment
Open

Update metamask/selected-network-controller to version 20.0.0 #12434

metamaskbot opened this issue Nov 26, 2024 · 1 comment

Comments

@metamaskbot
Copy link
Collaborator

Please update metamask/selected-network-controller to version 20.0.0

@adonesky1
Copy link
Contributor

We cannot/should not do this until we are fully ready to release the Per Dapp Selected Network Functionality in Mobile (discussed in the Multichain EVM UX Weekly meeting notes document), because doing so will start populating domains state for each connected dapp even though we're not ready to use that state yet.

adonesky1 added a commit to MetaMask/core that referenced this issue Dec 12, 2024
…p Selected Network Feature (#4941)" (#5065)

This reverts commit 4814cf1.

## Explanation

We are not yet ready to release per-dapp selected network functionality
on the mobile client and with this change there is no clean way to
[update the version of
SelectedNetworkController](MetaMask/metamask-mobile#12434 (comment))
in the mobile client without having the Domains state starting to
populate and possibly become corrupt since its not being consumed
by/updated by the frontend in the expected way and may need to be
migrated away when its time to actually start using the controller.

Without this revert the @MetaMask/wallet-framework team is blocked from
completing their goal to get both clients up to the latest versions of
all controllers.

## 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/queued-request-controller`

- **ADDED**: **BREAKING:** `createQueuedRequestMiddleware` now expects a
`useRequestQueue` property in its param options


### `@metamask/selected-network-controller`

- **ADDED**: **BREAKING:** `SelectedNetworkController` constructor now
expects
both a `useRequestQueuePreference` and a `onPreferencesStateChange`
param

## 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
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this issue Jan 9, 2025
…#29301)

## **Description**

The "Select networks for each site" preference toggle on the
experimental settings page has been live for many releases now since the
toggle has been turned on by default. We meant to remove it a while ago.

<img width="770" alt="Screenshot 2024-11-19 at 10 58 04 AM"
src="https://github.com/user-attachments/assets/aeca2483-c019-4f9f-b44e-520d10db9eee">

This PR removes this toggle ~and integrates new versions of the
QueuedRequestController and SelectedNetworkController which remove the
backend logic it operated.~
- We have delayed the updates to the controllers side because of some
mobile side requirements. See [this
PR](MetaMask/core#5065 (comment)) for
more context:

> We are not yet ready to release per-dapp selected network
functionality on the mobile client and with this change there is no
clean way to
MetaMask/metamask-mobile#12434 (comment)
in the mobile client without having the Domains state starting to
populate and possibly become corrupt since its not being consumed
by/updated by the frontend in the expected way and may need to be
migrated away when its time to actually start using the controller.

> Without this revert the @MetaMask/wallet-framework team is blocked
from completing their goal to get both clients up to the latest versions
of all controllers.

Beyond the fact that this removal is overdue, another reason we should
remove this now is that having this setting when turned off is [causing
a bug](#28441) with
`wallet_switchEthereumChain` and the interaction with the new chain
permissions feature.

## **Related issues**

Fixes: #2844

## **Manual testing steps**

1. Go to experimental tab of settings
2. See that there is no longer a toggleable preference called "Selected
Networks for each site"
3. See that Per Dapp Selected Network Functionality is still on by
default

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.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.
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this issue Jan 9, 2025
…#29301)

## **Description**

The "Select networks for each site" preference toggle on the
experimental settings page has been live for many releases now since the
toggle has been turned on by default. We meant to remove it a while ago.

<img width="770" alt="Screenshot 2024-11-19 at 10 58 04 AM"
src="https://github.com/user-attachments/assets/aeca2483-c019-4f9f-b44e-520d10db9eee">

This PR removes this toggle ~and integrates new versions of the
QueuedRequestController and SelectedNetworkController which remove the
backend logic it operated.~
- We have delayed the updates to the controllers side because of some
mobile side requirements. See [this
PR](MetaMask/core#5065 (comment)) for
more context:

> We are not yet ready to release per-dapp selected network
functionality on the mobile client and with this change there is no
clean way to
MetaMask/metamask-mobile#12434 (comment)
in the mobile client without having the Domains state starting to
populate and possibly become corrupt since its not being consumed
by/updated by the frontend in the expected way and may need to be
migrated away when its time to actually start using the controller.

> Without this revert the @MetaMask/wallet-framework team is blocked
from completing their goal to get both clients up to the latest versions
of all controllers.

Beyond the fact that this removal is overdue, another reason we should
remove this now is that having this setting when turned off is [causing
a bug](#28441) with
`wallet_switchEthereumChain` and the interaction with the new chain
permissions feature.

## **Related issues**

Fixes: #2844

## **Manual testing steps**

1. Go to experimental tab of settings
2. See that there is no longer a toggleable preference called "Selected
Networks for each site"
3. See that Per Dapp Selected Network Functionality is still on by
default

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.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.
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this issue Jan 9, 2025
…#29301)

## **Description**

The "Select networks for each site" preference toggle on the
experimental settings page has been live for many releases now since the
toggle has been turned on by default. We meant to remove it a while ago.

<img width="770" alt="Screenshot 2024-11-19 at 10 58 04 AM"
src="https://github.com/user-attachments/assets/aeca2483-c019-4f9f-b44e-520d10db9eee">

This PR removes this toggle ~and integrates new versions of the
QueuedRequestController and SelectedNetworkController which remove the
backend logic it operated.~
- We have delayed the updates to the controllers side because of some
mobile side requirements. See [this
PR](MetaMask/core#5065 (comment)) for
more context:

> We are not yet ready to release per-dapp selected network
functionality on the mobile client and with this change there is no
clean way to
MetaMask/metamask-mobile#12434 (comment)
in the mobile client without having the Domains state starting to
populate and possibly become corrupt since its not being consumed
by/updated by the frontend in the expected way and may need to be
migrated away when its time to actually start using the controller.

> Without this revert the @MetaMask/wallet-framework team is blocked
from completing their goal to get both clients up to the latest versions
of all controllers.

Beyond the fact that this removal is overdue, another reason we should
remove this now is that having this setting when turned off is [causing
a bug](#28441) with
`wallet_switchEthereumChain` and the interaction with the new chain
permissions feature.

## **Related issues**

Fixes: #2844

## **Manual testing steps**

1. Go to experimental tab of settings
2. See that there is no longer a toggleable preference called "Selected
Networks for each site"
3. See that Per Dapp Selected Network Functionality is still on by
default

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants