feat: add persistent metadata for account groups and wallets #6221
Conversation
- Allowed users to customize names, pinning, hiding, and collapsing states. - Added methods to set custom names, toggle pinned and hidden states for account groups, and toggle collapsed state for account wallets. - Updated types to reflect new metadata structures for better state management.
- Added tests to verify persistence of custom names, pinned, and hidden states for account groups and wallets across initialization calls. - Ensured that metadata structures are correctly initialized and preserved during state updates. - Included checks for handling non-existent groups and wallets gracefully without throwing errors.
There was a problem hiding this comment.
Will review more in-depth tomorrow, added comments in the meantime :)
Also, we will need to change the shape of metadata properties following the discussions we recently had:
So that they are all objects that contain a value and a lastUpdatedAt property.
Example (only a suggestion, let's discuss with everybody :)) :
export type UpdatableField<T> = { value: T; lastUpdatedAt: number };
...
type IsAccountWalletObject<
Type extends {
type: AccountWalletType;
id: AccountWalletId;
groups: {
[groupId: AccountGroupId]: AccountGroupObject;
};
metadata: {
name: UpdatableField<string>;
pinned: UpdatableField<boolean>;
// ...etc
};
},
> = Type;
| /** Custom name set by user, overrides default naming logic */ | ||
| name?: string; | ||
| /** Whether this group is pinned in the UI */ | ||
| pinned?: boolean; | ||
| /** Whether this group is hidden in the UI */ | ||
| hidden?: boolean; | ||
| /** Timestamp of last metadata update for sync conflict resolution */ | ||
| lastUpdatedAt?: number; |
There was a problem hiding this comment.
Yes, the "metadata" might becomes ambiguous... I think we should have 2 types like:
type Account{Group,Wallet}PersistedMetadata = { ... };
type Account{Group,Wallet}Metadata = { ... };and intersect the 2 types together on the *Object types.
This way, we could avoid having this Omit<..., 'name'> which kinda feels out-of-place (I was not a big fan of it when I added it, but I think it makes sense to "move" the name to some other types now).
| @@ -30,6 +58,10 @@ export type AccountTreeControllerState = { | |||
| }; | |||
| selectedAccountGroup: AccountGroupId | ''; | |||
| }; | |||
| /** Persistent metadata for account groups (names, pinning, hiding, sync timestamps) */ | |||
| accountGroupsMetadata: Record<AccountGroupId, AccountGroupMetadata>; | |||
There was a problem hiding this comment.
And then here have
Record<AccountGroupId, AccountGroupObject['metadata']>Record<AccountWalletId, AccountWalletObject['metadata']>
There was a problem hiding this comment.
If we go with having Account{Group,Wallet}PersistedMetadata, we could use those here instead.
There was a problem hiding this comment.
Pardon me, perhaps I don't understand correctly. As far as I understand:
Option 1 (me, Charly): separate persistent and "tree building" types
// Controller state
type AccountGroupPersistedMetadata = {
name?: UpdatableField<string>;
pinned?: UpdatableField<boolean>;
hidden?: UpdatableField<boolean>;
};
// During tree building, objects could extract .value from UpdatableField.value
type AccountGroupTreeMetadata = {
name: string;
pinned: boolean;
hidden: boolean;
};
Option 2 (Mathieu): unified
// Tree objects store UpdatableField directly
type AccountGroupMetadata = {
name: UpdatableField<string>;
entropy: { groupIndex: number };
pinned: UpdatableField<boolean>;
hidden: UpdatableField<boolean>;
};
In the latter option, the controller doesn't use a different type, so: accountGroupsMetadata: Record<AccountGroupId, AccountGroupObject['metadata']>;
BUT... the UI should be adjusted to access group.metadata.pinned.value.. no? @ccharly @mathieuartu
There was a problem hiding this comment.
Here's was I had in mind to "extract" a "normal object" out from a *PersistedMetadata type:
Which is:
type UpdatableField<Value> = {
value: Value;
lastUpdatedAt: number;
}
type ToValue<Field> = Field extends UpdatableField<unknown> ? Field['value'] : Field;
type ToObject<
ObjectValue extends Record<string, unknown>,
> = {
[Key in keyof ObjectValue]: ToValue<ObjectValue[Key]>
}
type PersistedMetadata = {
name: UpdatableField<string>;
test: boolean;
}
type Metadata = ToObject<PersistedMetadata>;Which then could be used:
type AccountGroupPersistedMetadata = {
name: UpdatableField<string>;
pinned: UpdatableField<boolean>;
hidden: UpdatableField<boolean>;
}
type AccountGroupMultichainAccountObject = {
type: AccountGroupType.MultichainAccount;
id: MultichainAccountGroupId;
// Blockchain Accounts (at least 1 account per multichain-accounts):
accounts: [AccountId, ...AccountId[]];
metadata: ToObject<AccountGroupPersistedMetadata> & {
entropy: {
groupIndex: number;
};
};Note
The ToObject name is "too generic" IMO, but you get the idea!
| */ | ||
| export type AccountWalletMetadata = { | ||
| /** Custom name set by user, overrides default naming logic */ | ||
| name?: string; |
There was a problem hiding this comment.
Question: Should those properties be required instead?
There was a problem hiding this comment.
Optional makes sense to me because not all groups/wallets will have custom names initially. @ccharly do we prefer null values by default (empty strings or so) ?
| */ | ||
| setAccountGroupName(groupId: AccountGroupId, name: string): void { | ||
| this.update((state) => { | ||
| state.accountGroupsMetadata[groupId] = { |
There was a problem hiding this comment.
We should probably check that the groupId also exists in this map before using it.
IMO, throwing if the groupId is not "valid" would make sense here.
| }); | ||
|
|
||
| // Rebuild tree to apply the new name | ||
| this.init(); |
There was a problem hiding this comment.
This seems "too much" to me. Though, we don't have any "contexts" or "mapping" using a groupId.
Maybe we should add one 🤔 so we could update the group node directly.
There was a problem hiding this comment.
I introduced a readonly #groupIdToWalletId: Map<AccountGroupId, AccountWalletId>; for fast lookup. Wdyt?
| this.update((state) => { | ||
| state.accountWalletsMetadata[walletId] = { | ||
| ...state.accountWalletsMetadata[walletId], | ||
| name, | ||
| lastUpdatedAt: Date.now(), | ||
| }; | ||
| }); | ||
|
|
||
| // Rebuild tree to apply the new name | ||
| this.init(); |
| this.update((state) => { | ||
| state.accountGroupsMetadata[groupId] = { | ||
| ...state.accountGroupsMetadata[groupId], | ||
| pinned, | ||
| lastUpdatedAt: Date.now(), | ||
| }; |
| state.accountGroupsMetadata[groupId] = { | ||
| ...state.accountGroupsMetadata[groupId], | ||
| hidden, | ||
| lastUpdatedAt: Date.now(), | ||
| }; |
| state.accountWalletsMetadata[walletId] = { | ||
| ...state.accountWalletsMetadata[walletId], | ||
| collapsed, | ||
| lastUpdatedAt: Date.now(), | ||
| }; |
| /** Custom name set by user, overrides default naming logic */ | ||
| name?: string; | ||
| /** Whether this group is pinned in the UI */ | ||
| pinned?: boolean; | ||
| /** Whether this group is hidden in the UI */ | ||
| hidden?: boolean; | ||
| /** Timestamp of last metadata update for sync conflict resolution */ | ||
| lastUpdatedAt?: number; |
There was a problem hiding this comment.
Yes, the "metadata" might becomes ambiguous... I think we should have 2 types like:
type Account{Group,Wallet}PersistedMetadata = { ... };
type Account{Group,Wallet}Metadata = { ... };and intersect the 2 types together on the *Object types.
This way, we could avoid having this Omit<..., 'name'> which kinda feels out-of-place (I was not a big fan of it when I added it, but I think it makes sense to "move" the name to some other types now).
| @@ -30,6 +58,10 @@ export type AccountTreeControllerState = { | |||
| }; | |||
| selectedAccountGroup: AccountGroupId | ''; | |||
| }; | |||
| /** Persistent metadata for account groups (names, pinning, hiding, sync timestamps) */ | |||
| accountGroupsMetadata: Record<AccountGroupId, AccountGroupMetadata>; | |||
There was a problem hiding this comment.
If we go with having Account{Group,Wallet}PersistedMetadata, we could use those here instead.
…ounts and groups - Updated account and group metadata to include updatable fields for names, pinned, hidden, and collapsed states. - Refactored methods to apply persisted metadata during initialization and state updates. - Improved type definitions to support new metadata structures, ensuring better state management and persistence.
…ta and related methods - Eliminated the collapsed state from account wallet and group metadata structures. - Refactored tests to remove references to the collapsed state. - Updated type definitions to reflect the removal of the collapsed property, ensuring cleaner state management.
…rsistence properties set - Enhanced methods to validate the existence of account groups and wallets before performing operations, throwing errors when they are not found. - Updated tests to reflect the new error handling behavior for non-existent groups and wallets. - Introduced a mapping for efficient access to wallet IDs associated with account groups, improving performance during updates.
…ount groups and wallets
…de pinned and hidden states - Added 'pinned' and 'hidden' properties to the metadata structure for account groups and wallets in the Entropy, Keyring, and Snap rules. - This enhancement aligns with previous updates to improve state management and metadata persistence.
Explanation
This PR adds persistence capabilities to the
AccountTreeControllerto support user customizations like custom names, pinning/hiding account groups, and sync metadata. Previously, the controller was stateless and would lose all user customizations on each tree rebuild.Current State & Problem
The
AccountTreeControllercurrently rebuilds the account tree from scratch using rules every time it initializes, resulting in:Solution Overview
This implementation adds persistent metadata storage with two new state properties that store user customizations separately from the tree structure:
accountGroupsMetadata: Record<AccountGroupId, AccountGroupMetadata>accountWalletsMetadata: Record<AccountWalletId, AccountWalletMetadata>Key Changes
Persistent State Structure:
AccountGroupMetadatatype supportingname,pinned,hidden, andlastUpdatedAtfieldsAccountWalletMetadatatype supportingnameandlastUpdatedAtfieldsEnhanced Tree Building Process:
#renameAccountWalletIfNeeded()and#renameAccountGroupIfNeeded()methods to prioritize persisted custom namesNew Public Methods:
setAccountGroupName(groupId, name)- Sets custom name for account groupssetAccountWalletName(walletId, name)- Sets custom name for walletssetAccountGroupPinned(groupId, pinned)- Controls group pinning statesetAccountGroupHidden(groupId, hidden)- Controls group visibilityOptimization Strategy:
Implementation Details
The persistence follows a metadata overlay pattern where:
This approach ensures backward compatibility while adding the persistence layer needed for Account Syncing V2 and advanced UI features like pinning/hiding.
References
lastUpdatedAttimestamps for conflict resolution)Checklist