fix: Fix SelectedNetworkController state corruption by making NetworkForm update in-place rather than remove and add for rpcUrl changes #26453

jiexi · 2024-08-15T20:22:33Z

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().

Related issues

Related: #26309

Manual testing steps

Open extension in expanded view
Visit the Settings -> Networks
Add a network manually and add an rpc provider for Arbitrum One (for your convenience, chainlist.org)
In the extension service worker console note the state via chrome.storage.local.get(console.log) for NetworkController.networkConfigurations
Visit the Network Form for that Network
Change the rpcUrl to a different valid one for Arbitrum
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
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"
    }
  ]
});

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
Again, Change the rpcUrl to a different valid one for Arbitrum
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

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

github-actions · 2024-08-15T20:22:43Z

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 · 2024-08-15T20:54:44Z

Builds ready [fffe69d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (258 ± 245 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	71	473	132	84	40
		domContentLoaded	9	101	37	24	11
		load	39	1951	258	510	245
		domInteractive	9	101	37	24	11

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: -22 Bytes (-0.00%)
common: -647 Bytes (-0.01%)

metamaskbot · 2024-08-15T22:30:17Z

Builds ready [7911aef]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (540 ± 458 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	80	164	118	26	12
		domContentLoaded	10	66	34	16	8
		load	46	3175	540	953	458
		domInteractive	10	66	34	16	8

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: -22 Bytes (-0.00%)
common: -360 Bytes (-0.00%)

codecov · 2024-08-15T22:30:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.12%. Comparing base (54b3237) to head (7911aef).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #26453      +/-   ##
===========================================
- Coverage    70.12%   70.12%   -0.01%     
===========================================
  Files         1428     1428              
  Lines        50103    50089      -14     
  Branches     13896    13894       -2     
===========================================
- Hits         35133    35120      -13     
+ Misses       14970    14969       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

davidmurdoch · 2024-08-15T23:03:19Z

.yarn/patches/@metamask-network-controller-npm-19.0.0-a5e0d1fe14.patch

@@ -1,24 +1,4263 @@
+diff --git a/dist/NetworkController.js b/dist/NetworkController.js


Is it possible to just release the fixed network-controller package really quickly and then update to it?

i don't think that'd be realistically possible given the breaking change in 20.0.0 removes providerConfig from state. This is probably why we are still on 19.0.0

https://github.com/MetaMask/core/blob/main/packages/network-controller/CHANGELOG.md#removed

I'm going to guess that you're suggesting this since the patch is not human readable

Is it possible to just release the fixed network-controller package really quickly and then update to it?

It is not possible to do this in a timely manner, but we need to get this fix into prod asap

jiexi · 2024-08-15T23:55:47Z

Do we need to include changes to rerun 120.5 in this PR too?

danjm

LGTM

danjm · 2024-08-15T23:59:56Z

Do we need to include changes to rerun 120.5 in this PR too?

yes, that makes sense

danjm

changing my approval as there will be adding more migration code added here

DDDDDanica · 2024-08-16T00:02:53Z

LGTM ! Tested locally and worked as described.

One thing as Nit is we can add an e2e test for editing rpcURL for selected network in the future ticket

sonarqubecloud · 2024-08-16T00:18:57Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-08-16T00:47:44Z

Builds ready [957f59a]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (140 ± 151 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	222	101	40	19
		domContentLoaded	9	134	27	26	12
		load	40	1505	140	315	151
		domInteractive	9	134	27	26	12

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.44 KiB (0.06%)
ui: -22 Bytes (-0.00%)
common: -360 Bytes (-0.00%)

jiexi · 2024-08-16T01:01:58Z

patch branch for network-controller 19.0.0

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

jiexi added 3 commits August 15, 2024 09:25

replace editAndSetNetworkConfiguration with upsertNetworkConfiguration

a2be061

change upsertNetworkConfiguration networkConfigurationId to id

58db82c

Merge branch 'develop' into jl/mme-26309/fix-network-form-upsert

fffe69d

jiexi mentioned this pull request Aug 15, 2024

feat: NetworkController.upsertNetworkConfiguration allow upsert by networkConfigurationId MetaMask/core#4614

Merged

3 tasks

metamaskbot added the team-wallet-api-platform label Aug 15, 2024

update @metamask/network-controller@19.0.0 patch

56aaf53

jiexi changed the title ~~Jl/mme 26309/fix network form upsert~~ fix: Fix SelectedNetworkController state corruption by making NetworkForm update in-place rather than remove and add for rpcUrl changes Aug 15, 2024

fix actions.test.js

7911aef

jiexi marked this pull request as ready for review August 15, 2024 22:23

jiexi requested a review from a team as a code owner August 15, 2024 22:23

davidmurdoch reviewed Aug 15, 2024

View reviewed changes

danjm approved these changes Aug 15, 2024

View reviewed changes

danjm requested changes Aug 16, 2024

View reviewed changes

actually run 120.5

957f59a

DDDDDanica approved these changes Aug 16, 2024

View reviewed changes

danjm approved these changes Aug 16, 2024

View reviewed changes

jiexi merged commit 2dc3768 into develop Aug 16, 2024
77 of 78 checks passed

jiexi deleted the jl/mme-26309/fix-network-form-upsert branch August 16, 2024 00:39

github-actions bot locked and limited conversation to collaborators Aug 16, 2024

metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 16, 2024

gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix SelectedNetworkController state corruption by making NetworkForm update in-place rather than remove and add for rpcUrl changes #26453

fix: Fix SelectedNetworkController state corruption by making NetworkForm update in-place rather than remove and add for rpcUrl changes #26453

jiexi commented Aug 15, 2024 •

edited

Loading

github-actions bot commented Aug 15, 2024

metamaskbot commented Aug 15, 2024

metamaskbot commented Aug 15, 2024

codecov bot commented Aug 15, 2024

davidmurdoch Aug 15, 2024

jiexi Aug 15, 2024

danjm Aug 15, 2024

jiexi commented Aug 15, 2024

danjm left a comment

danjm commented Aug 15, 2024 •

edited

Loading

danjm left a comment

DDDDDanica commented Aug 16, 2024 •

edited

Loading

sonarqubecloud bot commented Aug 16, 2024

metamaskbot commented Aug 16, 2024

jiexi commented Aug 16, 2024

		@@ -1,24 +1,4263 @@
		diff --git a/dist/NetworkController.js b/dist/NetworkController.js

fix: Fix SelectedNetworkController state corruption by making NetworkForm update in-place rather than remove and add for rpcUrl changes #26453

fix: Fix SelectedNetworkController state corruption by making NetworkForm update in-place rather than remove and add for rpcUrl changes #26453

Conversation

jiexi commented Aug 15, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Aug 15, 2024

metamaskbot commented Aug 15, 2024

metamaskbot commented Aug 15, 2024

codecov bot commented Aug 15, 2024

Codecov Report

davidmurdoch Aug 15, 2024

Choose a reason for hiding this comment

jiexi Aug 15, 2024

Choose a reason for hiding this comment

danjm Aug 15, 2024

Choose a reason for hiding this comment

jiexi commented Aug 15, 2024

danjm left a comment

Choose a reason for hiding this comment

danjm commented Aug 15, 2024 • edited Loading

danjm left a comment

Choose a reason for hiding this comment

DDDDDanica commented Aug 16, 2024 • edited Loading

sonarqubecloud bot commented Aug 16, 2024

Quality Gate passed

metamaskbot commented Aug 16, 2024

jiexi commented Aug 16, 2024

jiexi commented Aug 15, 2024 •

edited

Loading

danjm commented Aug 15, 2024 •

edited

Loading

DDDDDanica commented Aug 16, 2024 •

edited

Loading