feat: Add `MultichainNetworkController` to handle both EVM and non-EVM network and account switching by Cal-L · Pull Request #5215 · MetaMask/core

Cal-L · 2025-01-30T06:01:54Z

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 MultichainNetworkController:networkDidChange 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

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

packages/accounts-controller/src/AccountsController.ts

tommasini · 2025-01-30T11:34:13Z

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

…script documentation, and solve most of the review comments

…Mask/core into feat/handle-multichain-network-and-account-switching

…ichain-network-and-account-switching

mcmire

This looks a lot better now. I had a couple of comments about the use of barrel exports and also a missing dependency in accounts-controller but other than that the rest are minor and can be deferred to a future PR.

packages/accounts-controller/package.json

packages/multichain-network-controller/jest.config.js

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

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

packages/multichain-network-controller/tsconfig.json

…ichain-network-and-account-switching

Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>

mcmire

This looks good, just one more thing.

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

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

mcmire

This looks good!

mcmire

Linting errors :(

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

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

…ntroller.ts Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

gantunesr

LGTM. We'll follow up this PR with the decoupling of the selected network from the network configurations

ccharly · 2025-02-12T20:27:16Z

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

+    }
+
+    // Handle switching to non-EVM network
+    const nonEvmChainId = getChainIdForNonEvmAddress(accountAddress);


We should use account.scopes to check which chains are supported by the account.

With the new scopes we don't need this kind of runtime logic anymore.

Could you elaborate on how this logic might look with your suggestion? FYI, one of the reasons that we kept accounts controller and multichain network controller away from each other is prevent circular dependencies.

ccharly · 2025-02-12T20:32:48Z

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

+    // Notify listeners that setActiveNetwork was called
+    this.messagingSystem.publish(
+      'MultichainNetworkController:networkDidChange',
+      id,
+    );


IMO this should have been fired after the actually update the network configuration.

If some logic is checking the actual state from this controller (and its counterpart the NetworkController) and react to this event, the state won't be "consistent", no?

ccharly · 2025-02-12T20:33:30Z

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

+    // Notify listeners that setActiveNetwork was called
+    this.messagingSystem.publish(
+      'MultichainNetworkController:networkDidChange',
+      id,
+    );


Why do we notify twice? We just do it in the if right before.

Good catch. It was missing a return statement. Ended up simplifying this logic

ccharly · 2025-02-12T20:36:22Z

packages/accounts-controller/src/AccountsController.ts

+    this.update((currentState) => {
+      currentState.internalAccounts.accounts[accountId].metadata.lastSelected =
+        Date.now();
+      currentState.internalAccounts.selectedAccount = accountId;
+    });
+  };


I assume we're not calling setSelectedAccount here to avoid triggering the "account change" event?

If yes, we should add a comment to explain this, cause this could easily be refactored and since there's no comment, we could make a mistake here.

Also, I think that changing the account without publishing this event might have some side effects (like for other controllers that listen to this change event).

I'm not sure how to avoid infinite loop with those inter-dependent events, but that's something we might wanna re-visit at some point.

Updated with comment. Yeah it's a tricky problem. We're just being extra careful not to subscribe and publish to the same controller in each handler atm. Regarding side effects, if another controller relies on account changes, it should subscribe to the state change event and manage its own logic

ccharly · 2025-02-12T20:38:39Z

packages/accounts-controller/src/AccountsController.ts

+   *
+   * @param id - The EVM client ID or non-EVM chain ID that changed.
+   */
+  readonly #handleMultichainNetworkChange = (


The handler name is not following the same convention as the others, it should be #handleOnMultichainNetworkDidChange

ccharly · 2025-02-12T20:44:42Z

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

+    this.update((state) => {
+      state.selectedMultichainNetworkChainId = nonEvmChainId;
+      state.isEvmSelected = false;
+    });


Same here, we should add a comment to explain why we are not firing any event in this case.

## Explanation CODEOWNER rules have been added for all packages that are currently missing them: * `example-controllers`: These are examples maintained by the wallet framework team. * `mutlichain-network-controller`: This package has been a collaboration between many teams, but for now the owners have been set to the same as the `network-controller` plus the accounts team (who proposed this controller's creation). We can change this further if appropriate. ## References The `multichain-network-controller` package was added here: #5215 ## Changelog N/A ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

gantunesr and others added 13 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

added blank line between functions

2f8058a

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

6b7b3b6

Handle network and account updates when switching networks

8a5c9d0

Register message handlers

f898056

Separate concerns when updating network on multichain network controller

edb6868

Handle switching network when account changes

1c3dc66

Remove unused actions and events

c1050f7

Cal-L requested a review from a team as a code owner January 30, 2025 06:01

tommasini reviewed Jan 30, 2025

View reviewed changes

packages/accounts-controller/src/AccountsController.ts Outdated Show resolved Hide resolved

tommasini and others added 7 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

Merge branch 'feat/multichain-networks-controller' of github.com:Meta…

f159822

…Mask/core into feat/handle-multichain-network-and-account-switching

Remove draft type

dab430b

Fix account change subscription in multichain network controller

6994dbe

Add comment

ebe7e29

replace nativeAsset name varialbe for nativeCurrency name variable

20a1dbc

Merge branch 'feat/multichain-networks-controller' of github.com:Meta…

063e25a

…Mask/core into feat/handle-multichain-network-and-account-switching

gauthierpetetin assigned Cal-L Feb 3, 2025

Cal-L marked this pull request as draft February 3, 2025 18:29

Cal-L added 5 commits February 3, 2025 16:57

git commit -m "Duplicate internal accounts mock"

cf31cc3

Update package deps

429f6ad

Clean up MultichainNetworkController

a1618ab

Update constants

7f1ecd9

Update branch coverage threshold

ba0d1ea

Update codeowners

2892d25

Cal-L requested a review from a team as a code owner February 12, 2025 03:18

Cal-L added 4 commits February 11, 2025 19:20

Merge branch 'main' of github.com:MetaMask/core into feat/handle-mult…

1f61229

…ichain-network-and-account-switching

Merge with main

a0bcef5

Update based off latest main

fdfe886

Run yarn update-readme-content

f36c02e

mcmire reviewed Feb 12, 2025

View reviewed changes

tommasini previously approved these changes Feb 12, 2025

View reviewed changes

Address comments in PR

4623c12

Cal-L dismissed tommasini’s stale review via 4623c12 February 12, 2025 18:03

Cal-L and others added 4 commits February 12, 2025 10:04

Merge branch 'main' of github.com:MetaMask/core into feat/handle-mult…

d3e0088

…ichain-network-and-account-switching

update yarn lock

7e3d301

chore: add util methods to update network configuration (#5315)

75a7f30

Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>

Fix lint error

53bb53d

mcmire reviewed Feb 12, 2025

View reviewed changes

packages/multichain-network-controller/src/index.ts Show resolved Hide resolved

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

50ba21e

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

mcmire previously approved these changes Feb 12, 2025

View reviewed changes

mcmire reviewed Feb 12, 2025

View reviewed changes

packages/multichain-network-controller/src/index.ts Outdated Show resolved Hide resolved

packages/multichain-network-controller/src/MultichainNetworkController.ts Outdated Show resolved Hide resolved

Fix lint issue

517713c

Cal-L dismissed mcmire’s stale review via 517713c February 12, 2025 19:07

Update packages/multichain-network-controller/src/MultichainNetworkCo…

d91df82

…ntroller.ts Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>

tommasini approved these changes Feb 12, 2025

View reviewed changes

gantunesr approved these changes Feb 12, 2025

View reviewed changes

mcmire approved these changes Feb 12, 2025

View reviewed changes

Cal-L merged commit ce12485 into main Feb 12, 2025
130 checks passed

Cal-L deleted the feat/handle-multichain-network-and-account-switching branch February 12, 2025 19:20

ccharly reviewed Feb 12, 2025

View reviewed changes

Cal-L mentioned this pull request Feb 13, 2025

Release 299.0.0 #5318

Merged

4 tasks

Gudahtt mentioned this pull request Feb 17, 2025

chore: Add missing CODEOWNER package rules #5351

Merged

4 tasks

Uh oh!

Conversation

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

Uh oh!

Explanation

References

Changelog

@metamask/accounts-controller

@metamask/multichain-network-controller

Checklist

Uh oh!

Uh oh!

tommasini commented Jan 30, 2025

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gantunesr left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Cal-L commented Jan 30, 2025 •

edited

Loading

`@metamask/accounts-controller`

`@metamask/multichain-network-controller`