refactor(multichain-account-service): Improved performance across package classes and improved error messages #6654
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to wallets and groups
…tead of getAccountByAddress which iterates through the whole of internal accounts in the AccountsController
hmalik88
commented
Sep 18, 2025
| accountsList, | ||
| ); | ||
| // we cast here because we know that the accounts are BIP-44 compatible | ||
| return internalAccounts as Bip44Account<KeyringAccount>[]; |
Contributor
Author
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
hmalik88
commented
Sep 19, 2025
| MultichainAccountWallet<Bip44Account<KeyringAccount>> | ||
| >; | ||
|
|
||
| readonly #accountIdToContext: Map< |
Contributor
Author
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}
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)
Contributor
Author
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
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Major perf-focused refactor that replaces scan-based sync with a single state index and pushes sliced state to wallets/groups.
ServiceStateonce, initializes providers withinit(ids), and passes state toMultichainAccountWallet.init/MultichainAccountGroup.initcreateMultichainAccountWallet(supportsimport,create,restore) and addsremoveMultichainAccountWalletMultichainAccountGroupaddsgetAccountIds, rewritesalignAccounts(handles disabled providers; aggregates failures), and updates internal maps viaupdateinit(ids)andalignAccounts, maintain their own ID sets;AccountProviderWrappergates ops when disabledEvmAccountProviderswitches to ID-based lookups (getAccount(s)) and deterministic ID derivation viagetUUIDFromAddressOfNormalAccountWritten by Cursor Bugbot for commit a8c05bb. This will update automatically on new commits. Configure here.