Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behavior for NetworkController.findNetworkClientIdByChainId method/action is potentially surprising if more than one network has same chain ID #3686

Open
mcmire opened this issue Dec 19, 2023 · 3 comments

Comments

@mcmire
Copy link
Contributor

mcmire commented Dec 19, 2023

This method returns the network client that hits the chain with the given identifier. This is a problem because more than one network client can be assigned to the same chain ID. This method just returns the first one that matches, but that may not be the network client that the consumer expected, especially if the user has added a custom network that overrides a built-in one (for instance, Mainnet).

@Gudahtt
Copy link
Member

Gudahtt commented Dec 20, 2023

We intentionally mirrored the behavior of wallet_switchEthereumChain when implementing this. It prefers built-in networks first, and second to that it prefers older configurations to newer ones.

I wouldn't call it undefined, but it's certainly not a reasonable solution. For wallet_switchEthereumChain, I believe the plan was to update the UI to let the user choose a specific RPC endpoint. I'm not sure what other cases exist though, that solution might not be applicable to all of them.

@mcmire mcmire changed the title Behavior for NetworkController.findNetworkClientIdByChainId method/action is undefined if more than one network has same chain ID Behavior for NetworkController.findNetworkClientIdByChainId method/action is potentially surprising if more than one network has same chain ID Jan 3, 2024
@mcmire
Copy link
Contributor Author

mcmire commented Jan 3, 2024

Right, it's coming back to me now, thanks for the pointer on that. I've updated the PR title to be more accurate.

The reason I'm bringing up this method is that now that it's part of the public API, we might be tempted to use it in order to get from chain ID to network client ID (in fact I've seen it pop up in a PR recently). The existence of this method implies that there is a one-to-one mapping between chain ID and network client ID, but that's not true at all. So it's a pretty big footgun — but you wouldn't know it if you just stumbled across this.

Considering that we had to create this for wallet_switchEthereumChain, but this is the only use case, I'm wondering if we should make this method intentionally difficult to use somehow — push this on to the consumer. Or at least we should rename this method so that people understand it's not safe to use.

@Gudahtt
Copy link
Member

Gudahtt commented Jan 3, 2024

We can mark it as deprecated, that would work as a warning against using it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants