feat: add `multichain-network-controller` package by tommasini · Pull Request #5209 · MetaMask/core

tommasini · 2025-01-29T00:23:54Z

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
Fixes https://github.com/MetaMask/accounts-planning/issues/804

Changelog

`@metamask/multichain-network-controller`

: Proxy to NetworkController set current evm active network
: State variable nonEvmSelected that tells consumers which evm should they show (evm or non-evm)
: Setter to change nonEvmSelected variable to true
: Setter to change nonEvmSelected variable to false

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

… to update nonEvmNetwork state variable to false; added unit tests for setActiveNetwork and both new setters

tommasini · 2025-01-29T18:08:17Z

packages/multichain-network-controller/LICENSE

@@ -0,0 +1,20 @@
+MIT License
+
+Copyright (c) 2024 MetaMask


Should this be 2025?

tommasini · 2025-01-29T18:12:57Z

packages/multichain-network-controller/tsconfig.json

+  /**
+   * 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"]
+  }


Suggested change

/**

* 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"]

}

Agreed, we should not need these changes as they are provided by tsconfig.packages.json.

tommasini · 2025-01-29T18:14:51Z

packages/multichain-network-controller/tsconfig.json

+  },
+  "references": [
+    { "path": "../base-controller" },
+    { "path": "../accounts-controller" },


Should we remove this for now, since it's not interacting with the accounts controller yet?

I would recommend doing so, yes, and adding it in a future PR.

mcmire

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.

mcmire · 2025-01-29T18:50:26Z

packages/multichain-network-controller/package.json

@@ -0,0 +1,70 @@
+{
+  "name": "@metamask/multichain-network-controller",
+  "version": "0.0.1",


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:

Suggested change

"version": "0.0.1",

"version": "0.0.0",

mcmire · 2025-01-29T18:51:32Z

packages/multichain-network-controller/package.json

+  "devDependencies": {
+    "@metamask/accounts-controller": "^21.0.1",
+    "@metamask/auto-changelog": "^3.4.4",
+    "@metamask/network-controller": "^22.1.1",


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

mcmire · 2025-01-29T18:53:39Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+  blockExplorerUrls: string[];
+  defaultBlockExplorerUrlIndex?: number;
+  lastUpdated?: number;
+  isEvm?: false;


Is there a reason why this property is optional? Could it be this instead?

Suggested change

isEvm?: false;

isEvm: boolean;

mcmire · 2025-01-29T18:54:18Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+    string,
+    MultichainNetworkConfiguration
+  >;
+  selectedMultichainNetworkChainId: string;


Should this also be a CAIP-19 type?

Should also be CaipChainId here IMO yes (CAIP-2) from @metamask/utils

mcmire · 2025-01-29T19:18:11Z

packages/multichain-network-controller/src/MultichainNetworkController.test.ts

+  let messenger: ControllerMessenger<AllowedActions, AllowedEvents>;
+
+  beforeEach(() => {
+    messenger = buildMessenger();


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:

core/packages/assets-controllers/src/NftController.test.ts

Line 154 in d158e46

function setupController({

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.

mcmire · 2025-01-29T19:23:27Z

packages/multichain-network-controller/tsconfig.json

+  },
+  "references": [
+    { "path": "../base-controller" },
+    { "path": "../accounts-controller" },


I would recommend doing so, yes, and adding it in a future PR.

mcmire · 2025-01-29T19:24:07Z

packages/multichain-network-controller/src/constants.ts

+export const bitcoinCaip2ChainId = 'bip122:000000000019d6689c085ae165831e93';
+export const solanaCaip2ChainId = 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp';
+
+export const multichainNetworkConfigurations: Record<string, MultichainNetworkConfiguration> = {


Where are these constants used?

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?

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:

core/packages/network-controller/src/NetworkController.ts

Line 542 in 846253a

function getDefaultNetworkConfigurationsByChainId(): Record<

core/packages/network-controller/src/NetworkController.ts

Line 583 in 846253a

getDefaultNetworkConfigurationsByChainId();

Does it make sense to do a similar thing for the MultichainNetworkController? I know things work differently here so I'm not sure.

mcmire · 2025-01-29T19:24:48Z

packages/multichain-network-controller/package.json

+    "@metamask/utils": "^11.0.1"
+  },
+  "devDependencies": {
+    "@metamask/accounts-controller": "^21.0.1",


Similarly to your question in tsconfig.json, do we need this dependency yet?

mcmire · 2025-01-29T19:25:22Z

packages/multichain-network-controller/tsconfig.json

+  /**
+   * 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"]
+  }


Agreed, we should not need these changes as they are provided by tsconfig.packages.json.

mcmire · 2025-01-29T19:31:35Z

A couple more things:

It looks like there are some lint violations in these new files. Due to some recent changes yarn lint:fix doesn't work as before (and the CI output is not very helpful, sorry). I would advise turning on Prettier in your editor and then re-saving all of the files to see if that gets through a lot of the lint violations. I'll make some suggestions in the PR if I spot anything other than formatting issues.
You might want to add this new package under teams.json and CODEOWNERS. We've had a couple of new controllers added recently — you can see this PR for an example: https://github.com/MetaMask/core/pull/5133/files#diff-3d36a1bf06148bc6ba1ce2ed3d19de32ea708d955fed212c0d27c536f0bd4da7

mcmire · 2025-01-29T19:33:45Z

packages/multichain-network-controller/src/index.ts

+  AllowedActions,
+  AllowedEvents,


It is okay to export these from the file, but not from the package itself:

Suggested change

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

ccharly · 2025-01-30T07:43:56Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+};
+
+export type MultichainNetworkConfiguration = {
+  chainId: string; // Should be Caip2 type


We should use CaipChainId from @metamask/utils

ccharly · 2025-01-30T07:44:14Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+export type MultichainNetworkConfiguration = {
+  chainId: string; // Should be Caip2 type
+  name: string;
+  nativeCurrency: string; // Should be Caip19 type


We should use CaipAssetType from @metamask/utils

ccharly · 2025-01-30T07:45:12Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+    string,
+    MultichainNetworkConfiguration
+  >;
+  selectedMultichainNetworkChainId: string;


Should also be CaipChainId here IMO yes (CAIP-2) from @metamask/utils

ccharly · 2025-01-30T07:50:05Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+  >;
+  selectedMultichainNetworkChainId: string;
+  multichainNetworksMetadata: Record<string, MultichainNetworkMetadata>;
+  nonEvmSelected: boolean;


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.

ccharly · 2025-01-30T07:54:19Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+    });
+  }
+
+  async setActiveNetwork(clientId: string, chainId?: string): Promise<void> {


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 😅

Suggested change

async setActiveNetwork(clientId: string, chainId?: string): Promise<void> {

async setActiveNetwork(clientId: string, chainId?: CaipChainId): Promise<void> {

ccharly · 2025-01-30T07:57:15Z

packages/multichain-network-controller/src/constants.ts

+export const bitcoinCaip2ChainId = 'bip122:000000000019d6689c085ae165831e93';
+export const solanaCaip2ChainId = 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp';


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:

https://github.com/MetaMask/accounts/blob/v24.0.0/packages/keyring-api/src/btc/constants.ts#L7-L11

https://github.com/MetaMask/accounts/blob/v24.0.0/packages/keyring-api/src/sol/constants.ts#L7-L9

ccharly · 2025-01-30T07:58:19Z

packages/multichain-network-controller/src/constants.ts

+
+export const multichainNetworkConfigurations: Record<string, MultichainNetworkConfiguration> = {
+  bitcoinCaip2ChainId: {
+    chainId: bitcoinCaip2ChainId,


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 😅

ccharly · 2025-01-30T07:58:28Z

packages/multichain-network-controller/src/constants.ts

+    isEvm: false,
+  },
+  solanaCaip2ChainId: {
+    chainId: solanaCaip2ChainId,


Similar comment, but with SolScope

…script documentation, and solve most of the review comments

gantunesr · 2025-02-04T00:28:47Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+  multichainNetworkConfigurationsByChainId: { persist: true, anonymous: true },
+  selectedMultichainNetworkChainId: { persist: true, anonymous: true },
+  multichainNetworksMetadata: { persist: true, anonymous: true },
+  nonEvmSelected: { persist: true, anonymous: true },


Why are these being set to anonymous: true?

Stated by @mcmire

anonymous: true means that there isn't any PII and the property is safe to send to Sentry, anonymous: false means there is PII and it should be masked in Sentry.

We have this documented here now for reference: https://github.com/MetaMask/core/blob/main/docs/writing-controllers.md#define-metadata-for-state-properties

gantunesr · 2025-02-04T00:29:21Z

packages/multichain-network-controller/src/MultichainNetworkController.ts

+/**
+ * State used by the {@link MultichainNetworkController} to cache network configurations.
+ */
+export type MultichainNetworkControllerState = {


I need to update these type based on a recent change on the ADR https://github.com/MetaMask/decisions/pull/51

tommasini · 2025-02-04T20:14:29Z

I'm closing this PR, since we branched off this PR in this one and this one have the latest: #5215

gantunesr and others added 6 commits January 24, 2025 09:56

feat add multichain-network-controller package

47c7310

chore: update exports

7b64a8c

fix: build

37dc7c5

chore: update initial state

454c3db

add setter for update nonEvmNetwork state variable to true and setter…

2f4d06c

… to update nonEvmNetwork state variable to false; added unit tests for setActiveNetwork and both new setters

merge main and solve yarn lock file conflicts

4ee7b32

tommasini added the needs-dev-review label Jan 29, 2025

tommasini added 2 commits January 29, 2025 00:44

added blank line between functions

2f8058a

removed unused import and re order imports, to fix lint warnings

6b7b3b6

tommasini commented Jan 29, 2025

View reviewed changes

mcmire reviewed Jan 29, 2025

View reviewed changes

ccharly reviewed Jan 30, 2025

View reviewed changes

tommasini mentioned this pull request Jan 30, 2025

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

Merged

4 tasks

tommasini added 2 commits January 30, 2025 21:46

btc and solana chain id from keyring api, update types and added java…

a89bff4

…script documentation, and solve most of the review comments

replace nativeAsset name varialbe for nativeCurrency name variable

20a1dbc

gantunesr changed the title ~~feat: Add multichain-network-controller package~~ feat: add multichain-network-controller package Feb 3, 2025

gantunesr reviewed Feb 4, 2025

View reviewed changes

tommasini closed this Feb 4, 2025

	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';

Uh oh!

Conversation

tommasini commented Jan 29, 2025 • edited by gantunesr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Changelog

@metamask/multichain-network-controller

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcmire Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tommasini commented Feb 4, 2025

Uh oh!

tommasini commented Jan 29, 2025 •

edited by gantunesr

Loading

`@metamask/multichain-network-controller`

mcmire Jan 30, 2025 •

edited

Loading

mcmire commented Jan 29, 2025 •

edited

Loading

mcmire Jan 29, 2025 •

edited

Loading