Skip to content

V12.0.5 cherrypick 2dc3768 fix network form update configuration in place state corruption#26457

Merged
danjm merged 6 commits intoVersion-v12.0.5from
jl/Version-v12.0.5-cherrypick-2dc3768-fix-network-form-update-configuration-in-place-state-corruption
Aug 16, 2024
Merged

V12.0.5 cherrypick 2dc3768 fix network form update configuration in place state corruption#26457
danjm merged 6 commits intoVersion-v12.0.5from
jl/Version-v12.0.5-cherrypick-2dc3768-fix-network-form-update-configuration-in-place-state-corruption

Conversation

@jiexi
Copy link
Copy Markdown
Member

@jiexi jiexi commented Aug 16, 2024

Cherrypick #26453

No major merge conflicts except for having to recreate the @metamask/network-controller patch against 19.0.0 to 18.1.2

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

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.

jiexi added 2 commits August 15, 2024 17:58
…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()`.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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
@jiexi jiexi changed the title Jl/version v12.0.5 cherrypick 2dc3768 fix network form update configuration in place state corruption V12.0.5 cherrypick 2dc3768 fix network form update configuration in place state corruption Aug 16, 2024
@github-actions
Copy link
Copy Markdown
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.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Aug 16, 2024
@jiexi
Copy link
Copy Markdown
Member Author

jiexi commented Aug 16, 2024

patch branch for network-controller 18.1.2

https://github.com/MetaMask/core/compare/jl/network-controller%4018.1.2-patch?expand=1

@DDDDDanica
Copy link
Copy Markdown
Contributor

@metamaskbot update-policies

@metamaskbot
Copy link
Copy Markdown
Collaborator

Policy update failed. You can review the logs or retry the policy update here

@DDDDDanica
Copy link
Copy Markdown
Contributor

@metamaskbot update-policies

@metamaskbot
Copy link
Copy Markdown
Collaborator

Policy update failed. You can review the logs or retry the policy update here

@danjm
Copy link
Copy Markdown
Contributor

danjm commented Aug 16, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Copy Markdown
Collaborator

Policy update failed. You can review the logs or retry the policy update here

@danjm
Copy link
Copy Markdown
Contributor

danjm commented Aug 16, 2024

@metamaskbot update-policies

@metamaskbot
Copy link
Copy Markdown
Collaborator

Policy update failed. You can review the logs or retry the policy update here

@danjm danjm marked this pull request as ready for review August 16, 2024 02:15
@danjm danjm requested a review from a team as a code owner August 16, 2024 02:15
@danjm danjm merged commit b066542 into Version-v12.0.5 Aug 16, 2024
@danjm danjm deleted the jl/Version-v12.0.5-cherrypick-2dc3768-fix-network-form-update-configuration-in-place-state-corruption branch August 16, 2024 02:20
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [9b3fa55]
Page Load Metrics (51 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6310881115
domContentLoaded9231131
load417851105
domInteractive9231131

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [9b3fa55]
Page Load Metrics (51 ± 5 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6310881115
domContentLoaded9231131
load417851105
domInteractive9231131

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

INVALID-PR-TEMPLATE PR's body doesn't match template

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants