feat: Add multichain-network-controller package #5209
Conversation
… to update nonEvmNetwork state variable to false; added unit tests for setActiveNetwork and both new setters
| @@ -0,0 +1,20 @@ | |||
| MIT License | |||
|
|
|||
| Copyright (c) 2024 MetaMask | |||
There was a problem hiding this comment.
Should this be 2025?
| /** | ||
| * Here we ensure that TypeScript resolves `@metamask/*` imports to the | ||
| * uncompiled source code for packages that live in this repo. | ||
| * | ||
| * NOTE: This must be synchronized with the `moduleNameMapper` option in | ||
| * `jest.config.packages.js`. | ||
| * | ||
| * NOTE 2: This is not necessary when copying this package to `packages/`. | ||
| */ | ||
| "paths": { | ||
| "@metamask/*": ["../../packages/*/src", "../*/src"] | ||
| } |
There was a problem hiding this comment.
| /** | |
| * Here we ensure that TypeScript resolves `@metamask/*` imports to the | |
| * uncompiled source code for packages that live in this repo. | |
| * | |
| * NOTE: This must be synchronized with the `moduleNameMapper` option in | |
| * `jest.config.packages.js`. | |
| * | |
| * NOTE 2: This is not necessary when copying this package to `packages/`. | |
| */ | |
| "paths": { | |
| "@metamask/*": ["../../packages/*/src", "../*/src"] | |
| } | |
There was a problem hiding this comment.
Agreed, we should not need these changes as they are provided by tsconfig.packages.json.
| }, | ||
| "references": [ | ||
| { "path": "../base-controller" }, | ||
| { "path": "../accounts-controller" }, |
There was a problem hiding this comment.
Should we remove this for now, since it's not interacting with the accounts controller yet?
There was a problem hiding this comment.
I would recommend doing so, yes, and adding it in a future PR.
There was a problem hiding this comment.
Hi @tommasini! I checked through this PR and made some suggestions on ways we could bring this controller in alignment with other controllers.
I don't know if you know about this but we have guidelines we've written for controllers her: https://github.com/MetaMask/core/blob/d158e46244c24e6426ac42e85f991fed34d71700/docs/writing-controllers.md. We also have example controllers you can browse here: https://github.com/MetaMask/core/tree/d158e46244c24e6426ac42e85f991fed34d71700/examples/example-controllers. A lot of my suggestions are based on these guidelines but you might find it helpful to take a look through yourself to make sure you're familiar.
Let me know if you have any questions on comments I've left.
| @@ -0,0 +1,70 @@ | |||
| { | |||
| "name": "@metamask/multichain-network-controller", | |||
| "version": "0.0.1", | |||
There was a problem hiding this comment.
Let's change this back to 0.0.0. We will bump this to the correct version when we make a new release. Otherwise, this package could get released accidentally:
| "version": "0.0.1", | |
| "version": "0.0.0", |
| "devDependencies": { | ||
| "@metamask/accounts-controller": "^21.0.1", | ||
| "@metamask/auto-changelog": "^3.4.4", | ||
| "@metamask/network-controller": "^22.1.1", |
There was a problem hiding this comment.
It looks like this controller talks to NetworkController. Can we add this as a peer dependency as well to tell clients that they need to be using the right version?
packages/multichain-network-controller/src/MultichainNetworkController.ts
Show resolved
Hide resolved
| blockExplorerUrls: string[]; | ||
| defaultBlockExplorerUrlIndex?: number; | ||
| lastUpdated?: number; | ||
| isEvm?: false; |
There was a problem hiding this comment.
Is there a reason why this property is optional? Could it be this instead?
| isEvm?: false; | |
| isEvm: boolean; |
| string, | ||
| MultichainNetworkConfiguration | ||
| >; | ||
| selectedMultichainNetworkChainId: string; |
There was a problem hiding this comment.
Should this also be a CAIP-19 type?
There was a problem hiding this comment.
Should also be CaipChainId here IMO yes (CAIP-2) from @metamask/utils
| let messenger: ControllerMessenger<AllowedActions, AllowedEvents>; | ||
|
|
||
| beforeEach(() => { | ||
| messenger = buildMessenger(); |
There was a problem hiding this comment.
Rather than setting up tests using beforeEach — which can make things complicated over time — what are your thoughts on using a helper method to set up the controller in each test? Unfortunately we don't have a great example of this in core, but NftController has one called setupController that works pretty well:
In your case I'm imagining something like:
/**
* Constructs a new global messenger for use in tests.
*
* @returns A new instance of ControllerMessenger that supports the actions and
* events MultichainNetworkController needs to access.
*/
function getGlobalMessenger() {
return new ControllerMessenger<
MultichainNetworkControllerActions | AllowedActions,
MultichainNetworkControllerEvents | AllowedEvents
>();
}
/**
* Constructs the messenger which is restricted for
* MultichainNetworkController.
*
* @param globalMessenger - The global messenger to base the messenger from.
* @returns The restricted messenger for MultichainNetworkController.
*/
function getControllerMessenger(
globalMessenger = getGlobalMessenger(),
): MultichainNetworkControllerMessenger {
return globalMessenger.getRestricted({
name: 'MultichainNetworkController',
allowedActions: ['NetworkController:setActiveNetwork'],
allowedEvents: [],
});
}
/**
* Constructs the MultichainNetworkController and the global messenger for use
* in tests.
*
* @param args - Arguments to this function.
* @param args.options - Controller options.
* @param args.mockSetActiveNetwork - The handler to use for the
* `NetworkController:setActiveNetwork` action.
* @returns The MultichainNetworkController and the global messenger.
*/
function setupController({
options,
mockSetActiveNetwork = jest.fn(async () => {
// do nothing
}),
}: {
options?: Partial<
ConstructorParameters<typeof MultichainNetworkController>[0]
>;
mockSetActiveNetwork?: NetworkControllerSetActiveNetworkAction['handler'];
}) {
const messenger = getGlobalMessenger();
messenger.registerActionHandler(
'NetworkController:setActiveNetwork',
mockSetActiveNetwork,
);
const controllerMessenger = getControllerMessenger(messenger);
const controller = new MultichainNetworkController({
...options,
messenger: controllerMessenger,
});
return { controller, messenger };
}Then you don't need to spy on the messenger, you can just pass in a function or mock function that lets you override the setActiveNetwork call.
| }, | ||
| "references": [ | ||
| { "path": "../base-controller" }, | ||
| { "path": "../accounts-controller" }, |
There was a problem hiding this comment.
I would recommend doing so, yes, and adding it in a future PR.
| export const bitcoinCaip2ChainId = 'bip122:000000000019d6689c085ae165831e93'; | ||
| export const solanaCaip2ChainId = 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'; | ||
|
|
||
| export const multichainNetworkConfigurations: Record<string, MultichainNetworkConfiguration> = { |
There was a problem hiding this comment.
Where are these constants used?
There was a problem hiding this comment.
I think they were added here to import in the consumer (mobile/extension) and when we initialise the controller we can have this source of truth of data for non evm networks!
Do you think since it's not directly used in the multichain network controller doesn't make sense?
There was a problem hiding this comment.
Ah. For NetworkController we prepopulate the state with network configurations for all known supported Infura networks, so that a consumer can access information for each network without having to explicitly add it:
Does it make sense to do a similar thing for the MultichainNetworkController? I know things work differently here so I'm not sure.
| "@metamask/utils": "^11.0.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@metamask/accounts-controller": "^21.0.1", |
There was a problem hiding this comment.
Similarly to your question in tsconfig.json, do we need this dependency yet?
| /** | ||
| * Here we ensure that TypeScript resolves `@metamask/*` imports to the | ||
| * uncompiled source code for packages that live in this repo. | ||
| * | ||
| * NOTE: This must be synchronized with the `moduleNameMapper` option in | ||
| * `jest.config.packages.js`. | ||
| * | ||
| * NOTE 2: This is not necessary when copying this package to `packages/`. | ||
| */ | ||
| "paths": { | ||
| "@metamask/*": ["../../packages/*/src", "../*/src"] | ||
| } |
There was a problem hiding this comment.
Agreed, we should not need these changes as they are provided by tsconfig.packages.json.
|
A couple more things:
|
| AllowedActions, | ||
| AllowedEvents, |
There was a problem hiding this comment.
It is okay to export these from the file, but not from the package itself:
| AllowedActions, | |
| AllowedEvents, |
See here for more: https://github.com/MetaMask/core/blob/main/docs/writing-controllers.md#define-but-do-not-export-a-type-union-for-external-action-types
| }; | ||
|
|
||
| export type MultichainNetworkConfiguration = { | ||
| chainId: string; // Should be Caip2 type |
There was a problem hiding this comment.
We should use CaipChainId from @metamask/utils
| export type MultichainNetworkConfiguration = { | ||
| chainId: string; // Should be Caip2 type | ||
| name: string; | ||
| nativeCurrency: string; // Should be Caip19 type |
There was a problem hiding this comment.
We should use CaipAssetType from @metamask/utils
| string, | ||
| MultichainNetworkConfiguration | ||
| >; | ||
| selectedMultichainNetworkChainId: string; |
There was a problem hiding this comment.
Should also be CaipChainId here IMO yes (CAIP-2) from @metamask/utils
| >; | ||
| selectedMultichainNetworkChainId: string; | ||
| multichainNetworksMetadata: Record<string, MultichainNetworkMetadata>; | ||
| nonEvmSelected: boolean; |
There was a problem hiding this comment.
Also, instead of "storing" a state like this, we could compute differently maybe.
Unless we really need this to be in the state?
If we assume that selectedMultichainNetworkChainId will also be set to a CAIP-2 chain ID for EVM, we could compute it like:
isNonEvmSelectedNetwork(): boolean {
const { namespace } = parseCaipChainId(this.state.selectedMultichainNetworkChainId);
// 'eip155' is the CAIP-2 namespace for EVMs.
return namespace !== KnownCaipNamespace.Eip155;
}IMO, that's "safer" to compute it dynamically to avoid any de-sync logic between the 2, WDYT?
Note
This is true only if selectedMultichainNetworkChainId also references EVM chain IDs.
| }); | ||
| } | ||
|
|
||
| async setActiveNetwork(clientId: string, chainId?: string): Promise<void> { |
There was a problem hiding this comment.
Should also be a CaipChainId rather than a string here.
I include a suggestion, but you will need to add the associated import { CaipChainId } from '@metamask/utils' yourself 😅
| async setActiveNetwork(clientId: string, chainId?: string): Promise<void> { | |
| async setActiveNetwork(clientId: string, chainId?: CaipChainId): Promise<void> { |
| export const bitcoinCaip2ChainId = 'bip122:000000000019d6689c085ae165831e93'; | ||
| export const solanaCaip2ChainId = 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'; |
There was a problem hiding this comment.
Those are now exported by the keyring-api, but not sure we want to have an extra deps to this package here?
We keep repeating those constants here and there, so for now we could just depend on it until we move them somewhere else if need be, see:
|
|
||
| export const multichainNetworkConfigurations: Record<string, MultichainNetworkConfiguration> = { | ||
| bitcoinCaip2ChainId: { | ||
| chainId: bitcoinCaip2ChainId, |
There was a problem hiding this comment.
Could be a BtcScope for now if we decide to use the keyring-api but again... IDK about depending on this package in this context 😅
| isEvm: false, | ||
| }, | ||
| solanaCaip2ChainId: { | ||
| chainId: solanaCaip2ChainId, |
There was a problem hiding this comment.
Similar comment, but with SolScope
…script documentation, and solve most of the review comments
Implementation of the new Multichain network controller, this is the first version of the multichain network controller and it will handle the source of truth for non evm networks.
Multichain network controller includes a state variable that helps the consumer knows which networks should be interacting with (evm or non-evm).
Multichain network controller will work as a proxy with the existing Network Controller, which currently handles the evm networks source of truth.
References
Related to MetaMask/metamask-mobile#13234
Changelog
@metamask/multichain-network-controllerChecklist