Move state update call after networkClient is instantiated and added to registry in upsertNetworkConfiguration

[adonesky1]

@metamask/network-controller

Changed:

• Moves state update call after networkClient was instantiated so that NetworkController:stateChange event can be safely used to identify when new networkClients have been added and are available for use

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

[mcmire]

Can you tell me what use case this is intended to solve? The network configuration has nothing to do with the network client. If you wanted to wait for the network configuration to change then the current state update works fine. If you wanted to wait until a network client was ready, that's what networkDidChange does, and you should use that instead of waiting for state change. Is this necessary because we use the ID of the network configuration as the network client ID or something?

[mcmire]

@adonesky1 and I spoke more about this and I agree conceptually with this change, so I'm approving so we can merge this. I'll explain more in the next comment.

[mcmire]

For more context, my concern with using network configuration to discern whether the provider for the newly added network is available comes from past troubles with providerConfig combined with a desire to improve data integrity between state data and non-state data.

There are a lot of places in our code that listen for NetworkController state change to know when the chain ID is changed. Sometimes they do this so they can keep data organized by chain in state, and sometimes they need the chain ID to feed it to some external service, but sometimes they do this as a way of knowing when the provider is updated so that they can reset an EthQuery or Web3Provider instance (something where there is definitely a better approach, but that's beside the point). They do this currently by listening to providerConfig. While this appears to work in practice, this shouldn't! providerConfig gets set first and then the provider is updated, so a better way to track the provider would be to listen to networkDidChange, which always fires after the provider is updated. As a plus, networkDidChange is a very clear name and it is difficult to misunderstand what this means.

I see networkConfigurations as an analog to providerConfig, and I want to avoid the same mistakes that we made with providerConfig. Of course, if we updated providerConfig after the provider was updated, that would solve the problem, but using an event rather than state seems to be less of a footgun, and it communicates the intent to consumers better.

Besides this, there is the fact that while networkConfigurations is kept in state, the network client registry is not. So we need to have a guarantee that there is some link between the two, and TypeScript seems like a good way to do this. We can sort of make this guarantee today, but it is not perfect. If we treat networkConfigurations as the source of truth, and if we know that a network client inherits its ID from the network configuration, then we can effectively treat the network configuration ID as a network client ID and feed it directly into getNetworkClientById. The problem with this is that in TypeScript, NetworkClientId and NetworkConfigurationId both resolve to string, and that probably shouldn't be the case if they're supposed to be different concepts, because it means we can't enforce that they are different in TypeScript. Or, if they are supposed to be the same, then we should unify them by defining a "network ID" concept and then consciously sharing that name: call the property on NetworkConfiguration networkId instead of id, and then identify network clients by network ID instead of network client ID. Perhaps we could do this when we incorporate built-in networks into networkConfigurations. Anyway, this problem with naming seems like a tiny thing, but I feel like allows us to do what we seem to want to do, which is use to state as the ultimate source of truth and really trust that the NetworkController works in a cohesive fashion.