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}
| ) 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. |
|
@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
BREAKING – performance refactor
ServiceStateand state-driveninit/updateforService,Wallet, andGroup(no more on-demand scans or reverse context map)createMultichainAccountWalletsupporting import/create/restore; addsremoveMultichainAccountWalletinitwith account IDs, track IDs internally, exposealignAccounts, and fetch viagetAccount(s)by ID (EVM uses deterministic ID)MultichainAccountGroup/Walletconstruct from provided state, addgetAccountIds, and align all providers with aggregated error reporting and disabled-provider handlingsyncflows and account context handlers; cleans up utilsWritten by Cursor Bugbot for commit fd4aea5. This will update automatically on new commits. Configure here.