Ensure that all networkConfiguration object in networkController state have an id #18513

danjm · 2023-04-08T09:48:34Z

Explanation

We partially fixed the symptom of 18509 and 18453 with #18483, but that fix was incomplete.

This bug will be experienced by anyone who added a network while on v10.27.0 or earlier, and are now on v10.28.1 or v10.28.2 and trying to confirm a dapp prompted switch to that network. This bug will not affect users who first added the network, via a dapp wallet_addEthereumChain request, while on v10.28.1 . This bug also won't affect users who added the network via the MetaMask UI in v10.28.1

The problem is that migration 82 did not add an id property to each network configuration object, but we assume that id property exists in the switch-ethereum-chain.js handler. If a user adds a network on version v10.27.0 or earlier, and then updates to v10.28.x, migration 82 will create an object within NetworkController.networkConfigurations keyed by a random id, but that id will not be added to that object itself. Later, when we attempt to switch to the network using that objects id property, it fails. It does not fail if the network is added while on v10.28.x, because the upsertNetworkConfiguration method, which is called when adding a network on v10.28.x, adds an id property to each network.

This PR corrects the problem with a migration that ensures that the each key within networkConfigurations is used to set the id property of the respective object.

Before

This video shows what would happen when upgrading from v10.27.0 -> v10.28.2 and then trying to switch to a previously added network:

switchnetworkfail-v10282.mp4

After

This shows switch network working after an upgrade from v10.27.0 to the branch of this PR:

switchnetwork-fix.mp4

Manual Testing Steps

Do as is done in the above "After" video:

Build v10.27.0, from d8591d4, install and onboard
Go to https://syncswap.xyz/, connect and add the suggested network
In MetaMask, select Ethereum Mainnet, so that the zkSync network is not selected
git checkout this branch, build and then reload the metamask installation that was previously built from v10.27.0
login to MetaMask and then go to https://syncswap.xyz/
click "Connect Wallet" and confirm the switch network popup
open MetaMask, you should now have the zksync network selected

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

…e have an id

github-actions · 2023-04-08T09:48:47Z

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.

danjm · 2023-04-08T10:04:09Z

This will require #18512 (review) to get the build passing.

metamaskbot · 2023-04-08T10:28:40Z

Builds ready [fb3f650]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1591 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	94	135	107	10	5
		domContentLoaded	1409	1761	1556	91	44
		load	1409	1812	1591	108	52
		domInteractive	1408	1761	1556	91	44

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

background: 948 bytes
ui: 0 bytes
common: 0 bytes

metamaskbot · 2023-04-09T02:07:31Z

Builds ready [5ec4489]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1639 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	94	180	111	21	10
		domContentLoaded	1486	1736	1593	62	30
		load	1518	1841	1639	81	39
		domInteractive	1486	1736	1593	62	30

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

background: 948 bytes
ui: 0 bytes
common: 0 bytes

metamaskbot · 2023-04-10T11:01:02Z

Builds ready [7af5ddc]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1603 ± 75 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	84	343	117	54	26
		domContentLoaded	1413	1955	1583	158	76
		load	1414	1955	1603	157	75
		domInteractive	1413	1955	1583	158	76

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

background: 948 bytes
ui: 0 bytes
common: 0 bytes

metamaskbot · 2023-04-10T22:46:07Z

Builds ready [43d690a]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1953 ± 120 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	99	381	158	69	33
		domContentLoaded	1605	2479	1932	258	124
		load	1607	2479	1953	251	120
		domInteractive	1605	2479	1932	258	124

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

background: 948 bytes
ui: 0 bytes
common: 0 bytes

app/scripts/migrations/084.ts

adonesky1 · 2023-04-10T23:27:14Z

It probably does make sense to fix the state here but - just a thought - another lighter weight way to fix this (I think) would be to modify the findNetworkConfigurationBy method on MetamaskController like this:

   findNetworkConfigurationBy(rpcInfo) {
     const { networkConfigurations } = this.networkController.store.getState();
-    const networkConfiguration = Object.values(networkConfigurations).find(
-      (configuration) => {
-        return Object.keys(rpcInfo).some((key) => {
-          return configuration[key] === rpcInfo[key];
-        });
-      },
-    );
+    const [id, networkConfiguration] = Object.entries(
+      networkConfigurations,
+    ).find((configuration) => {
+      return Object.keys(rpcInfo).some((key) => {
+        return configuration[key] === rpcInfo[key];
+      });
+    });
 
-    return networkConfiguration || null;
+    return { ...networkConfiguration, id } || null;
   }

legobeat

LGTM

app/scripts/migrations/084.ts

app/scripts/migrations/084.test.js

metamaskbot · 2023-04-11T12:37:42Z

Builds ready [e751c04]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1640 ± 107 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	91	175	115	27	13
		domContentLoaded	1443	2224	1620	222	107
		load	1443	2224	1640	224	107
		domInteractive	1443	2224	1620	222	107

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

background: 948 bytes
ui: 0 bytes
common: 0 bytes

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

adonesky1

LGTM!

metamaskbot · 2023-04-11T15:27:26Z

Builds ready [83545e3]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1713 ± 77 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	88	179	122	26	13
		domContentLoaded	1449	2016	1700	162	78
		load	1449	2016	1713	161	77
		domInteractive	1449	2016	1700	162	78

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

background: 974 bytes
ui: 0 bytes
common: 0 bytes

metamaskbot · 2023-04-11T18:19:19Z

Builds ready [affa960]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1610 ± 64 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	102	164	119	16	8
		domContentLoaded	1371	1856	1601	126	61
		load	1371	1878	1610	133	64
		domInteractive	1371	1856	1601	126	61

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

background: 1016 bytes
ui: 0 bytes
common: 0 bytes

Gudahtt

LGTM!

Ensure that all networkConfiguration object in networkController stat…

aca0d13

…e have an id

danjm requested a review from a team as a code owner April 8, 2023 09:48

danjm requested a review from NiranjanaBinoy April 8, 2023 09:48

Lint fix

fb3f650

Merge branch 'develop' into fix-wallet_switchEthereumNetwork-2

5ec4489

kraouf3 referenced this pull request Apr 9, 2023

update contributing.md

29b3da6

Merge branch 'develop' into fix-wallet_switchEthereumNetwork-2

7af5ddc

Merge branch 'develop' into fix-wallet_switchEthereumNetwork-2

43d690a

adonesky1 reviewed Apr 10, 2023

View reviewed changes

app/scripts/migrations/084.ts Outdated Show resolved Hide resolved

Merge branch 'develop' into fix-wallet_switchEthereumNetwork-2

9829ed1

legobeat previously approved these changes Apr 11, 2023

View reviewed changes

DDDDDanica previously approved these changes Apr 11, 2023

View reviewed changes

Gudahtt reviewed Apr 11, 2023

View reviewed changes

app/scripts/migrations/084.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Apr 11, 2023

View reviewed changes

app/scripts/migrations/084.ts Outdated Show resolved Hide resolved

Merge branch 'develop' into fix-wallet_switchEthereumNetwork-2

e751c04

Gudahtt reviewed Apr 11, 2023

View reviewed changes

app/scripts/migrations/084.test.js Show resolved Hide resolved

Update app/scripts/migrations/084.ts

2a8c2e7

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

danjm dismissed stale reviews from legobeat and DDDDDanica via 2a8c2e7 April 11, 2023 13:23

danjm added 3 commits April 11, 2023 11:05

Add unit tests for error cases

65a8b8f

Simplify code

bb008c4

Remove unnecessary any typing

8116d9f

adonesky1 previously approved these changes Apr 11, 2023

View reviewed changes

Fix network controller type checking

a601b34

danjm dismissed adonesky1’s stale review via a601b34 April 11, 2023 14:51

Lint fix

83545e3

adonesky1 previously approved these changes Apr 11, 2023

View reviewed changes

Improve typing

affa960

danjm dismissed adonesky1’s stale review via affa960 April 11, 2023 17:25

Gudahtt approved these changes Apr 11, 2023

View reviewed changes

adonesky1 approved these changes Apr 11, 2023

View reviewed changes

danjm merged commit b3c4790 into develop Apr 11, 2023

danjm deleted the fix-wallet_switchEthereumNetwork-2 branch April 11, 2023 19:16

github-actions bot locked and limited conversation to collaborators Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that all networkConfiguration object in networkController state have an id #18513

Ensure that all networkConfiguration object in networkController state have an id #18513

danjm commented Apr 8, 2023

github-actions bot commented Apr 8, 2023

danjm commented Apr 8, 2023

metamaskbot commented Apr 8, 2023

metamaskbot commented Apr 9, 2023

metamaskbot commented Apr 10, 2023

metamaskbot commented Apr 10, 2023

adonesky1 commented Apr 10, 2023

legobeat left a comment

metamaskbot commented Apr 11, 2023

adonesky1 left a comment

metamaskbot commented Apr 11, 2023

metamaskbot commented Apr 11, 2023

Gudahtt left a comment

Ensure that all networkConfiguration object in networkController state have an id #18513

Ensure that all networkConfiguration object in networkController state have an id #18513

Conversation

danjm commented Apr 8, 2023

Explanation

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Apr 8, 2023

danjm commented Apr 8, 2023

metamaskbot commented Apr 8, 2023

metamaskbot commented Apr 9, 2023

metamaskbot commented Apr 10, 2023

metamaskbot commented Apr 10, 2023

adonesky1 commented Apr 10, 2023

legobeat left a comment

Choose a reason for hiding this comment

metamaskbot commented Apr 11, 2023

adonesky1 left a comment

Choose a reason for hiding this comment

metamaskbot commented Apr 11, 2023

metamaskbot commented Apr 11, 2023

Gudahtt left a comment

Choose a reason for hiding this comment