Conversation
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
jiexi
commented
Aug 15, 2024
Gudahtt
approved these changes
Aug 15, 2024
Member
Author
|
Tested by manually copying NetworkController changes into this Extension branch that gets rid of the network removal when editing an existing network |
jiexi
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 16, 2024
…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. --> ## **Description** 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 issues** Related: #26309 ## **Manual testing steps** 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 ## **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** - [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. ## **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.
jiexi
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 16, 2024
…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.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Explanation
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. 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 changing
NetworkController.upsertNetworkConfiguration()to accept an optionalidin theNetworkConfigurationparam. When provided, it MUST match to an existing network configuration. Additionally, it must match the id for any network configuration that may match the target rpcUrlReferences
Related: MetaMask/metamask-extension#26309
Changelog
@metamask/network-controllerNetworkController.upsertNetworkConfiguration()accepts an optionalidproperty on theNetworkConfigurationparam. It now allows a network configuration to have itsrpcUrlupdated in place when anidis specified, but only if that newrpcUrldoes not already exist on a different network configuration.Checklist