Skip to content

Commit ec1b5fc

Browse files
authored
feat(account-tree-controller): persist accountTree (#8437)
## Explanation We used to not persist the account-tree before and we were re-constructing it "fresh" upon `init`. However, some other controllers are dependent on the tree and have to wait for it to be built before consuming it. Since the tree should never really change between lock/unlock, that's ok to have a on-disk copy of it to speedup consumers. We will still try to re-construct it during `init` though (in case rules have changed or that new accounts have appeared that went unnoticed). ## References N/A ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Persists and rehydrates core controller state, which can affect startup/lock-unlock behavior and introduce stale/mismatched tree data if persisted state diverges from current accounts. > > **Overview** > `AccountTreeController` now **persists `accountTree`** (state metadata `persist: true`) instead of always recomputing it on `init`. > > The controller constructor now pre-initializes internal reverse lookup maps from the persisted tree via `#initTreeContext`, allowing methods like `getAccountContext`, `getAccountGroupObject`, and `getAccountsFromSelectedAccountGroup` to work *before* `init()` runs. > > Tests and snapshots are updated to cover pre-populated persisted state behavior and the new persisted-state surface, and the changelog documents the new persistence behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 65dedea. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 6dc536e commit ec1b5fc

File tree

3 files changed

+140
-1
lines changed

3 files changed

+140
-1
lines changed

packages/account-tree-controller/CHANGELOG.md

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

1010
### Changed
1111

12+
- Now persist `accounTree` ([#8437](https://github.com/MetaMask/core/pull/8437))
13+
- The tree is not persisted and can be used before `init` is called.
14+
- This allow consumers (like the assets controllers) to rely on the selected group's accounts right away.
1215
- Remove dynamic identifiers (wallet IDs, group IDs) from backup and sync thrown error messages to improve Sentry error grouping ([#8349](https://github.com/MetaMask/core/pull/8349))
1316
- Bump `@metamask/accounts-controller` from `^37.1.1` to `^37.2.0` ([#8363](https://github.com/MetaMask/core/pull/8363))
1417
- Bump `@metamask/keyring-controller` from `^25.1.1` to `^25.2.0` ([#8363](https://github.com/MetaMask/core/pull/8363))

packages/account-tree-controller/src/AccountTreeController.test.ts

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,46 @@ const MOCK_HARDWARE_ACCOUNT_1: InternalAccount = {
235235

236236
const mockGetSelectedMultichainAccountActionHandler = jest.fn();
237237

238+
const MOCK_PREPOPULATED_WALLET_ID = toMultichainAccountWalletId(
239+
MOCK_HD_KEYRING_1.metadata.id,
240+
);
241+
const MOCK_PREPOPULATED_GROUP_ID = toMultichainAccountGroupId(
242+
MOCK_PREPOPULATED_WALLET_ID,
243+
MOCK_HD_ACCOUNT_1.options.entropy.groupIndex,
244+
);
245+
const MOCK_PREPOPULATED_STATE: Partial<AccountTreeControllerState> = {
246+
selectedAccountGroup: MOCK_PREPOPULATED_GROUP_ID,
247+
accountTree: {
248+
wallets: {
249+
[MOCK_PREPOPULATED_WALLET_ID]: {
250+
id: MOCK_PREPOPULATED_WALLET_ID,
251+
type: AccountWalletType.Entropy,
252+
status: 'ready',
253+
groups: {
254+
[MOCK_PREPOPULATED_GROUP_ID]: {
255+
id: MOCK_PREPOPULATED_GROUP_ID,
256+
type: AccountGroupType.MultichainAccount,
257+
accounts: [MOCK_HD_ACCOUNT_1.id],
258+
metadata: {
259+
name: 'Account 1',
260+
entropy: {
261+
groupIndex: MOCK_HD_ACCOUNT_1.options.entropy.groupIndex,
262+
},
263+
pinned: false,
264+
hidden: false,
265+
lastSelected: 0,
266+
},
267+
},
268+
},
269+
metadata: {
270+
name: 'Wallet 1',
271+
entropy: { id: MOCK_HD_KEYRING_1.metadata.id },
272+
},
273+
},
274+
},
275+
},
276+
};
277+
238278
/**
239279
* Sets up the AccountTreeController for testing.
240280
*
@@ -877,6 +917,11 @@ describe('AccountTreeController', () => {
877917
keyrings: [MOCK_HD_KEYRING_1],
878918
});
879919

920+
// Reset mock call counts after construction: the constructor calls
921+
// #initTreeContext which seeds the fast-index Maps from persisted state,
922+
// incrementing the call counters before we even call init().
923+
jest.clearAllMocks();
924+
880925
controller.init();
881926
expect(
882927
mocks.AccountsController.listMultichainAccounts,
@@ -902,6 +947,11 @@ describe('AccountTreeController', () => {
902947
keyrings: [MOCK_HD_KEYRING_1],
903948
});
904949

950+
// Reset mock call counts after construction: the constructor calls
951+
// #initTreeContext which seeds the fast-index Maps from persisted state,
952+
// incrementing the call counters before we even call init().
953+
jest.clearAllMocks();
954+
905955
controller.init();
906956
expect(
907957
mocks.AccountsController.listMultichainAccounts,
@@ -1076,6 +1126,27 @@ describe('AccountTreeController', () => {
10761126
toMultichainAccountWalletId(MOCK_HD_KEYRING_2.metadata.id),
10771127
);
10781128
});
1129+
1130+
it('populates reverse mappings from persisted tree state without calling init()', () => {
1131+
const { controller } = setup({
1132+
accounts: [MOCK_HD_ACCOUNT_1],
1133+
keyrings: [MOCK_HD_KEYRING_1],
1134+
state: MOCK_PREPOPULATED_STATE,
1135+
});
1136+
1137+
// init() is NOT called — reverse mappings must be populated from the persisted tree.
1138+
const context = controller.getAccountContext(MOCK_HD_ACCOUNT_1.id);
1139+
expect(context).toBeDefined();
1140+
expect(context?.walletId).toBe(MOCK_PREPOPULATED_WALLET_ID);
1141+
expect(context?.groupId).toBe(MOCK_PREPOPULATED_GROUP_ID);
1142+
expect(context?.sortOrder).toBeDefined();
1143+
1144+
const group = controller.getAccountGroupObject(
1145+
MOCK_PREPOPULATED_GROUP_ID,
1146+
);
1147+
expect(group).toBeDefined();
1148+
expect(group?.id).toBe(MOCK_PREPOPULATED_GROUP_ID);
1149+
});
10791150
});
10801151

10811152
describe('getAccountsFromSelectAccountGroup', () => {
@@ -1147,6 +1218,19 @@ describe('AccountTreeController', () => {
11471218

11481219
expect(controller.getAccountsFromSelectedAccountGroup()).toHaveLength(0);
11491220
});
1221+
1222+
it('returns accounts from persisted state without calling init()', () => {
1223+
const { controller } = setup({
1224+
accounts: [MOCK_HD_ACCOUNT_1],
1225+
keyrings: [MOCK_HD_KEYRING_1],
1226+
state: MOCK_PREPOPULATED_STATE,
1227+
});
1228+
1229+
// init() is NOT called — accounts must be served from the persisted state.
1230+
expect(controller.getAccountsFromSelectedAccountGroup()).toStrictEqual([
1231+
MOCK_HD_ACCOUNT_1,
1232+
]);
1233+
});
11501234
});
11511235

11521236
describe('on AccountsController:accountsRemoved', () => {
@@ -4863,6 +4947,9 @@ describe('AccountTreeController', () => {
48634947
).toMatchInlineSnapshot(`
48644948
{
48654949
"accountGroupsMetadata": {},
4950+
"accountTree": {
4951+
"wallets": {},
4952+
},
48664953
"accountWalletsMetadata": {},
48674954
"hasAccountTreeSyncingSyncedAtLeastOnce": false,
48684955
"selectedAccountGroup": "",

packages/account-tree-controller/src/AccountTreeController.ts

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ const accountTreeControllerMetadata: StateMetadata<AccountTreeControllerState> =
6565
{
6666
accountTree: {
6767
includeInStateLogs: true,
68-
persist: false, // We do re-recompute this state everytime.
68+
persist: true,
6969
includeInDebugSnapshot: false,
7070
usedInUi: true,
7171
},
@@ -235,6 +235,12 @@ export class AccountTreeController extends BaseController<
235235
this.#createBackupAndSyncContext(),
236236
);
237237

238+
// We build the initial tree (from the state), but this might get updated
239+
// after `init` got called. Having it available now, allow our consumers to
240+
// re-use the state that was already available before we restart/lock the
241+
// wallet.
242+
this.#initTreeContext(this.state.accountTree);
243+
238244
this.messenger.subscribe('AccountsController:accountsAdded', (accounts) => {
239245
this.#handleAccountsAdded(accounts);
240246
});
@@ -275,6 +281,49 @@ export class AccountTreeController extends BaseController<
275281
);
276282
}
277283

284+
/**
285+
* Initialize the account tree from the given state.
286+
*
287+
* @param tree The account tree state to initialize from.
288+
*/
289+
#initTreeContext(tree: AccountTreeControllerState['accountTree']): void {
290+
// Fetch persisted accounts from the `AccountsController`.
291+
const accounts = new Map(
292+
this.#listAccounts().map((account) => [account.id, account]),
293+
);
294+
295+
for (const [walletId, wallet] of Object.entries(tree.wallets) as [
296+
AccountWalletId,
297+
AccountWalletObject,
298+
][]) {
299+
for (const [groupId, group] of Object.entries(wallet.groups) as [
300+
AccountGroupId,
301+
AccountGroupObject,
302+
][]) {
303+
this.#groupIdToWalletId.set(groupId, walletId);
304+
305+
// We still need to go through accounts for the sort order.
306+
for (const accountId of group.accounts) {
307+
const account = accounts.get(accountId);
308+
309+
// NOTE: The account should always be available, but if it is not, we
310+
// still let it inside the tree with a max sort order to avoid blocking
311+
// the entire tree construction.
312+
let sortOrder = MAX_SORT_ORDER;
313+
if (account) {
314+
sortOrder = ACCOUNT_TYPE_TO_SORT_ORDER[account.type];
315+
}
316+
317+
this.#accountIdToContext.set(accountId, {
318+
walletId,
319+
groupId,
320+
sortOrder,
321+
});
322+
}
323+
}
324+
}
325+
}
326+
278327
/**
279328
* Initialize the controller's state.
280329
*

0 commit comments

Comments
 (0)