Make findNetworkClientIdByChainId prefer the current client if multiple match

[bergeron]

Explanation

Modifies the network controller's findNetworkClientIdByChainId to prefer the current network client if multiple exist with the given chain id.

This fix was prompted by MetaMask/metamask-extension#22992. The setup is multiple networks/RPCs with the same chain ID 1. When extension triggers a token detection, it was doing so on the built-in mainnet RPC instead of the currently selected RPC.

I am not 100% sure about this fix.

References

Bug report: MetaMask/metamask-extension#22992

Changelog

@metamask/network-controller

• Changed: findNetworkClientIdByChainId will prefer the currently selected network client if multiple exist with the given chain id.

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

[bergeron]

cc @jiexi an edge case of our use of findNetworkClientIdByChainId in MetaMask/metamask-extension#22898 and MetaMask/metamask-extension#22814

[bergeron]

I initially tried return this.state.selectedNetworkClientId. But I found that during a chain switch, selectedNetworkClientId is still on the old chain while providerConfig is on the new chain:

Therefore had to rely on providerConfig

[jiexi]

Makes sense. A NetworkController:stateChange event for the providerConfig changing fires before a stateChange event for the selectedNetworkClientId changing.

I feel the order of events we fire is incorrect. Currently when the active network changes events are fired in the following order:

1. NetworkController:stateChange for the providerConfig change
2. NetworkController:networkWillChange
3. NetworkController:stateChange for the selectedNetworkClientId change and default networksMetadata
4. NetworkController:networkDidChange
5. NetworkController:stateChange for updating networksMetadata with EIP1559 compatibility
6. NetworkController:infuraIsUnblocked

It seems like steps 1 and 3 should be combined and fired after step 2. So it should look like this:

1. NetworkController:networkWillChange
2. NetworkController:stateChange for the providerConfig, selectedNetworkClientId, and default networksMetadata
3. NetworkController:networkDidChange
4. NetworkController:stateChange for updating networksMetadata with EIP1559 compatibility
5. NetworkController:infuraIsUnblocked

@Gudahtt @mcmire @adonesky1 @shanejonas @BelfordZ thoughts?

[mcmire]

I don't think it matters when the stateChange occurs, because clients shouldn't be using that event to know whether the network changes, they should be using networkDidChange — and that already works today. But grouping like state changes together in the same update does make sense, so that we can guarantee that providerConfig matches selectedNetworkClientId.

[adonesky1]

This fix was prompted by MetaMask/metamask-extension#22992. The setup is multiple networks/RPCs with the same chain ID 1. When extension triggers a token detection, it was doing so on the built-in mainnet RPC instead of the currently selected RPC.

Can you help me understand how ensuring the active networkClient is used for detection fixes this bug? I'm not saying we shouldn't address the mismatch but I am a bit confused about how it resolves the issue in this case

[bergeron]

Can you help me understand how ensuring the active networkClient is used for detection fixes this bug?

@adonesky1 Does this help?

• A network switch occurs
• DetectTokensController triggers a restartTokenDetection for the new chain.
• findNetworkClientIdByChainId converts from chainId to network client id
• That network client id is passed to AssetsContractController.getBalancesInSingleCall

In the old code, the chosen network client is an arbitrary one that matches the chain id. This may not be the active network if there are multiple with that chain id. Causing the balance check to run on a different RPC than user selected.

The new code gives precedence to the active network if multiple match the chain id.

In the linked bug, someone is using a tenderly fork which made it more obvious that the wrong RPC was being used.

[mcmire]

Hmm... does it make sense to fall back to this.state.providerConfig.type here? Despite the TypeScript types matching, this.state.providerConfig.type is not quite the same thing as the network client ID. To be specific, NetworkType (which is the type property is) includes "rpc", but there is no network client registered under "rpc". So if you tried to feed this into getNetworkClientById there is a small change you might get an error.

What about:

Suggested change

	if (this.state.providerConfig.chainId === chainId) {
	return this.state.providerConfig.id ?? this.state.providerConfig.type;
	if (
	this.state.providerConfig.chainId === chainId &&
	(
	this.state.providerConfig.id ??
	this.state.providerConfig.type !== NetworkType.rpc
	)
	) {
	return this.state.providerConfig.id ?? this.state.providerConfig.type;
	}

[bergeron]

Seems like a good idea. I'm trying to handle the fact that NetworkClientIds seem to come from different fields in custom vs infura. The suggested guard would prevent a bug in the (theoretical, shouldn't happen?) case of type === NetworkType.rpc but no id

[mcmire]

Yes, practically it shouldn't happen based on the logic we have in setActiveNetwork and is purely theoretical based on the fact that NetworkType is a slightly wider type than NetworkClientId.

[mcmire]

Reviewing this PR again, I'm curious if this is the right direction to take.

As far as I understand — @adonesky1 you can correct me if I'm wrong — findNetworkClientIdByChainId was created to support switchEthereumChain in extension. I don't think we should using it as much as we have elsewhere, but that's fundamentally why it exists. So if we change the behavior of this method, it changes how switchEthereumChain works, which may not be what we want.

I think DetectTokensController is where the bug really lies. There are a few problems I can see with it:

1. To know whether the network has changed, it's listening to NetworkController:stateChange. We have an event dedicated to network changes: it's called NetworkController:networkDidChange. We should be listening to this instead.
2. DetectTokensController also uses the providerConfig to get the chain ID. This is an outdated pattern, because what we really need is the network client ID, which we can get via selectedNetworkClientId.
3. At the same time, restartTokenDetection takes a chainId, but it should take a networkClientId. This way it can pass this along to detectNewTokens instead of the chainId.

So, if we make these changes in DetectTokensController then we don't have to mess with this method and thus switchEthereumChain.

For what it's worth, all of these changes have been made to TokenDetectionController in this repo, and we are very close to switching extension to use the controller instead of DetectTokensController. So, does it make sense to wait until we've done that?

[bergeron]

... we are very close to switching extension to use the controller instead of DetectTokensController. So, does it make sense to wait until we've done that?

That all makes sense to me! My remaining concern is #3923 is currently introducing the findNetworkClientIdByChainId pattern into core. But we can shift to improving that implementation instead.