V12.0.5 cherrypick 2dc3768 fix network form update configuration in place state corruption#26457
Merged
danjm merged 6 commits intoVersion-v12.0.5from Aug 16, 2024
Conversation
…Form update in-place rather than remove and add for rpcUrl changes (#26453) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> Currently it is not possible to change the rpcUrl for an existing network configuration without first removing it then re-adding it. This poses an issue on extension when the rpcUrl for the currently selected network is changed via the `NetworkForm`. The NetworkForm first removes the existing networkClient which causes the SelectedNetworkController to replace any references to the networkClientId being removed with the selectedNetworkClientId, but the problem is that the selectedNetworkClientId is no longer valid at that point and leaves the SelectedNetworkController in a corrupted state. Really what we want in this case is to allow the network configuration to be updated in place by id, but only if the rpcUrl isn't changing to one that already exists on a different network configuration. This PR achieves that by getting rid of the `removeNetworkConfiguration()` call triggered by `NetworkForm` and patching the `NetworkController` with changes from MetaMask/core#4614 which allow network configurations to have their rpcUrl updated in place. It also keeps the existing patch that removed a `lookupNetwork()` call in `initializeProvider()`. [](https://codespaces.new/MetaMask/metamask-extension/pull/26453?quickstart=1) Related: #26309 1. Open extension in expanded view 2. Visit the Settings -> Networks 3. Add a network manually and add an rpc provider for Arbitrum One (for your convenience, [chainlist.org](https://chainlist.org/chain/42161)) 4. In the extension service worker console note the state via `chrome.storage.local.get(console.log)` for NetworkController.networkConfigurations 5. Visit the Network Form for that Network 6. Change the rpcUrl to a different valid one for Arbitrum 7. In the extension service worker console check state via `chrome.storage.local.get(console.log)`, the id for the network configuration that was just edited should not have changed in NetworkController.networkConfigurations 8. Visit a dapp, switch the chain to Arbitrum ``` await window.ethereum.request({ "method": "eth_requestAccounts", }); await window.ethereum.request({ "method": "wallet_switchEthereumChain", "params": [ { "chainId": "0xa4b1" } ] }); ``` 7. In the extension service worker console check state via `chrome.storage.local.get(console.log)`, the dapp should have an entry for the network client we added above SelectedNetworkController.domains 8. Again, Change the rpcUrl to a different valid one for Arbitrum 9. In the extension service worker console check state via `chrome.storage.local.get(console.log)`, the dapp should still be pointed at the same network client id in SelectedNetworkController.domains which should still exist in NetworkController.networkConfigurations <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> <!-- [screenshots/recordings] --> <!-- [screenshots/recordings] --> - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] 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/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] 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.
…3768-fix-network-form-update-configuration-in-place-state-corruption
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. |
Member
Author
|
patch branch for network-controller 18.1.2 https://github.com/MetaMask/core/compare/jl/network-controller%4018.1.2-patch?expand=1 |
Contributor
|
@metamaskbot update-policies |
Collaborator
|
Policy update failed. You can review the logs or retry the policy update here |
Contributor
|
@metamaskbot update-policies |
Collaborator
|
Policy update failed. You can review the logs or retry the policy update here |
Contributor
|
@metamaskbot update-policies |
DDDDDanica
approved these changes
Aug 16, 2024
Collaborator
|
Policy update failed. You can review the logs or retry the policy update here |
Contributor
|
@metamaskbot update-policies |
Collaborator
|
Policy update failed. You can review the logs or retry the policy update here |
Collaborator
Builds ready [9b3fa55]
Page Load Metrics (51 ± 5 ms)
|
Collaborator
Builds ready [9b3fa55]
Page Load Metrics (51 ± 5 ms)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherrypick #26453
No major merge conflicts except for having to recreate the @metamask/network-controller patch against 19.0.0 to 18.1.2
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist