Remove NetworkId by jiexi · Pull Request #20652 · MetaMask/metamask-extension

jiexi · 2023-08-29T21:25:45Z

Explanation

Wallets shouldn't be directly concerned about the network ID as this more of a p2p concept for gossip. What wallets really care about is chain ID as that is the correct value to use to identify a chain, build transactions, etc. Although these two values usually match (ignoring hex/dec formatting), there are exceptions.

We want to remove usage of networkId and replace it with chainId where appropriate and necessary.

This was a generally straight forward change to make, except for the following cases:

networkVersion on the window.ethereum provider object
metamaskNetworkId on TransactionMeta objects

~~In both cases, we now use static mappings to assist in covering chainId <-> networkId edge cases.~~ See comments in this PR for more elaboration and the core PR linked below.

Fixes mmp-1068
See: core-1633
See: https://github.com/MetaMask/smart-transactions-controller

Screenshots/Screencaps

Before

After

Manual Testing Steps

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-08-29T21:26:00Z

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.

socket-security · 2023-08-30T18:21:02Z

Updated and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/selected-network-controller	1.0.0...2.0.1	None	`+0/-1`	27.3 kB	metamaskbot

🚮 Removed packages: @metamask/network-controller@12.2.0

jiexi · 2023-08-30T22:04:07Z

@metamaskbot update-policies

metamaskbot · 2023-08-30T22:17:29Z

Policies updated

codecov · 2023-08-30T23:49:41Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (fd84e8b) 68.54% compared to head (34fe4b8) 68.54%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #20652      +/-   ##
===========================================
+ Coverage    68.54%   68.54%   +0.01%     
===========================================
  Files         1029     1030       +1     
  Lines        41013    41037      +24     
  Branches     10963    10968       +5     
===========================================
+ Hits         28109    28128      +19     
- Misses       12904    12909       +5

Files	Coverage Δ
app/scripts/controllers/ens/ens.js	`78.95% <100.00%> (+1.17%)`	⬆️
app/scripts/controllers/ens/index.js	`80.77% <100.00%> (+0.77%)`	⬆️
app/scripts/controllers/swaps.js	`81.14% <100.00%> (ø)`
...s/transactions/EtherscanRemoteTransactionSource.ts	`89.74% <100.00%> (ø)`
...trollers/transactions/IncomingTransactionHelper.ts	`93.97% <100.00%> (-0.20%)`	⬇️
app/scripts/controllers/transactions/index.js	`73.02% <100.00%> (+0.22%)`	⬆️
...ripts/controllers/transactions/tx-state-manager.js	`92.00% <100.00%> (-0.27%)`	⬇️
app/scripts/lib/setupSentry.js	`41.07% <ø> (ø)`
app/scripts/migrations/101.ts	`100.00% <100.00%> (ø)`
app/scripts/migrations/index.js	`100.00% <ø> (ø)`
... and 17 more

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

metamaskbot · 2023-08-31T00:04:11Z

Builds ready [3b40e6b]

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
- common: 0, 1, 2, 3, 4, 5, 6
- 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 (1568 ± 94 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	107	183	127	22	10
		domContentLoaded	1412	2125	1566	192	92
		load	1412	2151	1568	195	94
		domInteractive	1412	2124	1566	191	92

Bundle size diffs [🚀 Bundle size reduced!]

background: -926 Bytes (-0.03%)
ui: -801 Bytes (-0.01%)
common: -1.33 KiB (-0.03%)

jiexi · 2023-09-05T16:57:53Z

We expose networkVersion on the provider via window.ethereum.networkVersion. As always, since this has been exposed to the world, some dapps may rely on it. EIP1193 does not specify exposing this as a requirement of spec. We have a few options:

Remove it. This is a breaking change, but can be worked around by making an async net_version request
Keep it. We still need to make the request for net_version ourselves.
We can do that in MetaMaskController (cache it, making only one request per chain change event) OR
We can keep NetworkController.networkId around, but rename it to deprecatedNetworkId to make it very obvious developers should not be using it internally, and of course we will keep the changes in this PR that actual replace/remove its usage in extension

EDIT: Opting for a mix of 3 and 4. Exposed NetworkController.deprecatedGetNetworkId and will use it to fetch and cache the networkId value used in metamask_chainChanged events in MetaMaskController

EDIT2: See below

pedronfigueiredo · 2023-10-17T17:17:15Z

Should we include a migration to remove networkId from the NetworkController state?

adonesky1 · 2023-10-17T17:46:09Z

@metamaskbot update-policies

metamaskbot · 2023-10-17T17:59:56Z

Policies updated

jiexi · 2023-10-17T19:55:11Z

@metamaskbot update-policies

metamaskbot · 2023-10-17T20:09:20Z

Policies updated

jiexi · 2023-10-17T21:55:21Z

Should we include a migration to remove networkId from the NetworkController state?

Yeah, we should be tidy. My bad. Added here: 1c3bb66

metamaskbot · 2023-10-18T17:44:29Z

Builds ready [34fe4b8]

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
- common: 0, 1, 2, 3, 4, 5, 6
- 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 (1040 ± 371 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	85	122	103	12	6
		domContentLoaded	71	119	93	15	7
		load	82	1814	1040	772	371
		domInteractive	71	119	93	15	7

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

background: 178.03 KiB (4.63%)
ui: -1.3 KiB (-0.02%)
common: -102.71 KiB (-2.08%)

jiexi · 2023-10-18T19:04:35Z

I guess this assumes that all chains support chain IDs? I mean they ought to, but is there any reason why we shouldn't do this?

All EVM chains support chain ID. I'm unsure of other chains. I can't think of a good reason not to do this besides not changing things if they aren't broken. This doesn't buy us any new features, just cleans up some tech debt

adonesky1 · 2023-10-18T19:34:22Z

package.json

    "@metamask/safe-event-emitter": "^2.0.0",
    "@metamask/scure-bip39": "^2.0.3",
-    "@metamask/selected-network-controller": "^1.0.0",
+    "@metamask/selected-network-controller": "^2.0.0",


[nit] Just curious, is this required for this PR?

selected-network-controller v1.0.0 had a peerDep of network-controller ^12.1.2.

v2.0.0 has a peerDep of 13

adonesky1

LGTM. Obviously a ton of the same little changes here so hard to feel super confident without spending many hours pouring over each one in context. So hoping we can get at least a few more sets of eyes and some thorough QA done before we merge this.

adonesky1 · 2023-10-18T19:43:07Z

@tmashuang do you have some bandwidth to give this a thorough QA?

jiexi · 2023-10-18T19:53:42Z

Thomas and I took another pass on this. I think we're good on the QA side

jiexi added 6 commits August 29, 2023 11:30

Core preview build packages

a68dacf

Remove networkId from transactions

8da19b1

Remove networkId from smart-transactions-controller

8a511da

fix mmc test

5fc2e60

update jsdoc in background.js

05b8946

Merge branch 'develop' into jl/remove-network-id

8ca270c

jiexi added 5 commits August 29, 2023 16:47

fix remaining specs

5436611

Update test fixtures

e38946e

Cleanup network constants. Fix remaining specs

18c6564

Merge branch 'develop' into jl/remove-network-id

7e875f1

temporarily remove ppom package

c611112

jiexi added 5 commits August 30, 2023 13:58

PPOM temporary

72c7d55

temporarily disable fitness sinon assert

3d2514c

yarn dedupe

087f301

lint

0b45205

Merge branch 'develop' into jl/remove-network-id

dfca596

jiexi added the team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead label Aug 30, 2023

revert chromedriver

0e72501

Update LavaMoat policies

f03d172

jiexi added 2 commits August 30, 2023 15:33

restore networkVersion

4a9facf

fix ens domain resolution

3b40e6b

re-enable assert sinon fitness

6dc3702

jiexi mentioned this pull request Sep 2, 2023

Migrate MetaMaskController from mocha to jest #20696

Merged

7 tasks

Update LavaMoat policies

0b7c955

jiexi added 2 commits October 17, 2023 11:31

Merge branch 'develop' into jl/remove-network-id

e1402f8

lint

94286c3

Update LavaMoat policies

3a91436

jiexi added 2 commits October 17, 2023 14:35

Remove networkId checks from provider-api e2e spec

25022c8

add migration

1c3bb66

jiexi force-pushed the jl/remove-network-id branch from 7371e74 to 8979f84 Compare October 17, 2023 22:42

include the migration...

bb46c3e

jiexi force-pushed the jl/remove-network-id branch from 8979f84 to bb46c3e Compare October 17, 2023 23:23

pedronfigueiredo approved these changes Oct 18, 2023

View reviewed changes

adonesky1 and others added 2 commits October 18, 2023 10:03

Merge branch 'develop' into jl/remove-network-id

d0cc587

Merge branch 'develop' into jl/remove-network-id

34fe4b8

adonesky1 reviewed Oct 18, 2023

View reviewed changes

adonesky1 approved these changes Oct 18, 2023

View reviewed changes

adonesky1 requested a review from tmashuang October 18, 2023 19:42

jiexi removed the needs-qa Label will automate into QA workspace label Oct 18, 2023

jiexi merged commit 703f4ba into develop Oct 18, 2023

jiexi deleted the jl/remove-network-id branch October 18, 2023 21:03

github-actions bot locked and limited conversation to collaborators Oct 18, 2023

metamaskbot added the release-11.5.0 Issue or pull request that will be included in release 11.5.0 label Oct 18, 2023

Conversation

jiexi commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Aug 29, 2023

Uh oh!

socket-security bot commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiexi commented Aug 30, 2023

Uh oh!

metamaskbot commented Aug 30, 2023

Uh oh!

codecov bot commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Aug 31, 2023

Uh oh!

jiexi commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedronfigueiredo commented Oct 17, 2023

Uh oh!

adonesky1 commented Oct 17, 2023

Uh oh!

metamaskbot commented Oct 17, 2023

Uh oh!

jiexi commented Oct 17, 2023

Uh oh!

metamaskbot commented Oct 17, 2023

Uh oh!

jiexi commented Oct 17, 2023

Uh oh!

metamaskbot commented Oct 18, 2023

Uh oh!

jiexi commented Oct 18, 2023

Uh oh!

adonesky1 Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

jiexi Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

adonesky1 commented Oct 18, 2023

Uh oh!

jiexi commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jiexi commented Aug 29, 2023 •

edited

Loading

socket-security bot commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

jiexi commented Sep 5, 2023 •

edited

Loading