refactor(multichain-account-service): Improved performance across package classes and improved error messages #6654
Conversation
…to wallets and groups
…tead of getAccountByAddress which iterates through the whole of internal accounts in the AccountsController
| accountsList, | ||
| ); | ||
| // we cast here because we know that the accounts are BIP-44 compatible | ||
| return internalAccounts as Bip44Account<KeyringAccount>[]; |
There was a problem hiding this comment.
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
| MultichainAccountWallet<Bip44Account<KeyringAccount>> | ||
| >; | ||
|
|
||
| readonly #accountIdToContext: Map< |
There was a problem hiding this comment.
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
…s, remove redundant state assignment, use assert to ensure wallet existence after creation
…ble with new changes
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| return state; | ||
| }, {}); | ||
|
|
||
| group.update(groupState); |
There was a problem hiding this comment.
Group state update clears existing provider accounts
High Severity
When #createNonEvmAccounts calls group.update(groupState), the groupState only contains non-EVM provider accounts. However, #setState iterates over ALL providers and calls #clearAccountToProviderState(provider) for each, including the EVM provider. Since the EVM provider isn't in groupState, its entries get cleared from #accountToProvider but remain in #providerToAccounts. This causes inconsistent state where getAccounts() returns EVM accounts but getAccount(evmId), getAccountIds(), and hasAccounts() don't recognize them. The bug affects both the awaitAll: true path (line 271) and background path (line 282).
Additional Locations (2)
| ); | ||
|
|
||
| this.#wallets.delete(wallet.id); | ||
| } |
There was a problem hiding this comment.
Wallet removal only deletes one account, orphans rest
Medium Severity
The removeMultichainAccountWallet method removes only ONE account via KeyringController:removeAccount but deletes the entire wallet reference from #wallets. A multichain wallet typically has multiple accounts across different group indexes and providers. After calling this method, all accounts except the one with the passed address would become orphaned in the KeyringController - they would still exist but no longer be accessible through the MultichainAccountService. The method name suggests removing the entire wallet, but the implementation leaves the underlying keyring and most accounts intact.
There was a problem hiding this comment.
At the point that this would have been used there are no other accounts created, only the one evm account through the createMultichainAccountWallet method.
| ) as HdKeyring[]; | ||
|
|
||
| const mnemonicAsBytes = mnemonicPhraseToBytes(mnemonic); | ||
|
|
There was a problem hiding this comment.
Wallet remains uninitialized after creation causing missing events
Medium Severity
When createMultichainAccountWallet creates a new wallet (via any of the three flows: import, create, or restore), wallet.init() is never called. The wallet remains with #initialized = false. However, #withLock in the wallet's finally block always sets #status = 'ready' unconditionally, creating an inconsistent state. This causes multichainAccountGroupCreated events to not be published when createMultichainAccountGroup is called later (line 479 checks #initialized). The service's init() method correctly calls wallet.init(), but the wallet creation flows do not.
Additional Locations (1)
packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts
Show resolved
Hide resolved
| for (const account of accounts) { | ||
| this.accounts.add(account); | ||
| } | ||
| } |
There was a problem hiding this comment.
Provider accounts not cleared during service re-initialization
Medium Severity
The BaseBip44AccountProvider.init() method only adds account IDs to the internal accounts Set without clearing it first. When MultichainAccountService.init() is called, it clears #wallets but then calls provider.init(state) which accumulates new IDs without removing old ones. If the service is re-initialized (e.g., after account changes), the providers retain stale account IDs. Subsequent calls to getAccount() for stale IDs would pass the has() check but then receive undefined from AccountsController:getAccount, causing the returned value to be undefined cast as Account.
Additional Locations (1)
There was a problem hiding this comment.
We currently don't have a concept of deleting accounts, so this is a non-issue at the moment, AFAK. cc: @ccharly
packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts
Show resolved
Hide resolved
| ); | ||
|
|
||
| this.#wallets.delete(wallet.id); | ||
| } |
There was a problem hiding this comment.
Provider accounts Set not cleared on wallet removal
Medium Severity
When removeMultichainAccountWallet is called, the wallet is removed from the service's map and the account is removed via KeyringController:removeAccount, but the provider's internal accounts Set (in BaseBip44AccountProvider) retains the stale account IDs. Since providers are shared across all wallets and init() only adds to the Set without clearing it first, subsequent calls to provider.getAccounts() will attempt to fetch these non-existent account IDs from the AccountsController. The unsafe cast in getAccounts() would hide any undefined values returned for missing accounts, potentially causing downstream issues when code expects valid Account objects.
Additional Locations (1)
There was a problem hiding this comment.
At the point that this method is called, there is no entry created in the service state yet for this account that is being removed. The point is for it to be created in state when discoverAccounts is called, but since it will not be ever called before this method is, that is not an issue.
packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts
Show resolved
Hide resolved
packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts
Show resolved
Hide resolved
…ate for providers existing in the group state passed in
packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts
Show resolved
Hide resolved
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Explanation
MultichainAccountServicecreateMultichainAccountWallet) that handles import/restore/new vault.ServiceStateindex in one pass and passes state slices to wallets/groups (cuts repeated controller scans/calls).initpath and removed deadaccountIdToContextmapping.MultichainAccountWalletinitnow consumes a pre-sliced wallet state (entropySource → groups → providerName → ids) instead of querying providers.MultichainAccountGroupinitregisters account IDs per provider and fills reverse maps; callsprovider.addAccounts(ids)to keep providers in sync.getAccountIds()for direct access to underlying IDs.BaseBip44AccountProvideraddAccounts(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.EvmAccountProvidergetAccount(s)) for create/discover (removesPerformance Analysis
When fully aligned$g = n / p$ .$g = max(f(p))$ , where $f(p)$ is the number of accounts associated with a provider.
When accounts are not fully aligned then
Consider two scenarios:
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
initperformance 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
Note
Overview (performance-focused, BREAKING)
ServiceStateindex inMultichainAccountService.initand pass slices toMultichainAccountWallet.initandMultichainAccountGroup.init/update(removes repeated controller scans and reverse mappings)createMultichainAccountWalletsupportingtype: 'import' | 'create' | 'restore'; addremoveMultichainAccountWalletBaseBip44AccountProvider.initstores IDs;getAccounts/getAccountuseAccountsController:getAccounts/getAccountby ID (no full list scans)EvmAccountProviderderives deterministic IDs viagetUUIDFromAddressOfNormalAccount, uses ID-based fetches; creation/discovery add IDs to provider stateAccountProviderWrapperforwardsinit/alignAccounts, supports enable/disable checks; alignment skips disabled providersGroup/Wallet APIs and behavior
MultichainAccountGroup: newgetAccountIds, state-driveninit/update, aggregates provider alignment errors (per‑provider messages), clears mappings per providerMultichainAccountWallet: state-driveninit, background non‑EVM creation with warnings;discoverAccountsbuilds group state and aligns missing accounts; emits status updatesRemovals/changes (BREAKING)
accountIdToContext,syncmethods, and account-added/removed event syncing; rely on explicit state +init/updateTests/infra
AccountsController:getAccounts/getAccount) and new flows; changelog updated with breaking notes.Written by Cursor Bugbot for commit 6a56850. This will update automatically on new commits. Configure here.