Group network configurations by chain #4286
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
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
mcmire
force-pushed
the
group-network-configurations-by-chain
branch
from
eb2e882
to
7f4d92b
Compare
mcmire
force-pushed
the
group-network-configurations-by-chain
branch
5 times, most recently
from
6b46044
to
f208b5b
Compare
mcmire
commented
May 22, 2024
| @@ -1,4 +1,38 @@ | |||
| export * from './NetworkController'; | |||
| export type { | |||
There was a problem hiding this comment.
I am explicitly naming these exports because there are some types that I am technically exporting but I don't want to make public (and we are introducing this pattern for each controller anyway).
No types that are exported today should be missing from this list except for the ones noted below.
| NetworkControllerOptions, | ||
| } from './NetworkController'; | ||
| export { | ||
| getDefaultNetworkControllerState, |
There was a problem hiding this comment.
getDefaultNetworkControllerState and RpcEndpointType should be the only new exports (in addition, defaultState has been removed).
| @@ -926,79 +949,103 @@ describe('TransactionController Integration', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('when changing rpcUrl of networkClient', () => { | |||
There was a problem hiding this comment.
This describe is inaccurate now, since the RPC URL of a network configuration is no longer being changed, a new RPC endpoint is being added to an existing network instead. And "when a new network is added" already takes care of this.
| ], | ||
| }); | ||
| const otherGoerliRpcEndpoint = | ||
| updatedGoerliNetworkConfiguration.rpcEndpoints.find((rpcEndpoint) => { |
There was a problem hiding this comment.
I was considering adding a new method to NetworkController to make finding the network client ID associated with a RPC URL easier, but it seems like something we only need in tests, so minimally we could add a new test helper, but this is only happening a few times so it didn't seem worth it to do this.
This was referenced May 22, 2024
mcmire
commented
May 23, 2024
mcmire
commented
May 23, 2024
| - The `defaultRpcEndpointUrl` of a network configuration in `networkConfigurationsByChainId` must match an entry in its `rpcEndpoints`. | ||
| - `selectedNetworkClientId` must match the `networkClientId` of an RPC endpoint in `networkConfigurationsByChainId`. | ||
| - **BREAKING:** Update `getNetworkConfigurationByNetworkClientId` so that when given an Infura network name (that is, a value from `InfuraNetworkType`), it will return a masked version of the RPC endpoint URL for the associated Infura network ([#4268](https://github.com/MetaMask/core/pull/4286)) | ||
| - If you want the unmasked version, you'll need the `url` property from the network _client_ configuration, which you can get by calling `getNetworkClientById` and then accessing the `configuration` property off of the network client. |
There was a problem hiding this comment.
As an alternative to requiring this step, I could make getNetworkConfigurationByNetworkClientId (or any other methods that return a single network configuration or multiple network configurations) unmask the Infura API URL automatically. Thoughts?
MajorLift
reviewed
Jun 6, 2024
Comment on lines
+301
to
306
| export type NetworksMetadata = Record<NetworkClientId, NetworkMetadata>; | ||
|
|
There was a problem hiding this comment.
tangent: This is probably outside of the PR's scope, but looks like NetworkProperties might be a better name for NetworkMetadata, which could be confused with NetworkStateMetadata.
In general, we might want to look into categorizing our data types and properties so that they consistently distinguish between the following categories. TransactionMeta, for example, might benefit from being broken up into a few types:
- Metadata: Static descriptors (e.g.
NetworkClientId,NetworkConfiguration['nativeTokenName']) - State: Transient state variables (
NetworkMetadata['status']) - Properties: Inherent/immutable properties that affect behavior (e.g.
NetworkMetadata['EIPS']) - Configurations: Assignable/mutable properties that affect behavior (e.g.
NetworkConfiguration['rpcEndpoints'])
There was a problem hiding this comment.
Good idea to rename NetworkMetadata. I have never quite liked that name.
Also a good idea to think about how state properties ought to be categorized. Knowing that some state properties are transient and can easily be recreated, and that some properties are expected to be immutable is important.
mcmire
force-pushed
the
remove-provider-config
branch
4 times, most recently
from
cd8a065
to
65fa9a8
Compare
3 tasks
Currently, in the client, it is possible to have multiple networks with
different RPC endpoint URLs representing the same chain. This creates a
problem because if all we have is a chain ID, we don't know which URL to
use for requests.
To solve this, we plan on consolidating the UX on the client side such
that each network corresponds to exactly one chain. Users can then
select which default RPC URL they'd like to use for requests. This
commit implements the controller changes necessary to support this UX.
Here are some more details on the changes here:
- The concept of a network configuration has been repurposed such that
instead of representing an RPC endpoint, it now represents a whole
chain.
- A network configuration may have multiple RPC endpoints, and
one of them must be designated as the default.
- Some RPC endpoints are special in that they represent Infura API
URLs; these have the same object shape as "non-Infura" (custom) RPC
endpoints, but the Infura project ID is hidden and injected into the
RPC URL when creating the network client.
- There is no longer a 1-to-1 relationship between network
configuration and network client; rather, the 1-to-1 relationship
exists between RPC endpoint and network client. This means that the
ID of the network client which is created for an RPC endpoint
is stored on that RPC endpoint instead of the whole network
configuration.
- The `networkConfigurations` state property has been replaced with
`networkConfigurationsByChainId`. This continues to be an object, but
the data inside is organized such that network configurations are
identified by chain ID instead of network client ID as they were
previously.
- The methods `upsertNetworkConfiguration` and
`removeNetworkConfiguration` have been removed. These methods always
did more than simply add or remove a network configuration; they also
updated the registry of network clients. Instead, these methods have
been replaced with `addNetwork`, `updateNetwork`, and `removeNetwork`.
- `addNetwork` creates new network clients for each RPC endpoint in
the given network configuration.
- `updateNetwork` takes a chain ID referring to a network
configuration and a draft version of that network configuration,
and adds or removes network clients for added or removed RPC
endpoints.
- `removeNetwork` takes a chain ID referring to a network
configuration and removes the network clients for each of its RPC
endpoints.
- In addition, due to the changes to network configuration itself, there
are new restrictions on `networkConfigurationsByChainId`, which are
validated on initialization and on update. These are:
- The network controller cannot be initialized with an empty
collection of network configurations. This is because there must be
a selected network client so that consumers have a provider to use
out of the gate.
- Consequently, the last network configuration cannot be removed.
- The chain ID of a network configuration must match the same chain
that it's filed under in `networkConfigurationsByChainId`.
- No two network configurations can have the same chain ID.
- A RPC endpoint in a network configuration must have a well-formed
URL.
- A network configuration cannot have duplicate RPC endpoints.
- No two RPC endpoints (regardless of network configuration) can have
the same URL. Equality is currently determined by normalizing URLs
as per RFC 3986 and may include data like request headers in the
future.
- If a network configuration has an Infura RPC endpoint, its chain ID
must match the set chain ID of the network configuration.
- Changing the chain ID of a network configuration is possible, but
any existing Infura RPC endpoint must be replaced with the one
that matches the new chain ID.
- No two RPC endpoints (regardless of network configuration) can have
the same network client ID.
- Finally, the `trackMetaMetricsEvent` option has been removed from the
constructor. This was previously used in `upsertNetworkConfiguration`
to create a MetaMetrics event when a new network added, but I've added
a new event `NetworkController:networkAdded` to allow the client to do
this on its own accord.
This reverts commit 417ab4ef6f6c2fe6e9dcabb93f0b4f3498d1145b.
mcmire
force-pushed
the
group-network-configurations-by-chain
branch
from
5897b85
to
c5f4534
Compare
Co-authored-by: Jongsun Suh <jongsun.suh@icloud.com>
bergeron
reviewed
Aug 26, 2024
| BlockTrackerProxy, | ||
| ProviderProxy, | ||
| AddNetworkFields, | ||
| UpdateNetworkFields, |
There was a problem hiding this comment.
Suggested change
| UpdateNetworkFields, | |
| UpdateNetworkFields, | |
| RpcEndpoint, |
RpcEndpoint would be convenient to export
bergeron
reviewed
Aug 27, 2024
|
|
||
| this.#networkConfigurationsByNetworkClientId = | ||
| buildNetworkConfigurationsByNetworkClientId( | ||
| this.state.networkConfigurationsByChainId, |
There was a problem hiding this comment.
Suggested change
| this.state.networkConfigurationsByChainId, | |
| state.networkConfigurationsByChainId, |
I think this fixes a sneaky bug in the context of adding or updating a network.
The network is added to state.networkConfigurationsByChainId a few lines above. But it's not yet added to this.state.networkConfigurationsByChainId at this point, because we're still inside an update function. Without this fix, the new network does not get passed to buildNetworkConfigurationsByNetworkClientId.
There was a problem hiding this comment.
I committed this change in b59ee0c so I can publish a preview version containing it
There was a problem hiding this comment.
Had to take another pass and fix it this way instead: 02e7ff9
|
@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. |
Merged
7 tasks
|
@mikesposito The only remaining thing I can't figure out, is an error The easiest repro is on thrown by TokenDetectionController when switching chains here:
It's possible it was introduced by b59ee0c. Perhaps using the draft state passed to Edit: I think I've fixed it in 02e7ff9 by performing |
|
@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. |
Thanks for taking care of that, the fix makes sense. |
## Explanation <!-- Thanks for your contribution! 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 changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> Currently, `SelectedNetworkController` subscribes to the `NetworkController:stateChange` event to make sure that network client ids associated with managed domains stay valid. Though, we are currently checking on the specific immer patch applied to the NetworkController state: this means that based on how the state draft was manipulated on the origin controller, we may or may not capture the change and react to it. To fix it, now `SelectedNetworkController` will look at the actual content of the updated state, instead of relying on immer patches. To reduce the amount of event instances received by the controller, a selector is applied to the subscription, filtering out irrelevant instances. ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/network-controller` - **ADDED**: Added `getNetworkConfigurations`, `getAvailableNetworkClientIds` and `selectAvailableNetworkClientIds` selectors - These new selectors can be applied to messenger event subscriptions ### `@metamask/selected-network-controller` No consumer-faced changes ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate
mikesposito
reviewed
Sep 2, 2024
Comment on lines
+195
to
+209
| (availableNetworkClientIds) => { | ||
| // if a network is updated or removed, update the networkClientId for all domains | ||
| // that were using it to the selected network client id | ||
| const { selectedNetworkClientId } = this.messagingSystem.call( | ||
| 'NetworkController:getState', | ||
| ); | ||
| Object.entries(this.state.domains).forEach( | ||
| ([domain, networkClientIdForDomain]) => { | ||
| if (!availableNetworkClientIds.includes(networkClientIdForDomain)) { | ||
| this.setNetworkClientIdForDomain(domain, selectedNetworkClientId); | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
| selectAvailableNetworkClientIds, |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
bergeron
approved these changes
Sep 3, 2024
|
This is ready to go on extension. Mobile is still in progress, but extension needs to go first |
matthewwalsh0
approved these changes
Sep 3, 2024
bergeron
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Sep 20, 2024
## **Description** This PR upgrades the network controller, and enables a new UI for adding and editing networks. The network controller changes are described in full in MetaMask/core#4286 In short, there's now a single network per chain id. Multiple RPC endpoints for a chain are now represented as an array under the single network configuration, instead of being separate networks. Each network has some RPC endpoint chosen as the default. Also the built in Infura networks are now represented in state, where they were not before. A migration is added to transform the network controller state to this new format. For the UI, networks are now added, edited, and deleted directly in the network list. Networks are no longer edited via the settings page. Users with multiple RPC endpoints per chain are shown a modal upon upgrade, allowing them to select a different endpoint as the default. The UI for `wallet_addEthereumChain` is changed, to message that users may be adding an additional endpoint to an existing network, rather than adding a new network. This PR contains both the controller upgrade and UI, because it's not possible to perfectly recrate the old UI with the new state or vice versa. To minimize changes, some selectors are shimmed to return equivalent data. This includes `getProviderConfig` and `getCurrentNetwork`. Other selectors have been removed in favor of selecting the new state, as there was no behavior that satisfied every caller. This includes `getNetworkConfigurations` and its dependents like `getNonTestNetworks` `getTestNetworks`. Places listing networks now go through the new `getNetworkConfigurationsByChainId`. [](https://codespaces.new/MetaMask/metamask-extension/pull/26433?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Upgrade from an older build with custom networks 2. Verify they all still appear in the network dropdown - Verify multiple endpoints for the same chain have been merged into 1 network with multiple RPC endponits 3. Click the network dropdown 4. Try: - Adding popular networks - Adding a custom network - Editing an existing network - Deleting an existing network 5. Try adding a new network via a dapp 6. Try adding a new RPC endpoint to an existing network via a dapp ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** https://github.com/user-attachments/assets/dd215f30-9c83-4490-83c3-8aaf39412763 ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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. --------- Co-authored-by: salimtb <salim.toubal@outlook.com> Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com> Co-authored-by: David Walsh <davidwalsh83@gmail.com> Co-authored-by: Howard Braham <howrad@gmail.com> Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
](https://codespaces.new/MetaMask/metamask-extension/pull/26433?quickstart=1){kind=link}
stormz310
approved these changes
Oct 22, 2024
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
Currently, in the client, it is possible to have multiple networks with different RPC endpoint URLs representing the same chain. This creates a problem because if all we have is a chain ID, we don't know which URL to use for requests.
To solve this, we plan on consolidating the UX on the client side such that each network corresponds to exactly one chain. Users can then select which default RPC URL they'd like to use for requests. This commit implements the controller changes necessary to support this UX.
Here are some more details on the changes here:
The concept of a network configuration has been repurposed such that instead of representing an RPC endpoint, it now represents a whole chain.
The
networkConfigurationsstate property has been replaced withnetworkConfigurationsByChainId. This continues to be an object, but the data inside is organized such that network configurations are identified by chain ID instead of network client ID as they were previously.The methods
upsertNetworkConfigurationandremoveNetworkConfigurationhave been removed. These methods always did more than simply add or remove a network configuration; they also updated the registry of network clients. Instead, these methods have been replaced withaddNetwork,updateNetwork, andremoveNetwork.addNetworkcreates new network clients for each RPC endpoint in the given network configuration.updateNetworktakes a chain ID referring to a network configuration and a draft version of that network configuration, and adds or removes network clients for added or removed RPCendpoints.
removeNetworktakes a chain ID referring to a network configuration and removes the network clients for each of its RPC endpoints.In addition, due to the changes to network configuration itself, there are new restrictions on
networkConfigurationsByChainId, which are validated on initialization and on update. These are:networkConfigurationsByChainId.Finally, the
trackMetaMetricsEventoption has been removed from the constructor. This was previously used inupsertNetworkConfigurationto create a MetaMetrics event when a new network added, but I've added a new eventNetworkController:networkAddedto allow the client to do this on its own accord.References
Fixes #4189.
Fixes #3793.
Changelog
(Updated in the PR.)
Checklist