Skip to content

Commit 5300158

Browse files
hmalik88ccharly
andauthored
perf(multichain-account-service)!: avoid excessive state syncing (#6654)
## 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. <ins>General formulas</ins> 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: <img width="678" height="401" alt="MisalignedLoops" src="https://github.com/user-attachments/assets/52009899-9d8b-4c66-8db3-b76b13fffc86" /> <img width="678" height="401" alt="MisalignedCalls" src="https://github.com/user-attachments/assets/b07222c7-ac86-4854-a29e-a364166bcc8f" /> <img width="678" height="401" alt="AlignedLoops" src="https://github.com/user-attachments/assets/92de8ae2-fae3-4071-b3bf-9575fb9a0f1a" /> <img width="678" height="401" alt="AlignedCalls" src="https://github.com/user-attachments/assets/bfe39332-edb8-4b7d-9623-ffe8e97cddf9" /> ## References - MetaMask/metamask-extension#38265 - MetaMask/metamask-mobile#25040 ## 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](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Introduces a centralized `ServiceState` and ID-based provider model to remove repeated controller scans and ad‑hoc syncing. > > - MultichainAccountService: builds `ServiceState` in one pass; `init` passes state slices to wallets/groups; unified `createMultichainAccountWallet` (import/create/restore); added `removeMultichainAccountWallet` action; removed reverse `accountIdToContext` and legacy sync handlers > - MultichainAccountWallet/Group: new `init`/`update` APIs; groups store provider→IDs, expose `getAccountIds`; alignment uses provider `alignAccounts`, aggregates per‑provider errors; background non‑EVM creation supported > - Providers: new `init(ids)` and `alignAccounts` on `BaseBip44AccountProvider`; providers track account IDs and fetch via `AccountsController:getAccount(s)`; `AccountProviderWrapper` forwards `init`, adds enable/disable handling > - EVM provider: derives deterministic account IDs, fetches via `getAccount`, reduces address scanning; discovery uses retry/timeout helpers > - Tests and changelog updated; BREAKING changes to provider interfaces and service init/creation flows > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit cd0d095. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
1 parent cb7e817 commit 5300158

22 files changed

+1432
-1019
lines changed

eslint-suppressions.json

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1092,20 +1092,11 @@
10921092
"packages/multichain-account-service/src/MultichainAccountWallet.test.ts": {
10931093
"id-length": {
10941094
"count": 2
1095-
},
1096-
"no-param-reassign": {
1097-
"count": 1
10981095
}
10991096
},
11001097
"packages/multichain-account-service/src/MultichainAccountWallet.ts": {
11011098
"@typescript-eslint/explicit-function-return-type": {
1102-
"count": 3
1103-
},
1104-
"@typescript-eslint/prefer-nullish-coalescing": {
1105-
"count": 1
1106-
},
1107-
"id-length": {
1108-
"count": 1
1099+
"count": 2
11091100
},
11101101
"require-atomic-updates": {
11111102
"count": 2
@@ -1126,11 +1117,6 @@
11261117
"count": 2
11271118
}
11281119
},
1129-
"packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts": {
1130-
"@typescript-eslint/explicit-function-return-type": {
1131-
"count": 4
1132-
}
1133-
},
11341120
"packages/multichain-account-service/src/providers/EvmAccountProvider.ts": {
11351121
"@typescript-eslint/naming-convention": {
11361122
"count": 1
@@ -1166,17 +1152,14 @@
11661152
},
11671153
"packages/multichain-account-service/src/tests/providers.ts": {
11681154
"@typescript-eslint/explicit-function-return-type": {
1169-
"count": 2
1155+
"count": 1
11701156
}
11711157
},
11721158
"packages/multichain-account-service/src/utils.ts": {
11731159
"@typescript-eslint/explicit-function-return-type": {
1174-
"count": 2
1175-
},
1176-
"id-denylist": {
11771160
"count": 1
11781161
},
1179-
"id-length": {
1162+
"id-denylist": {
11801163
"count": 1
11811164
}
11821165
},

packages/multichain-account-service/CHANGELOG.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- **BREAKING** A performance refactor was made around all the classes in this package ([#6654](https://github.com/MetaMask/core/pull/6654))
13+
- Add logic in the `createMultichainAccountWallet` method in `MultichainAccountService` so that it can handle all entry points: importing an SRP, recovering a vault and creating a new vault.
14+
- Add a `getAccountIds` method which returns all the account ids pertaining to a group.
15+
- Add an `addAccounts` method on the `BaseBip44AccountProvider` class which keeps track of all the account IDs that pertain to it.
16+
1017
### Changed
1118

1219
- Bump `@metamask/keyring-controller` from `^25.0.0` to `^25.1.0` ([#7713](https://github.com/MetaMask/core/pull/7713))
@@ -123,8 +130,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
123130

124131
### Changed
125132

133+
- **BREAKING** A performance refactor was made around all the classes in this package ([#6654](https://github.com/MetaMask/core/pull/6654))
134+
- The `MultichainAccountService` is refactored to construct a top level service state for its `init` function, this state is passed down to the `MultichainAccountWallet` and `MultichainAccountGroup` classes in slices for them to construct their internal states.
135+
- Additional state is generated at the entry points where it needs to be updated i.e. `createMultichainAccountGroup`, `discoverAccounts` and `alignAccounts`.
136+
- We no longer prevent group creation if some providers' `createAccounts` calls fail during group creation, only if they all fail.
137+
- The `getAccounts` method in the `BaseBip44AccountProvider` class no longer relies on fetching the entire list of internal accounts from the `AccountsController`, instead it gets the specific accounts that it stores in its internal accounts list.
138+
- The `EvmAccountProvider` no longer fetches from the `AccountController` to get an account for its ID, we deterministically get the associated account ID through `getUUIDFromAddressOfNormalAccount`.
139+
- The `EvmAccountProvider` now uses the `getAccount` method from the `AccountsController` when fetching an account after account creation as it is more efficient.
126140
- Bump `@metamask/base-controller` from `^8.4.1` to `^8.4.2` ([#6917](https://github.com/MetaMask/core/pull/6917))
127141

142+
### Removed
143+
144+
- **BREAKING** A performance refactor was made around all the classes in this package ([#6654](https://github.com/MetaMask/core/pull/6654))
145+
- Remove `#handleOnAccountAdded` and `#handleOnAccountRemoved` methods in `MultichainAccountService` due to internal state being updated within the service.
146+
- Remove `getAccountContext` (and associated map) in the `MultichainAccountService` as the service no longer uses that method.
147+
- Remove the `sync` method in favor of the sole `init` method for both `MultichainAccountWallet` and `MultichainAccountGroup`.
148+
128149
## [1.6.1]
129150

130151
### Changed

packages/multichain-account-service/src/MultichainAccountGroup.test.ts

Lines changed: 98 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,29 @@
11
import type { Bip44Account } from '@metamask/account-api';
22
import {
33
AccountGroupType,
4+
isBip44Account,
45
toMultichainAccountGroupId,
56
toMultichainAccountWalletId,
67
} from '@metamask/account-api';
78
import { EthScope, SolScope } from '@metamask/keyring-api';
89
import type { InternalAccount } from '@metamask/keyring-internal-api';
910

11+
import type { GroupState } from './MultichainAccountGroup';
1012
import { MultichainAccountGroup } from './MultichainAccountGroup';
1113
import { MultichainAccountWallet } from './MultichainAccountWallet';
14+
import type { RootMessenger, MockAccountProvider } from './tests';
1215
import {
1316
MOCK_SNAP_ACCOUNT_2,
1417
MOCK_WALLET_1_BTC_P2TR_ACCOUNT,
1518
MOCK_WALLET_1_BTC_P2WPKH_ACCOUNT,
1619
MOCK_WALLET_1_ENTROPY_SOURCE,
1720
MOCK_WALLET_1_EVM_ACCOUNT,
1821
MOCK_WALLET_1_SOL_ACCOUNT,
19-
setupNamedAccountProvider,
22+
setupBip44AccountProvider,
2023
getMultichainAccountServiceMessenger,
2124
getRootMessenger,
25+
mockCreateAccountsOnce,
2226
} from './tests';
23-
import type { MockAccountProvider, RootMessenger } from './tests';
2427
import type { MultichainAccountServiceMessenger } from './types';
2528

2629
function setup({
@@ -45,8 +48,11 @@ function setup({
4548
providers: MockAccountProvider[];
4649
messenger: MultichainAccountServiceMessenger;
4750
} {
48-
const providers = accounts.map((providerAccounts) => {
49-
return setupNamedAccountProvider({ accounts: providerAccounts });
51+
const providers = accounts.map((providerAccounts, idx) => {
52+
return setupBip44AccountProvider({
53+
name: `Provider ${idx + 1}`,
54+
accounts: providerAccounts,
55+
});
5056
});
5157

5258
const serviceMessenger = getMultichainAccountServiceMessenger(messenger);
@@ -64,6 +70,18 @@ function setup({
6470
messenger: serviceMessenger,
6571
});
6672

73+
// Initialize group state from provided accounts so that constructor tests
74+
// observe accounts immediately
75+
const groupState = providers.reduce<GroupState>((state, provider, idx) => {
76+
const ids = accounts[idx].filter(isBip44Account).map((a) => a.id);
77+
if (ids.length > 0) {
78+
state[provider.getName()] = ids;
79+
}
80+
return state;
81+
}, {});
82+
83+
group.init(groupState);
84+
6785
return { wallet, group, providers, messenger: serviceMessenger };
6886
}
6987

@@ -88,6 +106,10 @@ describe('MultichainAccount', () => {
88106
expect(group.type).toBe(AccountGroupType.MultichainAccount);
89107
expect(group.groupIndex).toBe(groupIndex);
90108
expect(group.wallet).toStrictEqual(wallet);
109+
expect(group.hasAccounts()).toBe(true);
110+
expect(group.getAccountIds()).toStrictEqual(
111+
expectedAccounts.map((a) => a.id),
112+
);
91113
expect(group.getAccounts()).toHaveLength(expectedAccounts.length);
92114
expect(group.getAccounts()).toStrictEqual(expectedAccounts);
93115
});
@@ -160,62 +182,118 @@ describe('MultichainAccount', () => {
160182
});
161183

162184
describe('alignAccounts', () => {
163-
it('creates missing accounts only for providers with no accounts', async () => {
185+
it('aligns accounts for all providers', async () => {
164186
const groupIndex = 0;
165187
const { group, providers, wallet } = setup({
166188
groupIndex,
167-
accounts: [
168-
[MOCK_WALLET_1_EVM_ACCOUNT], // provider[0] already has group 0
169-
[], // provider[1] missing group 0
170-
],
189+
accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
171190
});
172191

192+
mockCreateAccountsOnce(providers[0], [MOCK_WALLET_1_EVM_ACCOUNT]);
193+
mockCreateAccountsOnce(providers[1], [MOCK_WALLET_1_SOL_ACCOUNT]);
194+
173195
await group.alignAccounts();
174196

175-
expect(providers[0].createAccounts).not.toHaveBeenCalled();
197+
expect(providers[0].createAccounts).toHaveBeenCalledWith({
198+
entropySource: wallet.entropySource,
199+
groupIndex,
200+
});
176201
expect(providers[1].createAccounts).toHaveBeenCalledWith({
177202
entropySource: wallet.entropySource,
178203
groupIndex,
179204
});
205+
206+
expect(group.getAccounts()).toHaveLength(2);
207+
expect(group.getAccounts()).toStrictEqual([
208+
MOCK_WALLET_1_EVM_ACCOUNT,
209+
MOCK_WALLET_1_SOL_ACCOUNT,
210+
]);
180211
});
181212

182-
it('does nothing when already aligned', async () => {
213+
it('warns if alignment fails for a single provider', async () => {
183214
const groupIndex = 0;
184-
const { group, providers } = setup({
215+
const { group, providers, wallet } = setup({
185216
groupIndex,
186-
accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], [MOCK_WALLET_1_SOL_ACCOUNT]],
217+
accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
187218
});
188219

220+
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
221+
providers[1].createAccounts.mockRejectedValueOnce(
222+
new Error('Unable to create accounts'),
223+
);
224+
189225
await group.alignAccounts();
190226

191-
expect(providers[0].createAccounts).not.toHaveBeenCalled();
192-
expect(providers[1].createAccounts).not.toHaveBeenCalled();
227+
expect(providers[1].createAccounts).toHaveBeenCalledWith({
228+
entropySource: wallet.entropySource,
229+
groupIndex,
230+
});
231+
expect(consoleSpy).toHaveBeenCalledWith(
232+
`Failed to fully align multichain account group for entropy ID: ${wallet.entropySource} and group index: ${groupIndex}, some accounts might be missing. Provider threw the following error:\n- Provider 2: Unable to create accounts`,
233+
);
193234
});
194235

195-
it('warns if provider alignment fails', async () => {
236+
it('warns if alignment fails for multiple providers', async () => {
196237
const groupIndex = 0;
197238
const { group, providers, wallet } = setup({
198239
groupIndex,
199-
accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
240+
accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], [], []],
200241
});
201242

202243
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
203244
providers[1].createAccounts.mockRejectedValueOnce(
204245
new Error('Unable to create accounts'),
205246
);
206247

248+
providers[2].createAccounts.mockRejectedValueOnce(
249+
new Error('Unable to create accounts'),
250+
);
251+
207252
await group.alignAccounts();
208253

209-
expect(providers[0].createAccounts).not.toHaveBeenCalled();
210254
expect(providers[1].createAccounts).toHaveBeenCalledWith({
211255
entropySource: wallet.entropySource,
212256
groupIndex,
213257
});
258+
expect(providers[2].createAccounts).toHaveBeenCalledWith({
259+
entropySource: wallet.entropySource,
260+
groupIndex,
261+
});
214262
expect(consoleSpy).toHaveBeenCalledWith(
215-
`Failed to fully align multichain account group for entropy ID: ${wallet.entropySource} and group index: ${groupIndex}, some accounts might be missing`,
263+
`Failed to fully align multichain account group for entropy ID: ${wallet.entropySource} and group index: ${groupIndex}, some accounts might be missing. Providers threw the following errors:\n- Provider 2: Unable to create accounts\n- Provider 3: Unable to create accounts`,
216264
);
217265
});
218266

267+
it('does not create accounts when a provider is disabled', async () => {
268+
const groupIndex = 0;
269+
const { group, providers } = setup({
270+
groupIndex,
271+
accounts: [[], []],
272+
});
273+
274+
providers[1].setEnabled(false);
275+
await group.alignAccounts();
276+
277+
expect(providers[0].createAccounts).toHaveBeenCalled();
278+
expect(providers[1].createAccounts).not.toHaveBeenCalled();
279+
});
280+
281+
it('removes accounts from the group when a provider is disabled', async () => {
282+
const groupIndex = 0;
283+
const { group, providers } = setup({
284+
groupIndex,
285+
accounts: [[MOCK_WALLET_1_EVM_ACCOUNT], []],
286+
});
287+
288+
providers[0].setEnabled(false);
289+
await group.alignAccounts();
290+
291+
expect(providers[0].createAccounts).not.toHaveBeenCalled();
292+
expect(providers[1].createAccounts).toHaveBeenCalled();
293+
294+
expect(group.getAccount(MOCK_WALLET_1_EVM_ACCOUNT.id)).toBeUndefined();
295+
});
296+
219297
it('captures an error when a provider fails to create its account', async () => {
220298
const groupIndex = 0;
221299
const { group, providers, messenger } = setup({
@@ -227,7 +305,7 @@ describe('MultichainAccount', () => {
227305
const captureExceptionSpy = jest.spyOn(messenger, 'captureException');
228306
await group.alignAccounts();
229307
expect(captureExceptionSpy).toHaveBeenCalledWith(
230-
new Error('Unable to align accounts with provider "Mocked Provider"'),
308+
new Error('Unable to align accounts with provider "Provider 2"'),
231309
);
232310
expect(captureExceptionSpy.mock.lastCall[0]).toHaveProperty(
233311
'cause',

0 commit comments

Comments
 (0)