-
-
Notifications
You must be signed in to change notification settings - Fork 250
refactor(multichain-account-service): Improved performance across package classes and improved error messages #6654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
MultichainAccountService
, MultichainAccountWallet
, MultichainAccountGroup
performance and DevX improvements// Add the accounts to the provider's internal list of account IDs | ||
provider.addAccounts(accountIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment about:
Given the providers are the sources of accounts...
* | ||
* @param accounts - The accounts to add. | ||
*/ | ||
addAccounts(accounts: Bip44Account<KeyringAccount>['id'][]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/multichain-account-service/src/MultichainAccountService.ts
Outdated
Show resolved
Hide resolved
const wallet = new MultichainAccountWallet({ | ||
entropySource, | ||
providers: this.#providers, | ||
messenger: this.#messenger, | ||
}); | ||
wallet.init(serviceState[entropySource]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Explanation
MultichainAccountService
createMultichainAccountWallet
) that handles import/restore/new vault.ServiceState
index in one pass and passes state slices to wallets/groups (cuts repeated controller scans/calls).init
path and removed deadaccountIdToContext
mapping.MultichainAccountWallet
init
now consumes a pre-sliced wallet state (entropySource → groups → providerName → ids) instead of querying providers.MultichainAccountGroup
init
registers account IDs per provider and fills reverse maps; callsprovider.addAccounts(ids)
to keep providers in sync.getAccountIds()
for direct access to underlying IDs.BaseBip44AccountProvider
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
getAccount(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
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
Note
Major refactor to make multichain account management state-driven with a unified wallet creation flow, plus new AccountsController and KeyringController actions to support efficient lookups and vault operations.
ServiceState
; wallets/groups now useinit(...)
with pre-sliced state.createMultichainAccountWallet
supporting import/create/restore flows; validates params and uses new keyring actions.MultichainAccountWallet
):init(walletState)
; create groups from state; background provider creation; clearer warnings; discovery builds groups and updates state.MultichainAccountGroup
):init(groupState)
,getAccountIds()
; register provider IDs; skip disabled providers; aggregate errors with provider names.BaseBip44AccountProvider
: track account IDs (addAccounts
,removeAccountsFromList
);getAccounts
usesAccountsController:getAccounts
.EvmAccountProvider
: derive deterministic account ID; fetch viagetAccount
; keep retry/timeout discovery.AccountProviderWrapper
: addisDisabled
helper.getAccounts(ids)
action/handler (+ tests/exports).createNewVaultAndKeychain
andcreateNewVaultAndRestore
(+ handlers); expose for service flows.Written by Cursor Bugbot for commit 98db5d3. This will update automatically on new commits. Configure here.