feat(account-tree-controller): add payload/snapshot support - #9826
feat(account-tree-controller): add payload/snapshot support#9826ccharly wants to merge 11 commits into
Conversation
|
Using See: 9e0e993 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
gantunesr
left a comment
gantunesr
left a comment
There was a problem hiding this comment.
Looks good overall
- Would be good to have an integration test for the flow export→filter→serialize→deserialize. This is probably covered in the next PR for the controller wiring
| export type ExportStateOptions = { | ||
| /** When `true`, secrets (mnemonic / private keys) are included. Requires the vault to be unlocked. */ | ||
| includeSecrets?: boolean; | ||
| }; |
There was a problem hiding this comment.
do you think there is any explicit cleanup/teardown to do when secrets are exported?
There was a problem hiding this comment.
To like clearing them out of memory?
Like you'd like to have them being passed as options to control this?
There was a problem hiding this comment.
I investigated this a bit with claude.
The real problem is that we use raw string for the secrets, and it makes it hard to zero-ing memory completely with those (since we don't own the real memory buffer for those, only the view/value on it).
If we really wanted to do this, we would need to use real buffer types like ArrayBuffer or Uint8Array. But that means our payload will need special encoding functions (at least, to make it compatible with JSON-encoding for example).
Like we could have a .stringify() and .parse(json) for example.
But that would yet another steps before/after the snapshot's serialize and deserialize.
There was a problem hiding this comment.
After discussing a bit about this internally, I actually went for a different way of encoding those in our payload, so we "limit" the window of when those sensitive values are used.
They are just encoded differently, so it's more or less security-by-obscurity, but it's slightly better than having them in clear I think (and that's aligned with how we encode them elsewhere in the codebase)
| * @throws If `raw` is not a valid payload or its version is unsupported. | ||
| */ | ||
| static async deserialize(raw: unknown): Promise<AccountTreeSnapshot> { | ||
| // TODO: Use migration framework here. |
There was a problem hiding this comment.
Decided to omit the migration part for now. My plan was to use our new generic migration framework that we created for keyrings. Though, it's not needed here (we expect a v1 payload + the migration framework needs a slight rework around version handling + flattening the state/data being migrated).
Delaying this work for a bit to not block this PR.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ee63cf4. Configure here.
We indeed have some round-trips "integration" tests on the controller itself, to make sure the combo If needed, I'll add even more scenarios to also include filtering in this mix! |
2 participants
Explanation
First part of the new
:{import,export}Stateactions for the account-tree.This PR only exposes the new payload/snapshot types that will be used to import/export.
References
{import,export}Stateactions #9663Checklist
Note
Medium Risk
Touches wallet payload shapes and secret-bearing fields (mnemonics/private keys) with validation/redaction, but does not wire import/export into the controller yet; mistakes in schema or filtering could affect future cross-device account migration.
Overview
Introduces the v1 portable account-tree format and supporting types ahead of
importState/exportStateon the controller. New code lives underpackages/account-tree-controller/src/state/.AccountTreePayloaddefines a versioned flat snapshot (ACCOUNT_TREE_PAYLOAD_CURRENT_VERSION = 1) with mnemonic HD wallets and a merged private-key wallet entry, stablewallet:/ group ID helpers, and optional secret fields. Validation uses Superstruct (AccountTreePayloadStruct), rejects unknown versions and wallet types, enforces contiguous mnemonicgroupIndexvalues, and wraps secrets withsensitive()plusformatValidationErrorMessagesso validation errors do not leak mnemonics or keys.AccountTreeSnapshotis an immutable value object: entries are deep-cloned and frozen, withfilterWallets/filterGroups/filterAllGroups(dropping empty wallets),serialize()back to payload, anddeserialize()as the untrusted entry point. An optionalIdMapbridges local controller IDs to payload IDs through filtering; deserialized snapshots have no map unless supplied at construction.utilsadds JSON byte encoding,deepFreeze, and shared validation error formatting. Broad unit tests cover ID mapping, snapshot behavior, payload edge cases, and secret redaction.Reviewed by Cursor Bugbot for commit ccc122e. Bugbot is set up for automated code reviews on this repo. Configure here.