Fix #19941 - Correctly show network name and selection when chainIds collide #19947

darkwing · 2023-07-10T20:43:47Z

Explanation

Fixes [Bug]: RPC requests are mixed while using different networks with same chainId #19941

This bug was occurring because we were selecting the first network with a given chain ID. That means if there's a custom network with a chain ID that matches a pre-installed network (i.e. Mainnet having 0x1 and a localhost 0x1 chain, like the issue cites), we show the wrong network selected in the Network Picker despite switching to the correct network. This is a labeling issue, not a functionality issue.

To fix this issue, I fork the logic for retrieving the current network by checking to see if it's a custom network (by checking for type=rpc and searching for the network by configuration ID. If a pre-installed network, we use the chainId.

Screenshots/Screencaps

NetworkFix.mov

Manual Testing Steps

Add a bunch of predefined networks like Avalanche, Polygon, etc.
Start ganache with ganache --chain.chainId 1
Add localhost with a chainID of 1
Switch between all of the networks you've added and see that networks switch properly, the network picker shows the correct label, and the correct item in the network list shows as 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.

github-actions · 2023-07-10T20:44:01Z

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.

Gudahtt · 2023-07-10T21:53:14Z

ui/selectors/selectors.js

+  const filter =
+    providerConfig.type === 'rpc'
+      ? (network) => network.id === providerConfig.id
+      : (network) => network.chainId === currentChainId;


Thoughts on using provider.type instead? chainId works, but it makes the result order-dependent. type is reliably unique for built-in networks, so that should make this a little more reliable even if the order changes later.

It looks like providerConfig.type is available on the network list entries as network.providerType

Suggested change

: (network) => network.chainId === currentChainId;

: (network) => network.providerType === providerConfig.type;

Alternatively, since you've set the id field to the type for all of the built-in networks, maybe this would work:

Suggested change

: (network) => network.chainId === currentChainId;

: (network) => network.id === providerConfig.type;

Awesome idea!

In my mind, the rpcUrl (if available) would be the most reliable. rpcUrl || chainId might do the trick?

This should also make custom RPCs more robust, I think (it looks like multiple custom RPCs with overlapping chainId can still collide if the url is not taken into account in either case).

(IMO more generally, it would make sense for multiple reasons that rpcUrl would become a mandatory field, which would mean simplified logic in places like this)

Using id might be more forwards-compatible if we start introducing more custom RPC fields alongside rpc URL, making it no longer unique. We did get a request to support additional headers, where the header might determine the chain.

Gudahtt · 2023-07-11T11:40:07Z

The code looks good, but I'm seeing consistent e2e test failures for the test "finds all recent RPCs in history"

metamaskbot · 2023-07-12T16:38:07Z

Builds ready [2225629]

builds: chrome, firefox
builds (beta): chrome
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, 5
- 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, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1608 ± 45 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	108	238	141	33	16
		domContentLoaded	1456	1810	1608	94	45
		load	1456	1810	1608	94	45
		domInteractive	1456	1810	1608	94	45

Bundle size diffs

background: 0 bytes
ui: 238 bytes
common: 0 bytes

codecov · 2023-07-12T16:38:19Z

Codecov Report

Merging #19947 (f1c94b4) into develop (2f03c0d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop   #19947   +/-   ##
========================================
  Coverage    69.47%   69.48%           
========================================
  Files          988      988           
  Lines        37297    37301    +4     
  Branches      9980     9982    +2     
========================================
+ Hits         25912    25916    +4     
  Misses       11385    11385

Impacted Files	Coverage Δ
.../multichain/network-list-menu/network-list-menu.js	`69.64% <100.00%> (+0.55%)`	⬆️
ui/selectors/selectors.js	`87.01% <100.00%> (+0.06%)`	⬆️

... and 2 files with indirect coverage changes

metamaskbot · 2023-07-12T19:38:29Z

Builds ready [2225629]

builds: chrome, firefox
builds (beta): chrome
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, 5
- 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, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1565 ± 33 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	110	158	125	13	6
		domContentLoaded	1436	1733	1564	69	33
		load	1436	1734	1565	69	33
		domInteractive	1436	1733	1564	69	33

Bundle size diffs

background: 0 bytes
ui: 238 bytes
common: 0 bytes

…collide

metamaskbot · 2023-07-13T14:27:16Z

Builds ready [f1c94b4]

builds: chrome, firefox
builds (beta): chrome
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, 5
- 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, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1655 ± 47 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	112	191	141	22	10
		domContentLoaded	1455	1825	1654	98	47
		load	1455	1825	1655	98	47
		domInteractive	1455	1825	1654	98	47

Bundle size diffs

background: 0 bytes
ui: 238 bytes
common: 0 bytes

danjm · 2023-07-13T18:25:39Z

The code looks good, but I'm seeing consistent e2e test failures for the test "finds all recent RPCs in history"

This was fixed with this change https://github.com/MetaMask/metamask-extension/pull/19947/files#diff-50c876d2ff4c618bf7018751413649df82021af47d93754663e8bf7522bea68fL199-L200

Previously, there were two networks in the fixtures with the same id, and one of those networks would result in a 404 if a request was sent to it. So metamask was attempting to connect to the wrong network.

darkwing requested a review from a team as a code owner July 10, 2023 20:43

darkwing added the needs-ux-ds-review label Jul 10, 2023

Gudahtt reviewed Jul 10, 2023

View reviewed changes

darkwing force-pushed the 19941-selected-network branch from a458f13 to 85608c0 Compare July 11, 2023 00:50

darkwing mentioned this pull request Jul 11, 2023

[Bug]: Custom Networks with Reserved ChainID Should Be Removable in Network Menu #19950

Closed

darkwing force-pushed the 19941-selected-network branch 2 times, most recently from 4451a03 to 9c6b9cc Compare July 11, 2023 03:39

darkwing force-pushed the 19941-selected-network branch 3 times, most recently from ce06ab6 to 2225629 Compare July 12, 2023 16:03

darkwing force-pushed the 19941-selected-network branch from 4a1b384 to c80a858 Compare July 12, 2023 19:16

darkwing force-pushed the 19941-selected-network branch 2 times, most recently from 6f07329 to 6c5156d Compare July 12, 2023 22:12

darkwing added 6 commits July 12, 2023 19:56

Fix #19941 - Correctly show network name and selection when chainIds …

6027edd

…collide

Use network id and type instead of chainId

428eb54

Update test data to be more representative

59c5e3a

Update network name

ab5e3ff

WIP

18f1a1e

Fix failing E2E

736065d

darkwing force-pushed the 19941-selected-network branch 2 times, most recently from 76c7559 to 736065d Compare July 13, 2023 13:52

Merge branch 'develop' into 19941-selected-network

f1c94b4

NidhiKJha approved these changes Jul 13, 2023

View reviewed changes

danjm approved these changes Jul 13, 2023

View reviewed changes

darkwing merged commit 719d8a4 into develop Jul 13, 2023

darkwing deleted the 19941-selected-network branch July 13, 2023 18:29

github-actions bot locked and limited conversation to collaborators Jul 13, 2023

metamaskbot added the release-10.35.0 Issue or pull request that will be included in release 10.35.0 label Jul 13, 2023

PeterYinusa added release-10.34.0 Issue or pull request that will be included in release 10.34.0 and removed release-10.35.0 Issue or pull request that will be included in release 10.35.0 labels Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #19941 - Correctly show network name and selection when chainIds collide #19947

Fix #19941 - Correctly show network name and selection when chainIds collide #19947

darkwing commented Jul 10, 2023

github-actions bot commented Jul 10, 2023

Gudahtt Jul 10, 2023

darkwing Jul 11, 2023

legobeat Jul 11, 2023 •

edited

Loading

Gudahtt Jul 11, 2023

Gudahtt commented Jul 11, 2023

metamaskbot commented Jul 12, 2023

codecov bot commented Jul 12, 2023 •

edited

Loading

metamaskbot commented Jul 12, 2023

metamaskbot commented Jul 13, 2023

danjm commented Jul 13, 2023

	: (network) => network.chainId === currentChainId;
	: (network) => network.providerType === providerConfig.type;

	: (network) => network.chainId === currentChainId;
	: (network) => network.id === providerConfig.type;

Fix #19941 - Correctly show network name and selection when chainIds collide #19947

Fix #19941 - Correctly show network name and selection when chainIds collide #19947

Conversation

darkwing commented Jul 10, 2023

Explanation

Screenshots/Screencaps

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jul 10, 2023

Gudahtt Jul 10, 2023

Choose a reason for hiding this comment

darkwing Jul 11, 2023

Choose a reason for hiding this comment

legobeat Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Gudahtt Jul 11, 2023

Choose a reason for hiding this comment

Gudahtt commented Jul 11, 2023

metamaskbot commented Jul 12, 2023

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

metamaskbot commented Jul 12, 2023

metamaskbot commented Jul 13, 2023

danjm commented Jul 13, 2023

legobeat Jul 11, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading