core icon indicating copy to clipboard operation
core copied to clipboard

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

Open hmalik88 opened this issue 5 months ago • 6 comments

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:

  1. State 1 -> State 2 transition, the user has unaligned groups after the transition.
  2. 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:

MisalignedLoops MisalignedCalls AlignedLoops AlignedCalls

References

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

[!NOTE] Refactors multichain account service to state-driven initialization, unifies wallet creation (import/create/restore), and shifts providers to ID-based account tracking with improved alignment and error reporting.

  • Core
    • Service: Builds a single ServiceState from AccountsController, initializes wallets/groups from state; removes sync/account-context logic; adds unified createMultichainAccountWallet (supports import/create/restore via KeyringController actions); registers new actions; improves error reporting.
    • Wallet: New init(walletState); creates/updates groups from provider ID slices; group creation awaits EVM, runs others in background; discovery constructs groups then aligns; emits status and group lifecycle events.
    • Group: New state-driven init/update; tracks provider→IDs and reverse map; adds getAccountIds(); alignAccounts() uses provider alignAccounts, handles disabled providers, aggregates warnings, emits updated event.
  • Providers
    • BaseBip44AccountProvider: Tracks account IDs internally (init), getAccounts via AccountsController:getAccounts, getAccount by ID, adds alignAccounts helper.
    • AccountProviderWrapper: Forwards init, adds isDisabled() and disabled-aware alignAccounts/guards.
    • EVM/SOL/BTC/TRX: On create/discover, add IDs to internal set; EVM derives deterministic IDs with getUUIDFromAddressOfNormalAccount and fetches via AccountsController:getAccount.
  • Types/Messenger/Utils
    • Expands allowed actions (e.g., AccountsController:getAccounts, KeyringController:createNewVaultAndKeychain/Restore); removes unused error utils; updates tests accordingly.
  • Changelog
    • Notes BREAKING performance refactor, new methods, and behavior changes.

Written by Cursor Bugbot for commit 0a0db209836ef1abf95e25eda0205a2219fb1bda. This will update automatically on new commits. Configure here.

hmalik88 avatar Sep 18 '25 18:09 hmalik88

Bug: Unsafe Cast in getAccounts() Method

The getAccounts() method performs an unsafe cast on the result from AccountsController:getAccounts. This action can return undefined values, but the method casts the array to Bip44Account<KeyringAccount>[] without filtering them. This can lead to runtime errors, like "Cannot read property 'id' of undefined," when consumers (e.g., getAccount()) access properties on these undefined entries.

Fix in Cursor Fix in Web

cursor[bot] avatar Oct 23 '25 14:10 cursor[bot]

Bug: Duplicate Account IDs in Multichain Provider

The BaseBip44AccountProvider.addAccounts method doesn't prevent duplicate account IDs, allowing MultichainAccountGroup.init or update to add the same IDs multiple times. This results in providers holding duplicate account IDs, which may cause unexpected behavior in methods relying on this internal list.

Additional Locations (1)

Fix in Cursor Fix in Web

cursor[bot] avatar Oct 23 '25 14:10 cursor[bot]

Bug: Async Provider Failure Leaves Wallet Inconsistent

In createMultichainAccountGroup, when not awaiting all providers, the MultichainAccountGroup is created and added to the wallet's map synchronously. However, the EVM provider's account creation and group initialization are asynchronous. If the EVM provider fails, the group is left in an uninitialized or incomplete state in the map, leading to inconsistent wallet state and unexpected behavior for callers.

Fix in Cursor Fix in Web

cursor[bot] avatar Oct 23 '25 16:10 cursor[bot]

Bug: Error Handling Anti-Pattern in alignAccounts

The alignAccounts method uses Promise.reject(new Error('Already aligned')) for control flow, which is an anti-pattern. This relies on fragile string matching to distinguish between actual errors and control flow, risking incorrect failure reporting if the message changes.

Fix in Cursor Fix in Web

cursor[bot] avatar Oct 23 '25 16:10 cursor[bot]

@metamaskbot publish-preview

hmalik88 avatar Nov 25 '25 15:11 hmalik88

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/account-tree-controller": "4.0.0-preview-7334bbce",
  "@metamask-previews/accounts-controller": "35.0.0-preview-7334bbce",
  "@metamask-previews/address-book-controller": "7.0.1-preview-7334bbce",
  "@metamask-previews/analytics-controller": "0.0.0-preview-7334bbce",
  "@metamask-previews/announcement-controller": "8.0.0-preview-7334bbce",
  "@metamask-previews/app-metadata-controller": "2.0.0-preview-7334bbce",
  "@metamask-previews/approval-controller": "8.0.0-preview-7334bbce",
  "@metamask-previews/assets-controllers": "91.0.0-preview-7334bbce",
  "@metamask-previews/base-controller": "9.0.0-preview-7334bbce",
  "@metamask-previews/bridge-controller": "63.0.0-preview-7334bbce",
  "@metamask-previews/bridge-status-controller": "63.0.0-preview-7334bbce",
  "@metamask-previews/build-utils": "3.0.4-preview-7334bbce",
  "@metamask-previews/chain-agnostic-permission": "1.2.2-preview-7334bbce",
  "@metamask-previews/claims-controller": "0.2.0-preview-7334bbce",
  "@metamask-previews/composable-controller": "12.0.0-preview-7334bbce",
  "@metamask-previews/controller-utils": "11.16.0-preview-7334bbce",
  "@metamask-previews/core-backend": "5.0.0-preview-7334bbce",
  "@metamask-previews/delegation-controller": "2.0.0-preview-7334bbce",
  "@metamask-previews/earn-controller": "11.0.0-preview-7334bbce",
  "@metamask-previews/eip-5792-middleware": "2.0.0-preview-7334bbce",
  "@metamask-previews/eip-7702-internal-rpc-middleware": "0.1.0-preview-7334bbce",
  "@metamask-previews/eip1193-permission-middleware": "1.0.2-preview-7334bbce",
  "@metamask-previews/ens-controller": "19.0.0-preview-7334bbce",
  "@metamask-previews/error-reporting-service": "3.0.0-preview-7334bbce",
  "@metamask-previews/eth-block-tracker": "15.0.0-preview-7334bbce",
  "@metamask-previews/eth-json-rpc-middleware": "22.0.0-preview-7334bbce",
  "@metamask-previews/eth-json-rpc-provider": "6.0.0-preview-7334bbce",
  "@metamask-previews/foundryup": "1.0.1-preview-7334bbce",
  "@metamask-previews/gas-fee-controller": "26.0.0-preview-7334bbce",
  "@metamask-previews/gator-permissions-controller": "0.6.0-preview-7334bbce",
  "@metamask-previews/json-rpc-engine": "10.2.0-preview-7334bbce",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.8-preview-7334bbce",
  "@metamask-previews/keyring-controller": "25.0.0-preview-7334bbce",
  "@metamask-previews/logging-controller": "7.0.1-preview-7334bbce",
  "@metamask-previews/message-manager": "14.1.0-preview-7334bbce",
  "@metamask-previews/messenger": "0.3.0-preview-7334bbce",
  "@metamask-previews/multichain-account-service": "4.0.0-preview-7334bbce",
  "@metamask-previews/multichain-api-middleware": "1.2.4-preview-7334bbce",
  "@metamask-previews/multichain-network-controller": "3.0.0-preview-7334bbce",
  "@metamask-previews/multichain-transactions-controller": "7.0.0-preview-7334bbce",
  "@metamask-previews/name-controller": "9.0.0-preview-7334bbce",
  "@metamask-previews/network-controller": "26.0.0-preview-7334bbce",
  "@metamask-previews/network-enablement-controller": "4.0.0-preview-7334bbce",
  "@metamask-previews/notification-services-controller": "21.0.0-preview-7334bbce",
  "@metamask-previews/permission-controller": "12.1.1-preview-7334bbce",
  "@metamask-previews/permission-log-controller": "5.0.0-preview-7334bbce",
  "@metamask-previews/phishing-controller": "16.1.0-preview-7334bbce",
  "@metamask-previews/polling-controller": "16.0.0-preview-7334bbce",
  "@metamask-previews/preferences-controller": "22.0.0-preview-7334bbce",
  "@metamask-previews/profile-sync-controller": "27.0.0-preview-7334bbce",
  "@metamask-previews/rate-limit-controller": "7.0.0-preview-7334bbce",
  "@metamask-previews/remote-feature-flag-controller": "2.0.1-preview-7334bbce",
  "@metamask-previews/sample-controllers": "4.0.0-preview-7334bbce",
  "@metamask-previews/seedless-onboarding-controller": "7.0.0-preview-7334bbce",
  "@metamask-previews/selected-network-controller": "26.0.0-preview-7334bbce",
  "@metamask-previews/shield-controller": "3.0.0-preview-7334bbce",
  "@metamask-previews/signature-controller": "37.0.0-preview-7334bbce",
  "@metamask-previews/subscription-controller": "5.0.0-preview-7334bbce",
  "@metamask-previews/token-search-discovery-controller": "4.0.0-preview-7334bbce",
  "@metamask-previews/transaction-controller": "62.3.0-preview-7334bbce",
  "@metamask-previews/transaction-pay-controller": "10.1.0-preview-7334bbce",
  "@metamask-previews/user-operation-controller": "41.0.0-preview-7334bbce"
}

github-actions[bot] avatar Nov 25 '25 15:11 github-actions[bot]