Force-toggle testnets visibility in network switcher #3793
Conversation
Moves test network visibility user setting to preference service and sets force toggles it to display on all new and existing installs
| @@ -186,8 +186,8 @@ export default function Settings(): ReactElement { | |||
| ) | |||
| } | |||
|
|
|||
| const toggleShowTestNetworks = (defaultWalletValue: boolean) => { | |||
| dispatch(toggleTestNetworks(defaultWalletValue)) | |||
There was a problem hiding this comment.
We should probably kill this old action in the UI slice yeah?
There was a problem hiding this comment.
Wait this old action is still used to update the "cached" store value 👀 (in the redux service)
There was a problem hiding this comment.
Ahhh I see. Ok, so we have moved from, “if you want to toggle test networks you update directly in redux” to “if you want to toggle test networks you thunk in redux, which updates the preference service, which then updates in redux”, and toggleShowTestNetworks is the thunk and toggleTestNetworks is the direct state update.
Ok well that's clear as mud as in other cases that we've done this heh, and we have the same problem in the whole slice. We should figure out a clearer way to name these IMO—something like setShow... and updateShow... or something—and we should clearly document at the top of the slice.
This isn't a blocker though, just going to continue to be a pain point. The other option is abstracting preference settings a little more, but I remember I did a deep pass at that a couple of years ago and the typing is (or was) a near impossibility.
| @@ -392,6 +394,22 @@ export const setSelectedNetwork = createBackgroundAsyncThunk( | |||
| }, | |||
| ) | |||
|
|
|||
| export const toggleShowTestNetworks = createBackgroundAsyncThunk( | |||
| "ui/toggleShowTestNetworks", | |||
| async (value: boolean, { dispatch, getState }) => { | |||
There was a problem hiding this comment.
| async (value: boolean, { dispatch, getState }) => { | |
| async (updatedValue: boolean, { dispatch, getState }) => { |
Perhaps for clarity? Or even updatedShowTestNetworksValue or similar?
| const currentNetwork = state.ui.selectedAccount.network | ||
|
|
||
| // If user is on one of the built-in test networks, don't leave them stranded | ||
| if (TEST_NETWORK_BY_CHAIN_ID.has(currentNetwork.chainID)) { |
There was a problem hiding this comment.
We should also guard on !value right? Like, if the user is toggling from off to on, we shouldn't push them into Ethereum—in theory they wouldn't be there anyway because they wouldn't be on a test network, but in fact they might be if they've added the testnet as a custom chain, for example.
There was a problem hiding this comment.
Good catch, I think this should also happen if the production build has the testnet as a built in network that's still under a feature flag.
This PR moves the testnet visibility setting to the preference service, and sets a migration to enable it on all existing and new installs.
Testing
Latest build: extension-builds-3793 (as of Tue, 04 Mar 2025 05:05:23 GMT).