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

chore: Remove toggle to turn on/off Per Dapp Selected Network Feature #29301

Merged
merged 9 commits into from
Jan 9, 2025

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Dec 17, 2024

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.

Screenshot 2024-11-19 at 10 58 04 AM

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 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 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

Before

After

Pre-merge author checklist

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.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@adonesky1 adonesky1 changed the title Remove Queuing/Per Dapp Selected Network Experimental Toggle chore: Remove toggle to turn on/off Per Dapp Selected Network Feature Dec 17, 2024
@adonesky1 adonesky1 marked this pull request as ready for review December 17, 2024 22:04
@adonesky1 adonesky1 requested a review from a team as a code owner December 17, 2024 22:04
@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@adonesky1 adonesky1 force-pushed the ad/remove-queueing-toggle branch from ad54e1a to 52b5b17 Compare December 19, 2024 16:02
@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@adonesky1 adonesky1 force-pushed the ad/remove-queueing-toggle branch from 52b5b17 to 241d30b Compare December 19, 2024 21:57
@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@metamaskbot
Copy link
Collaborator

❌ API Spec Test Failed. View the report here.

@adonesky1 adonesky1 force-pushed the ad/remove-queueing-toggle branch 2 times, most recently from 1ce5b14 to 257815d Compare January 7, 2025 21:23
@adonesky1 adonesky1 force-pushed the ad/remove-queueing-toggle branch from 257815d to 813fe63 Compare January 7, 2025 21:46
@metamaskbot
Copy link
Collaborator

Builds ready [ac9677b]
Page Load Metrics (1600 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1442179616008742
domContentLoaded1407176615718440
load1443178616008742
domInteractive21188433818
backgroundConnect115826157
firstReactRender1699503216
getState45611157
initialActions00000
loadScripts1033132311767636
setupStore675162211
uiStartup163724521940248119

@adonesky1 adonesky1 enabled auto-merge January 8, 2025 00:09
'PreferencesController:stateChange',
listener,
);
listener({ useRequestQueue: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

can this hook just be a noop now instead? or do we have to immediately call the listener for some other reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here: c6dc756

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like there's a ts and js version of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gooood catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed here: c6dc756

@metamaskbot
Copy link
Collaborator

Builds ready [c6dc756]
Page Load Metrics (1636 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39918481524375180
domContentLoaded1492180616139545
load15101846163610149
domInteractive228036157
backgroundConnect65522136
firstReactRender1685332412
getState54612136
initialActions01000
loadScripts1101138212119143
setupStore65312136
uiStartup16912089186811455

@adonesky1 adonesky1 added this pull request to the merge queue Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@adonesky1 adonesky1 added this pull request to the merge queue Jan 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025
@adonesky1 adonesky1 added this pull request to the merge queue Jan 9, 2025
Merged via the queue into main with commit 4cbce35 Jan 9, 2025
77 checks passed
@adonesky1 adonesky1 deleted the ad/remove-queueing-toggle branch January 9, 2025 16:53
@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2025
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Jan 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-wallet-api-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants