feat: Add MultichainNetworkController to handle both EVM and non-EVM network and account switching

[Cal-L]

Explanation

This PR updates both the MultichainNetworkController and AccountsController to handle network switching as well as account switching. The logic handles the following logic:

• Switching accounts on AccountsController will notify MultichainNetworkController to update if the account belongs to evm vs non-evm network (MultichainNetworkController subscribes to AccountsController event)
• Switching between networks on MultichainNetworkController will notify AccountsController to update accounts based on which network the account belongs to (AccountsController subscribes to MultichainNetworkController event)

References

Fixes https://github.com/MetaMask/accounts-planning/issues/804

Changelog

@metamask/accounts-controller

• BREAKING:
  • Added MultichainNetworkSetActiveNetworkEvent to allowed events. This is used to subscribe to the setActiveNetwork event from the MultichainNetworkController and is responsible for updating selected account based on network changes (both EVM and non-EVM).

@metamask/multichain-network-controller

• ADDED:
  • Allowed actions - NetworkControllerGetStateAction | NetworkControllerSetActiveNetworkAction. The MultichainNetworkController acts as a proxy for the NetworkController and will update it based on EVM network changes.
  • Allowed events - AccountsControllerSelectedAccountChangeEvent to allowed events. This is used to subscribe to the selectedAccountChange event from the AccountsController and is responsible for updating active network based on account changes (both EVM and non-EVM).

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
• I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

[tommasini]

This looks amazing! Lets align today with the review made on the PR #5209

[socket-security]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@solana/[email protected]	Transitive: environment	+8	2.82 MB	lorisleiva

View full report↗︎

[tommasini]

should we include tests folder?

[mcmire]

We do this in other projects that have a tests/ directory, so yes, I think so:

Suggested change

	"include": ["../../types", "./src"]
	"include": ["../../types", "./src", "./tests"]

[mcmire]

Hello! I left some comments. I'm still trying to understand the changes introduced here. I know more changes are coming, but I am still curious why some things are being introduced here without being explicitly used, when they could be introduced later. Other than that I had some improvements we could make to the types, and some naming adjustments.

[mcmire]

Hmm, I would expect an action to be called setActiveNetwork, but not an event. Besides this, it looks like we already have an action called MultichainNetworkController:setActiveNetwork, so this could be confusing.

Perhaps we want to call this networkDidChange to mimic NetworkController?

[mcmire]

This could create an API networkConfiguration.blockExplorers.urls. Is that what we want? Usually I would expect a property that ends in a plural to be an array or a collection of some kind. But here it seems we are using blockExplorers as just a grouping.

There are only two properties here, what are our thoughts on using networkConfiguration.blockExplorerUrls and networkConfiguration.defaultBlockExplorerUrlIndex?

Besides this, this API diverges from NetworkController so it adds overhead when translating between the two.

If we really want this, then perhaps this property should be called defaultUrlIndex to reflect that it refers to the urls property?

[mcmire]

Same commentary as for blockExplorers.

If we really want this then perhaps this should be called defaultUrlIndex?

[mcmire]

Should we use CaipChainId for the keys here?

Suggested change

	string,
	CaipChainId,

[mcmire]

Since this is a boolean, what do you think about prefixing this with is? I think we can also tweak this a bit. And what do you think about inverting it?

Suggested change

	nonEvmSelected: boolean;
	isEvmNetworkSelected: boolean;

[mcmire]

We should probably make this a proper guideline, but something that we should really try to do when writing controllers is ensure that state is coherent at all times. That is, if there is a property in state that connects to another property, that connection should be valid at all times.

In this case, selectedMultichainNetworkChainId is really a connection to an entry in multichainNetworkConfigurationsByChainId. That is, a consumer should always be able to fetch the network configuration for a chain ID by saying state.multichainNetworkConfigurationsByChainId[state.selectedMultichainNetworkChainId].

As it is, if the consumer were to create a MultichainNetworkController without passing any initial state, then the state of the resulting controller would not be coherent, because although state.selectedMultichainNetworkChainId would be present, state.multichainNetworkConfigurationsByChainId[state.selectedMultichainNetworkChainId] would be undefined.

In the NetworkController we solved this by prepopulating networkConfigurationsByChainId with known networks, one of which is Mainnet. This way we can default selectedNetworkClientId to Mainnet and then the consumer can call controller.getNetworkConfigurationByNetworkClientId(state.selectedNetworkClientId) and then get the resulting network configuration.

Can we prepopulate multichainNetworkConfigurationsByChainId with Solana and Bitcoin? Or, alternatively, maybe we cannot default selectedMultichainNetworkChainId to anything since we won't know what the wallet will have?

[mcmire]

What are your thoughts on using SCREAMING_SNAKE_CASE for these constants?

Suggested change

	export const btcNativeAsset = `${BtcScope.Mainnet}/slip44:0`;
	export const BTC_NATIVE_ASSET = `${BtcScope.Mainnet}/slip44:0`;

[mcmire]

Suggested change

	export const solNativeAsset = `${SolScope.Mainnet}/token:EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v`;
	export const SOL_NATIVE_ASSET = `${SolScope.Mainnet}/token:EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v`;

[mcmire]

What are your thoughts on using SCREAMING_SNAKE_CASE and also using the word "available" (to communicate that these are things that you could initialize the state with, but you don't have to use all of them).

Suggested change

	export const multichainNetworkConfigurations: Record<
	export const AVAILABLE_MULTICHAIN_NETWORK_CONFIGURATIONS: Record<

[mcmire]

Should we throw a specific error, so that if it is a bug, we have some more information?

[mcmire]

@Cal-L Can you run yarn update-readme-content when you get a chance? This should add the controller to the README and update the graph there.