Remove networkId from NetworkController
#1633
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jiexi
commented
Aug 24, 2023
jiexi
commented
Aug 24, 2023
jiexi
commented
Aug 25, 2023
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
jiexi
commented
Aug 29, 2023
mcmire
reviewed
Aug 30, 2023
jiexi
commented
Sep 5, 2023
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
jiexi
commented
Sep 5, 2023
adonesky1
previously approved these changes
Sep 18, 2023
There was a problem hiding this comment.
LGTM! Would love to get at least one more set of eyes on it before we merge though, cc @Gudahtt @shanejonas @BelfordZ
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
adonesky1
reviewed
Sep 22, 2023
adonesky1
reviewed
Sep 22, 2023
adonesky1
approved these changes
Sep 22, 2023
BelfordZ
approved these changes
Sep 25, 2023
Merged
3 tasks
jiexi
added a commit
to MetaMask/smart-transactions-controller
that referenced
this pull request
Sep 27, 2023
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](https://medium.com/@pedrouid/chainid-vs-networkid-how-do-they-differ-on-ethereum-eec2ed41635b). We want to remove usage of `networkId` from the SmartTransactionController. * Fixes [mmp-1068](MetaMask/MetaMask-planning#1068) * See: [core-1633](MetaMask/core#1633) * See: MetaMask/metamask-extension#20652
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
## Explanation Wallets shouldn't be directly concerned about the network ID as this is more of a node 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](https://medium.com/@pedrouid/chainid-vs-networkid-how-do-they-differ-on-ethereum-eec2ed41635b). We want to remove `networkId` from the NetworkController state to encourage the replacement of networkId with chainId in any usage downstream (extension, mobile, etc). It will still be possible to get networkId using the rpc client to make a call to the `net_version` method for cases that truly still rely on it. ## References * Fixes [mmp-1068](MetaMask/MetaMask-planning#1068) ## Changelog ### `@metamask/controller-utils` - **BREAKING**: `NETWORK_ID_TO_ETHERS_NETWORK_NAME_MAP` renamed to `CHAIN_ID_TO_ETHERS_NETWORK_NAME_MAP ` and is now a mapping of `Hex` chain ID to `BuiltInNetworkName` - **REMOVED**: `NetworkId` constant and type ### `@metamask/ens-controller` - **CHANGED**: From Network state, uses `providerConfig.chainId` instead of `networkId` to determine ENS compatability ### `@metamask/network-controller` - **REMOVED**: `NetworkId` type - **REMOVED**: From `NetworkState` removed `networkId` field - **CHANGED**: No longer calls `net_version` to determine network status (only relies on `eth_getBlockByNumber` now) - **CHANGED**: For Built-in Infura Networks, `net_version` is no longer locally resolved by the scaffold middleware ### `@metamask/transaction-controller` - **BREAKING**: Uses `chainId` and `txParams.chainId` to determine if a `TransactionMeta` object belongs to the current chain. No longer considers `networkID` - **BREAKING**: `TransactionMeta.chainId` is now a required field - **REMOVED**: From `RemoteTransactionSourceRequest` removed `currentNetworkId` field - **CHANGED**: From `RemoteTransactionSource` updated `isSupportedNetwork()` params to exclude networkId - **DEPRECATED**: `TransactionMeta.networkID` is deprecated and marked readonly
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
## Explanation Wallets shouldn't be directly concerned about the network ID as this is more of a node 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](https://medium.com/@pedrouid/chainid-vs-networkid-how-do-they-differ-on-ethereum-eec2ed41635b). We want to remove `networkId` from the NetworkController state to encourage the replacement of networkId with chainId in any usage downstream (extension, mobile, etc). It will still be possible to get networkId using the rpc client to make a call to the `net_version` method for cases that truly still rely on it. ## References * Fixes [mmp-1068](MetaMask/MetaMask-planning#1068) ## Changelog ### `@metamask/controller-utils` - **BREAKING**: `NETWORK_ID_TO_ETHERS_NETWORK_NAME_MAP` renamed to `CHAIN_ID_TO_ETHERS_NETWORK_NAME_MAP ` and is now a mapping of `Hex` chain ID to `BuiltInNetworkName` - **REMOVED**: `NetworkId` constant and type ### `@metamask/ens-controller` - **CHANGED**: From Network state, uses `providerConfig.chainId` instead of `networkId` to determine ENS compatability ### `@metamask/network-controller` - **REMOVED**: `NetworkId` type - **REMOVED**: From `NetworkState` removed `networkId` field - **CHANGED**: No longer calls `net_version` to determine network status (only relies on `eth_getBlockByNumber` now) - **CHANGED**: For Built-in Infura Networks, `net_version` is no longer locally resolved by the scaffold middleware ### `@metamask/transaction-controller` - **BREAKING**: Uses `chainId` and `txParams.chainId` to determine if a `TransactionMeta` object belongs to the current chain. No longer considers `networkID` - **BREAKING**: `TransactionMeta.chainId` is now a required field - **REMOVED**: From `RemoteTransactionSourceRequest` removed `currentNetworkId` field - **CHANGED**: From `RemoteTransactionSource` updated `isSupportedNetwork()` params to exclude networkId - **DEPRECATED**: `TransactionMeta.networkID` is deprecated and marked readonly
3 tasks
jiexi
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Oct 18, 2023
## 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](https://medium.com/@pedrouid/chainid-vs-networkid-how-do-they-differ-on-ethereum-eec2ed41635b). 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](MetaMask/MetaMask-planning#1068) * See: [core-1633](MetaMask/core#1633) * See: https://github.com/MetaMask/smart-transactions-controller <!-- Thanks for the pull request. Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues, Slack conversations, Zendesk issues, user stories, etc. reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 --> ## Screenshots/Screencaps <!-- If you're making a change to the UI, make sure to capture a screenshot or a short video showing off your work! --> ### Before <!-- How did the UI you changed look before your changes? Drag your file(s) below this line: --> ### After <!-- How does it look now? Drag your file(s) below this line: --> ## Manual Testing Steps <!-- How should reviewers and QA manually test your changes? For instance: - Go to this screen - Do this - Then do this --> ## Pre-merge author checklist - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added ## Pre-merge reviewer checklist - [x] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [x] 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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com> Co-authored-by: Alex Donesky <adonesky@gmail.com>
k-g-j
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Nov 1, 2023
## 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](https://medium.com/@pedrouid/chainid-vs-networkid-how-do-they-differ-on-ethereum-eec2ed41635b). 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](MetaMask/MetaMask-planning#1068) * See: [core-1633](MetaMask/core#1633) * See: https://github.com/MetaMask/smart-transactions-controller <!-- Thanks for the pull request. Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues, Slack conversations, Zendesk issues, user stories, etc. reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 --> ## Screenshots/Screencaps <!-- If you're making a change to the UI, make sure to capture a screenshot or a short video showing off your work! --> ### Before <!-- How did the UI you changed look before your changes? Drag your file(s) below this line: --> ### After <!-- How does it look now? Drag your file(s) below this line: --> ## Manual Testing Steps <!-- How should reviewers and QA manually test your changes? For instance: - Go to this screen - Do this - Then do this --> ## Pre-merge author checklist - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added ## Pre-merge reviewer checklist - [x] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [x] 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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com> Co-authored-by: Alex Donesky <adonesky@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Wallets shouldn't be directly concerned about the network ID as this is more of a node 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
networkIdfrom the NetworkController state to encourage the replacement of networkId with chainId in any usage downstream (extension, mobile, etc). It will still be possible to get networkId using the rpc client to make a call to thenet_versionmethod for cases that truly still rely on it.References
Changelog
@metamask/controller-utilsNETWORK_ID_TO_ETHERS_NETWORK_NAME_MAPrenamed toCHAIN_ID_TO_ETHERS_NETWORK_NAME_MAPand is now a mapping ofHexchain ID toBuiltInNetworkNameNetworkIdconstant and type@metamask/ens-controllerproviderConfig.chainIdinstead ofnetworkIdto determine ENS compatability@metamask/network-controllerNetworkIdtypeNetworkStateremovednetworkIdfieldnet_versionto determine network status (only relies oneth_getBlockByNumbernow)net_versionis no longer locally resolved by the scaffold middleware@metamask/transaction-controllerchainIdandtxParams.chainIdto determine if aTransactionMetaobject belongs to the current chain. No longer considersnetworkIDTransactionMeta.chainIdis now a required fieldRemoteTransactionSourceRequestremovedcurrentNetworkIdfieldRemoteTransactionSourceupdatedisSupportedNetwork()params to exclude networkIdTransactionMeta.networkIDis deprecated and marked readonlyChecklist