feat(wallet): Add split persistence backed by better-sqlite3#8480
feat(wallet): Add split persistence backed by better-sqlite3#8480rekmarks wants to merge 20 commits intofeat/wallet-libraryfrom
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This comment was marked as resolved.
This comment was marked as resolved.
|
@SocketSecurity ignore npm/better-sqlite3@11.10.0 Yes, it has native binaries. |
|
@SocketSecurity ignore-all All alerts are due to |
8f58b8c to
f66328d
Compare
623f9af to
3fb60ee
Compare
1d6d1fc to
937ba80
Compare
937ba80 to
29c6ac6
Compare
0cf2d0d to
520a971
Compare
45d013e to
6ec54b9
Compare
|
|
||
| readonly #unsubscribePersistence: () => void; | ||
|
|
||
| constructor({ options, databasePath = ':memory:' }: WalletConstructorArgs) { |
There was a problem hiding this comment.
If Wallet is to be used in all clients it should be agnostic to persistence. I think these changes should target the CLI branch and be part of the CLI class.
There was a problem hiding this comment.
Alternatively we can come up with an interface where each client can pass in their own persistence, but it may be simpler to not deal with it at all in Wallet.
There was a problem hiding this comment.
Made some benign modifications to the Wallet interface and made it agnostic of persistence. I kept the persistence files in the wallet package for now to avoid creating a complicated PR stack (or bloating the CLI PR). We can move the persistence stuff out of the wallet package once this is merged.
| const key = `${controllerName}.${prop}`; | ||
| const persistFlag = metadata[prop]?.persist; |
There was a problem hiding this comment.
We should use deriveStateFromMetadata
There was a problem hiding this comment.
They're similar but I don't think it's a good fit. deriveStateFromMetadata iterates over every top-level state property and errors on missing properties. The subscribeToChanges handlers iterate over changed properties and deletes missing properties from persistence, which shouldn't happen in practice but that is not for the persistence layer to know. Then there's also the complex error handling logic in deriveStateFromMetadata which doesn't buy us anything here.
Persist controller state to SQLite using a per-property key-value scheme (one row per ControllerName.propertyName). Writes are synchronous within the same call stack as controller state updates, using stateChanged events and Immer patches to write only changed, persist-flagged properties. Defaults to ':memory:' when no database path is provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The build tsconfig is stricter than the dev tsconfig — the union type of controller instances does not expose the protected `destroy` method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review findings: add constructor cleanup on failure, make destroy() idempotent with Promise.allSettled and finally, handle remove patches via store.delete(), catch and log handler write failures, validate degenerate store keys, handle root state replacement patches, add contextful JSON.parse errors, tighten loadState return type, extract PersistableController type and storeKey utility, remove premature KeyValueStore public export, and add tests for new behaviors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
520a971 to
cfad387
Compare
Make `@metamask/wallet` platform-agnostic by removing the SQLite-backed `KeyValueStore` from the `Wallet` constructor. `Wallet` now accepts a flat `WalletOptions` (including an optional `state` payload) and owns no persistence state. Consumers arrange persistence themselves. - `Wallet` exposes a `controllerMetadata` getter so persistence can operate on metadata alone, without reaching into controller instances. - A new `Root:walletDestroyed` event is published after controllers tear down during `destroy()`. `subscribeToChanges` listens for it and self-unsubscribes, eliminating the need for callers to hold an unsubscribe handle. - `subscribeToChanges` now takes a `Record<string, StateMetadataConstraint>` instead of the controller-instance map, and specializes its messenger type to `RootMessenger<DefaultActions, DefaultEvents>` so subscribing to `Root:walletDestroyed` type-checks without suppression. - `KeyValueStore`, `loadState`, and `subscribeToChanges` are exposed via a new `@metamask/wallet/persistence` subpath export; they'll move to their own package later.
So that @metamask/wallet/persistence can be moved to its own package without needing to deep-import from the wallet package.
06b91f1 to
f4e8dfa
Compare
Renames install-anvil.sh to install-binaries.sh and adds a better-sqlite3 native addon rebuild step for CI environments where the prebuilt binding is absent or incompatible with the Node version. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Check for the compiled addon at `node_modules/better-sqlite3/build/Release/better_sqlite3.node` and skip `prebuild-install` when it's already present. Avoids unnecessary network calls on every `test:prepare` run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 4c16ea9. Configure here.

Summary
persist: truemetadata gets its own row in akvtable (key format:ControllerName.propertyName)controller.update()viastateChangedevent subscriptions, eliminating data loss windows:memory:when no database path is providedTest plan
yarn workspace @metamask/wallet exec jest --no-coverage --watchman=false src/persistence/— 22 unit tests covering KeyValueStore CRUD, loadState grouping, persist filtering, StateDeriver application, patch-based diffing, unsubscribe, and multi-controller scenariosyarn workspace @metamask/wallet exec tsc --noEmit— no new type errors🤖 Generated with Claude Code
Note
Medium Risk
Adds a native
better-sqlite3dependency plus new persistence wiring and a new wallet lifecycle event; failures here could impact state durability and build/install behavior across environments.Overview
Adds a new
@metamask/wallet/persistenceentrypoint that provides synchronous SQLite-backed persistence viabetter-sqlite3: aKeyValueStoreplusloadState()andsubscribeToChanges()to persist per-controller, per-property state (keyed asController.property) based on controllerpersistmetadata and Immer patches.Updates
Walletlifecycle to exposecontrollerMetadata, makedestroy()idempotent, tear down controllers viaPromise.allSettled, and publish a newRoot:walletDestroyedevent (also added toDefaultEvents) to support persistence self-unsubscribe; adjusts wallet construction to acceptWalletOptionsdirectly (with optionalstate).Build/test plumbing is updated to support the native addon: adds
better-sqlite3(+ typings), enables it in LavamoatallowScripts, replaces the test prep script withinstall-binaries.sh(installsanviland downloads the sqlite prebuild when missing), and documents rebuild steps in the wallet README.Reviewed by Cursor Bugbot for commit 58ab0c5. Bugbot is set up for automated code reviews on this repo. Configure here.