refactor(multichain-account-service): Improved performance across package classes and improved error messages #6654

hmalik88 · 2025-09-18T18:24:55Z

Explanation

`MultichainAccountService`

Unified wallet creation into one flow (createMultichainAccountWallet) that handles import/restore/new vault.
Builds a single ServiceState index in one pass and passes state slices to wallets/groups (cuts repeated controller scans/calls).
Simplified init path and removed dead accountIdToContext mapping.

`MultichainAccountWallet`

init now consumes a pre-sliced wallet state (entropySource → groups → providerName → ids) instead of querying providers.
Emits clear events on group creation/updates; alignment orchestration uses provider state instead of full scans.

`MultichainAccountGroup`

init registers account IDs per provider and fills reverse maps; calls provider.addAccounts(ids) to keep providers in sync.
Added getAccountIds() for direct access to underlying IDs.
Improved partial‑failure reporting (aggregates provider errors by name).

`BaseBip44AccountProvider`

Added addAccounts(ids: string[]), enabling providers to track their own account ID lists.
getAccounts() paths rely on known IDs (plural lookups) rather than scanning the full controller list.

`EvmAccountProvider`

Switched from address‑based scans to ID‑based fetches (getAccount(s)) for create/discover (removes $O(n)$ scans).

Performance Analysis

n = total BIP-44 accounts in the AccountsController
p = number of providers (currently 4)
w = number of wallets (entropy sources)
g = total number of groups
e = number of created EVM accounts

When fully aligned $g = n / p$.
When accounts are not fully aligned then $g = max(f(p))$, where $f(p)$ is the number of accounts associated with a provider.

Consider two scenarios:

State 1 -> State 2 transition, the user has unaligned groups after the transition.
Already transitioned to State 2, the service is initialized after onboarding and every time the client is unlocked.

General formulas

For Scenario 2, the formulas are as follows:

Before this refactor, the number of loops can be represented $n * p * (1 + w + g)$, which with $p = 4$, becomes $n^2 + 4n(1 + w)$.

Before this refactor, the number of controller calls can be represented as $1 + w + g$, which with $p = 4$, becomes $1 + w + n/4$.

After this refactor, the number of loops can be represented by $n * p$, which with $p = 4$, becomes $4n$.

After this refactor, the number of calls is just $1$.

For Scenario 1, the formulas are entirely dependent on the breakdown of the number of accounts each provider has amongst the $n$ accounts, let's consider a scenario where Solana has $n/2$, Ethereum has $n/8$, Bitcoin has $n/4$ and Tron has $n/8$, the formulas would be as follows:

Before this refactor, the number of loops in the alignment process can be represented as $(p * g) + (n * e)$, which with $p=4$ and $g = n/2$, becomes $2n + 3n^2/8$. Therefore the number of loops for initialization + alignment in this scenario with $p = 4$ and $g = n/2$, becomes $(19/8)n^2 + (4w + 6)n$.

Before this refactor, the number of controller calls in the alignment process can be represented as $e$, which becomes $3n/8$. Therefore the number of controller calls for initialization + alignment in this scenario with $p = 4$, becomes $1 + w + 5n/8$.

After this refactor, the number of loops in the alignment process can be represented as $p * g$, which becomes $2n$. Therefore, the number of loops for initialization + alignment in this scenario with $p = 4$ and $g = n/2$, becomes $6n$.

After this refactor, the number of controller calls in the alignment process can be represented as $e$ which becomes $3n/8$. Therefore, the number of controller calls for initialization + alignment in this scenario with $p = 4$ and $g = n/2$, becomes $1 + 3n/8$.

In short, previous init performance for loops and controller calls was quadratic and linear, respectively. After, it is linear and constant.

Performance Charts

Below are charts that show performance (loops and controller calls) $n = 0$ -> $n = 256$ for Scenario 1 and 2 with $w = 2$, respectively:

References

N/A

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 communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Note

Refactors MultichainAccountService to build/init wallets/groups from a single indexed state and updates providers/alignment/discovery, while adding AccountsController:getAccounts and KeyringController create/restore vault actions.

Multichain Account Service:
- State-driven init/update: Build a ServiceState once from AccountsController:listMultichainAccounts; MultichainAccountWallet/MultichainAccountGroup now init/update from slices.
- Group/Wallet behavior: Create/update groups without full scans; improved partial-failure handling (aggregate provider errors); alignment skips disabled providers and cleans their accounts; discovery assembles groups before align.
- Providers:
  - BaseBip44AccountProvider: Track account IDs (addAccounts, removeAccountsFromList); fetch via AccountsController:getAccounts instead of scanning.
  - EvmAccountProvider: Use deterministic ID + AccountsController:getAccount; keep discovery/create logic; ID-based lookups.
  - AccountProviderWrapper: add isDisabled/setEnabled behavior.
- API/types: Remove reverse mapping and account event syncing; update messenger allowed actions/events.
Accounts Controller:
- Add getAccounts(accountIds[]) method and action; tests and exports.
Keyring Controller:
- Add actions: createNewVaultAndKeychain, createNewVaultAndRestore; register handlers; extend action union.
Changelogs/Tests: Update changelogs and expand tests to cover new flows, error aggregation, and disabled-provider behavior.

^{Written by Cursor Bugbot for commit 99d70dd. This will update automatically on new commits. Configure here.}

…to wallets and groups

…tead of getAccountByAddress which iterates through the whole of internal accounts in the AccountsController

…vx-improvements

hmalik88 · 2025-09-18T18:36:27Z

packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts

+      accountsList,
+    );
+    // we cast here because we know that the accounts are BIP-44 compatible
+    return internalAccounts as Bip44Account<KeyringAccount>[];


Although the getAccounts's return type is (InternalAccount | undefined)[], we're sure to get back all the accounts we want since the accounts list will never be stale.

…accountAdded and accountRemoved handling, it is dead code

…vx-improvements

hmalik88 · 2025-09-19T16:10:08Z

packages/multichain-account-service/src/MultichainAccountService.ts

    MultichainAccountWallet<Bip44Account<KeyringAccount>>
  >;

-  readonly #accountIdToContext: Map<


Decided to get rid of this mapping since it was only being used for handling the accountRemoved and accountAdded events, removing this gets rid of a large loop in the init function as well. If there's a particular need for this data at the client level, we can always add this back in.

…handle createNewVaultAndKeychain and createNewVaultAndRestore code paths

…oints

…s, remove redundant state assignment, use assert to ensure wallet existence after creation

…ble with new changes

…ned promise

ccharly · 2025-09-29T07:09:47Z

packages/multichain-account-service/src/MultichainAccountGroup.ts

+        // Add the accounts to the provider's internal list of account IDs
+        provider.addAccounts(accountIds);


See my other comment about:

Given the providers are the sources of accounts...

ccharly · 2025-09-29T07:11:36Z

packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts

+   *
+   * @param accounts - The accounts to add.
+   */
+  addAccounts(accounts: Bip44Account<KeyringAccount>['id'][]): void {


Given the providers are the sources of accounts, I don't think they should have an addAccounts method? 🤔

They should be able to hold a list of known accounts once they are initialized? And since we're sharing the same providers instances across the service/wallets/groups, this initialization should happen only once. Further updates to the internal account list happens when a createAccounts is called on that providers.

WDYT?

This is to more so maintain a global list of the ids and reduce the number of calls/work we would have to make to other controllers. I wanted it this way because we were just grabbing the entire list of accounts from the AccountsController every time we called the getAccount(s) in the group and provider classes, so maintaining a global list that the provider can fetch from the new AccountsController:getAccounts method made sense to me.

Well, I think that makes sense that providers keep an internal list of account IDs yes. But the consumers (wallet/group/service here) should have no control over that IMO.

Like, when we call createAccounts we know we have to update this internal list with new accounts. Same goes for removeAccounts, if a provider is getting disabled, then it should automatically update its internal list IMO, it's not the consumer role to make sure this list is "consistent".

However, consumers can still react to those actions/events and update their own mapping (basically the #accountToProviders maps here).

That's why I wanted to avoid to have some state in the providers initially, mainly to not have those mutable methods. But I still think that your internal state makes sense and align with the existing interface, so we should be able to make this work still, no?

Hmm that's a fair assessment, I suppose I can move the addAccounts call to createAccounts and have the AccountProviderWrapper clear it's internal list if the provider is disabled when createAccounts is called in alignment?

packages/multichain-account-service/src/MultichainAccountService.ts

ccharly · 2025-09-29T07:22:45Z

packages/multichain-account-service/src/MultichainAccountService.ts

+      const wallet = new MultichainAccountWallet({
+        entropySource,
        providers: this.#providers,
        messenger: this.#messenger,
      });
+      wallet.init(serviceState[entropySource]);


I wonder if we could omit the init part?

I really like that idea of "passing" a in-memory-state to the wallet/groups, because indeed, they are just "wrapper" around methods for their own domain (wallet domain or group domain).

Having the service being the owner of the entire memory layout and passing "views" on each "wallet/group state" would make sense to me.

And I think we could have a similar pattern on the providers too, so the providers can update the state directly, and since every components would share the same "data views", they would get updated automatically too.

Just need to double-check for concurrent accesses if we start sharing the same spaces though 😅

Hmm I'd prefer to keep this pattern, it feels less complicated and more explicit about the intended action to the wallet/group state. I think we would have to end up changing the provider interface as well and would essentially be moving computation here to the provider. We're still reacting to the provider's actions by initing with additional state at the points of creation.

Alright, then WDYT about adding it in the constructor's params? So we are "forced" to pass the initial state of the wallet?

This way, we can also rename init to setState maybe, and re-use it in other contexts (cause I think init makes sense when we init right after constructing an object, but it does sounds "bad" when we just re-use it to "reset" the internal state, so reset or setState could be more appropriate for those).

WDYT?

Yeah, I can name it something else.

Ok now I remember why I was against the idea of passing it through the constructor, not every wallet will have an initial state i.e. new wallets that haven't had any discovery ran on it. I will however add another method and call it updateState instead of calling init (it'll be just the init call under the hood anyway, but for clarity purpose we can call it something else).

…vx-improvements

ccharly · 2025-10-08T09:46:00Z

packages/accounts-controller/src/AccountsController.test.ts

+      const result = accountsController.getAccounts([mockAccount.id]);
+
+      expect(result).toStrictEqual([mockAccount]);


Maybe use multiple accounts instead?

Suggested change

const result = accountsController.getAccounts([mockAccount.id]);

expect(result).toStrictEqual([mockAccount]);

const result = accountsController.getAccounts([mockAccount.id. mockAccount2.id]);

expect(result).toStrictEqual([mockAccount, mockAccount2]);

ccharly · 2025-10-08T09:47:43Z

packages/multichain-account-service/src/MultichainAccountGroup.ts

  readonly #groupIndex: number;

-  readonly #providers: NamedAccountProvider<Account>[];
+  readonly #providers: BaseBip44AccountProvider[];


IMO we should re-introduce the generic parameter for those providers. It was meant to be "extensible" and would align it with the Account type parameter being used by the top-level class here

There was a good reason I had to change this, the types were not compatible for some reason which I'm forgetting why honestly (it's been a while this PR has been open 😅). I'll go back and change and see if it's compatible if not, I'll report back here.

ccharly · 2025-10-08T09:48:59Z

packages/multichain-account-service/src/MultichainAccountGroup.ts

-    return provider.getAccount(id);
+
+    // We cast here because TS cannot infer the type of the account from the provider
+    return provider.getAccount(id) as Account;


We need the cast because the providers are not using the Account type parameter. This cast should be gone if we re-add it I guess

ccharly · 2025-10-08T09:51:55Z

packages/multichain-account-service/src/MultichainAccountGroup.ts

          return created;
        }
-        return Promise.resolve();
+        return Promise.reject(new Error('Already aligned'));


I would not use an error message in that case. Maybe we can re-use what you did in the EvmAccountProvider.#createAccount last time, like returning a boolean to indicate if we created accounts or not?

Like const [didCreate, accounts] = .... WDYT?

Do you mean changing the provider's createAccounts return type or returning the tuple in the promise in Promise.allSettled? I found this method easier because then we're returning a tuple just for this one use case.

packages/multichain-account-service/src/MultichainAccountGroup.ts

packages/multichain-account-service/src/MultichainAccountService.ts

ccharly · 2025-10-14T08:00:23Z

packages/multichain-account-service/src/MultichainAccountService.ts

+        return {
+          entropySource: account.options.entropy.id,
+          groupIndex: account.options.entropy.groupIndex,
+          providerName: provider.getName(),


I start to think we should introduce a .id for providers too 🤔 Name is sort of unique today, but I feel like it's more appropriate for logging/debugging.

On the other hand id fits more in our patterns, and given that we now use it a lot to "map" things, it sounds better to me. WDYT?

I don't know if it's worth the effort to do that but I'm not completely opposed to it, a provider's name would be unique otherwise it wouldn't be serving it's purpose haha. Are you proposing assigning some uuid in the provider's constructor?

packages/multichain-account-service/src/MultichainAccountService.ts

ccharly · 2025-10-14T08:03:51Z

packages/multichain-account-service/src/MultichainAccountService.ts

+      const wallet = new MultichainAccountWallet({
+        entropySource,
        providers: this.#providers,
        messenger: this.#messenger,
      });
+      wallet.init(serviceState[entropySource]);


Alright, then WDYT about adding it in the constructor's params? So we are "forced" to pass the initial state of the wallet?

This way, we can also rename init to setState maybe, and re-use it in other contexts (cause I think init makes sense when we init right after constructing an object, but it does sounds "bad" when we just re-use it to "reset" the internal state, so reset or setState could be more appropriate for those).

WDYT?

ccharly · 2025-10-14T08:42:40Z

packages/keyring-controller/CHANGELOG.md


+### Added
+
+- Add actions for `createNewVaultAndKeychain` and `createNewVaultAndRestore` ([#6654](https://github.com/MetaMask/core/pull/6708))


Suggested change

- Add actions for `createNewVaultAndKeychain` and `createNewVaultAndRestore` ([#6654](https://github.com/MetaMask/core/pull/6708))

- Add actions for `createNewVaultAndKeychain` and `createNewVaultAndRestore` ([#6654](https://github.com/MetaMask/core/pull/6654))

packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts

ccharly · 2025-10-14T08:47:57Z

packages/accounts-controller/src/AccountsController.ts

We should probably also split this from this PR. For similar reasons (see my other comment about packages/keyring-controller/src/KeyringController.ts file)

packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts

ccharly · 2025-10-14T08:50:56Z

packages/multichain-account-service/src/MultichainAccountWallet.ts

              entropySource: this.#entropySource,
              groupIndex: targetGroupIndex,
-            });
+            })) as Account[];


We also need that cast because our provider type does not use the generic type of this class. So IMO, we should re-introduce it.

Nodonisko · 2025-10-17T11:18:54Z

packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts

+   */
  getAccounts(): Bip44Account<KeyringAccount>[] {
-    return this.#getAccounts();
+    const accountsList = this.#getAccountsList();


Make sure this method is fast enough, it's called very often and previously it was quite slow. You can do something like this to check it:

getAccounts(): Bip44Account<KeyringAccount>[] { const perf = performance.now() // ... method code console.log("getAccounts perf", performance.now() - perf) return internalAccounts }

Yeah, previously this method would iterate through the whole list of accounts but now I've added a method to the accounts controller to just do constant time lookup for each account id. With the refactor we also aren't relying on this method anymore for state construction either so this will no longer be an issue.

…vx-improvements

cursor · 2025-10-17T22:43:57Z

packages/multichain-account-service/src/MultichainAccountService.ts


-    let sync = true;
+    return serviceState;
+  }


Bug: Service State Initialization Fails for Non-HD Keyrings

The #constructServiceState method initializes serviceState only for HD keyrings. If an account's entropy source doesn't correspond to an HD keyring, subsequent attempts to access serviceState[entropySource][groupIndex] will result in a "Cannot read property 'groupIndex' of undefined" error.

hmalik88 added 13 commits September 14, 2025 12:50

feat: have group align method print all provider errors

aa318fe

Merge branch 'main' into hm/bip-44-perf-and-devx-improvements

17e855f

Merge branch 'main' into hm/bip-44-perf-and-devx-improvements

924a679

Merge branch 'main' into hm/bip-44-perf-and-devx-improvements

be8b2f7

feat: construct service state at the top level and pass state slices …

b49f7b4

…to wallets and groups

Merge branch 'main' into hm/bip-44-perf-and-devx-improvements

a261e1e

chore: readd code removed by mistake

6104131

feat: add getAccounts method to AccountsController

c45f612

chore: update types to include getAccounts action

acf36e0

refactor: derive account ID and use that to do a getAccounts call ins…

ce3cef4

…tead of getAccountByAddress which iterates through the whole of internal accounts in the AccountsController

feat: finish refactor

891fa50

Merge remote-tracking branch 'origin/main' into hm/bip-44-perf-and-de…

83060d6

…vx-improvements

chore: add JSDoc for wallet init

01096bf

hmalik88 commented Sep 18, 2025

View reviewed changes

hmalik88 added 2 commits September 19, 2025 12:08

chore: remove accountId to context mapping since with the removal of …

4c2e582

…accountAdded and accountRemoved handling, it is dead code

Merge remote-tracking branch 'origin/main' into hm/bip-44-perf-and-de…

43972f8

…vx-improvements

hmalik88 commented Sep 19, 2025

View reviewed changes

hmalik88 added 13 commits September 22, 2025 11:01

feat: start to add logic that will let createMultichainAccountWallet …

7cec854

…handle createNewVaultAndKeychain and createNewVaultAndRestore code paths

feat: remove need for mnemonicAsBytes

f05e6da

Merge branch 'main' into hm/bip-44-perf-and-devx-improvements

e325c77

feat: update creatMultichainAccountWallet method to cover all entry p…

6c5b86e

…oints

chore: update JSDoc

7d805ef

chore: update log messages

6b6e776

chore: add more JSDocs

33e5e34

test: update multichain service tests

f4d79c0

fix: address type errors, remove try/catch block to not swallow error…

350aa93

…s, remove redundant state assignment, use assert to ensure wallet existence after creation

fix: lint fixes

ce322e0

chore: remove commented test

31a6d71

refactor: make changes to messenger and providers to make the compati…

4c485a6

…ble with new changes

refactor: simplify init and properly filter for rejected already alig…

c82de17

…ned promise

test: have account group creation test check actual failure message

4b19ed5

hmalik88 changed the title ~~refactor: MultichainAccountService, MultichainAccountWallet, MultichainAccountGroup performance and DevX improvements~~ refactor(multichain-account-service): Improved performance across package classes and improved error messages Sep 28, 2025

hmalik88 marked this pull request as ready for review September 28, 2025 02:29

hmalik88 requested review from a team as code owners September 28, 2025 02:29

This comment was marked as outdated.

Sign in to view

ccharly reviewed Sep 29, 2025

View reviewed changes

hmalik88 added 2 commits October 9, 2025 00:17

Merge remote-tracking branch 'origin/main' into hm/bip-44-perf-and-de…

5bcaa53

…vx-improvements

test: update tests

c12435a

This comment was marked as outdated.

Sign in to view

hmalik88 added 4 commits October 9, 2025 01:41

chore: remove old comment

e9ab58d

chore: update changelog

521a71b

Merge remote-tracking branch 'origin/main' into hm/bip-44-perf-and-de…

1162348

…vx-improvements

feat: add code to remove disabled provider state from groups

31f0193

This comment was marked as outdated.

Sign in to view

hmalik88 added 4 commits October 13, 2025 15:30

test: add tests for disabled provider

7128b55

fix: update param passed to clean disabled provider state function

0c9324c

fix: update logic and tests

6b15c2d

Merge remote-tracking branch 'origin/main' into hm/bip-44-perf-and-de…

0ec9181

…vx-improvements

This comment was marked as outdated.

Sign in to view

fix: lint fix

98db5d3

This comment was marked as outdated.

Sign in to view

refactor: rename function and add JSDoc

98d6373

ccharly reviewed Oct 14, 2025

View reviewed changes

Nodonisko reviewed Oct 17, 2025

View reviewed changes

hmalik88 added 3 commits October 17, 2025 08:26

Merge remote-tracking branch 'origin/main' into hm/bip-44-perf-and-de…

5733fac

…vx-improvements

Merge remote-tracking branch 'origin/main' into hm/bip-44-perf-and-de…

eab65ca

…vx-improvements

chore: apply code review

99d70dd

cursor bot reviewed Oct 17, 2025

View reviewed changes

		// Add the accounts to the provider's internal list of account IDs
		provider.addAccounts(accountIds);

		const result = accountsController.getAccounts([mockAccount.id]);

		expect(result).toStrictEqual([mockAccount]);


		### Added

		- Add actions for `createNewVaultAndKeychain` and `createNewVaultAndRestore` ([#6654](https://github.com/MetaMask/core/pull/6708))

Uh oh!

refactor(multichain-account-service): Improved performance across package classes and improved error messages #6654

Are you sure you want to change the base?

refactor(multichain-account-service): Improved performance across package classes and improved error messages #6654

Conversation

hmalik88 commented Sep 18, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

MultichainAccountService

MultichainAccountWallet

MultichainAccountGroup

BaseBip44AccountProvider

EvmAccountProvider

Performance Analysis

Performance Charts

References

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

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!

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!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

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!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

hmalik88 commented Sep 18, 2025 •

edited by cursor bot

Loading

`MultichainAccountService`

`MultichainAccountWallet`

`MultichainAccountGroup`

`BaseBip44AccountProvider`

`EvmAccountProvider`