feat(wallet-cli): add SQLite-backed controller-state persistence#9067
feat(wallet-cli): add SQLite-backed controller-state persistence#9067sirtimid wants to merge 4 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
|
@SocketSecurity ignore npm/better-sqlite3@12.10.0 |
|
@SocketSecurity ignore npm/detect-libc@2.1.2 @SocketSecurity ignore npm/simple-get@4.0.1 @SocketSecurity ignore npm/tunnel-agent@0.6.0 @SocketSecurity ignore npm/chownr@1.1.4 @SocketSecurity ignore npm/prebuild-install@7.1.3 All alerts are transitive deps of better-sqlite3's prebuild-install toolchain (better-sqlite3 → prebuild-install → {detect-libc, simple-get, tunnel-agent, tar-fs → chownr}), which downloads/unpacks the prebuilt native SQLite addon. None are in @metamask/wallet-cli's own code or runtime path. The monorepo sets enableScripts: false, so none run on yarn install; prebuild-install runs only via the package's explicit test:prepare script and in CI. |
94090fe to
5ef15ec
Compare
Add the persistence layer for the wallet daemon: - `KeyValueStore`: a synchronous `better-sqlite3`-backed key-value store. - `loadState`: rehydrates per-controller state from the store. - `subscribeToChanges`: writes persist-flagged controller state through to disk on every `<Controller>:stateChanged` event, returning an unsubscribe handle. (The teardown handle, not a `Wallet:destroyed` event, owns unsubscription — that event does not exist in `@metamask/wallet`.) `better-sqlite3` ships a native addon that Yarn does not build at install time (`enableScripts: false`), so a `test:prepare` step fetches a matching prebuild via `scripts/install-binaries.sh`, `engines.node` is bumped to `>=20` (no Node 18 prebuilds), and the package is excluded from the CI Node 18.x test matrix. `yarn.config.cjs` exempts the package from the standard `test` script and engines constraints accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…build better-sqlite3 12.10.0 dropped its Node 20 (ABI v115) Linux prebuild that 12.9.0 still shipped, so on CI `prebuild-install` finds no matching binary and the `test:prepare` script aborts under `set -e`. Mirror better-sqlite3's own install step (`prebuild-install || node-gyp rebuild --release`) so the addon is compiled from source whenever no prebuild is published for the active Node ABI/platform. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a238e5d to
fb00cf0
Compare
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 fb00cf0. Configure here.
loadState merged every row from the store into the Wallet constructor state, while subscribeToChanges only ever writes persist-flagged properties. After a migration removed a controller or disabled a property's persist flag, the stale row lingered on disk and was resurrected on restart. loadState now takes controllerMetadata and skips rows that are not currently persist-flagged, through a shared isPersisted predicate used by both the read and write paths. Also addresses review feedback: - Narrow the persistence-write try/catch so a throwing StateDeriver surfaces on its own instead of being misreported as a write failure; cover the deriver-throws and serializes-to-undefined cases with tests. - Replace the statement-level @ts-expect-error on messenger subscribe/unsubscribe with a typed subscribeToStateChanged helper that keeps the handler payload shape compile-checked. - Surface prebuild-install's failure before falling back to a source build in install-binaries.sh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fb0950f to
b9854de
Compare

Explanation
@metamask/wallet-cliruns a background daemon that hosts a@metamask/walletinstance. For controller state to survive daemon restarts, it needs durable local storage. This PR adds that persistence layer. It is purely additive and not yet wired into a daemon process — the daemon entry point that consumes it lands in a follow-up.KeyValueStore— a synchronousbetter-sqlite3-backed key-value store (a singlekvtable with a TEXT key and a JSON-serialized TEXT value). Corrupt rows fail loud (a descriptive, key-scoped error) rather than returning garbage.loadState(store)— rehydrates persisted state into the{ [controllerName]: { [property]: value } }shape accepted by theWalletconstructor'sstateoption.subscribeToChanges(messenger, controllerMetadata, store, log?)— subscribes to every<Controller>:stateChangedevent and writes persist-flagged top-level properties through to the store (applyingStateDeriverpersist functions, and deleting a key when its property is removed from state). It returns an unsubscribe handle; teardown is owned by the caller (adispose()handle in a later PR), so there is no package-level teardown event.better-sqlite3native addonbetter-sqlite3ships a native C addon. The monorepo runs Yarn withenableScripts: false, so the addon is not built duringyarn install. To handle this:test:preparestep (scripts/install-binaries.sh) fetches a matching prebuild viaprebuild-installbefore tests run;engines.nodeis set to>=20(better-sqlite3 v12 only ships prebuilt binaries for Node 20+);@metamask/wallet-cliis excluded from the Node 18.x CI test matrix;yarn.config.cjsexempts the package from the standardtest-script andengines.nodeconstraints accordingly.References
wallettowallet-cli#8682@metamask/wallet-clipackage scaffold #9065 (the@metamask/wallet-clipackage scaffold). This PR targets that branch and should merge after it.Checklist
🤖 Generated with Claude Code
Note
Medium Risk
New durable state path can diverge from in-memory state on disk write failures (logged only today); native addon and Node 20+ requirement affect install and CI.
Overview
Adds SQLite-backed persistence in
@metamask/wallet-cliso wallet controller state can survive restarts (not yet wired into the daemon).A synchronous
KeyValueStore(better-sqlite3, JSON values in akvtable) is the storage backend.loadStaterebuilds{ controllerName: { property: value } }forWalletconstruction, honoring only properties still markedpersistin controller metadata so stale rows are not resurrected.subscribeToChangeslistens on<Controller>:stateChanged, writes persist-flagged top-level fields from Immer patches (includingStateDerivertransforms and deletes when properties are removed), logs write failures without stopping the process, and returns an unsubscribe handle.better-sqlite3is added withengines.node>=20, atest:prepare/install-binaries.shprebuild step (monorepoenableScripts: false), CI matrix exclusion for Node 18 on this package, andyarn.config.cjsexceptions for the custom test script and engines field. Dependencies on@metamask/wallet,@metamask/base-controller, andimmerare added with matching tsconfig project references.Reviewed by Cursor Bugbot for commit b9854de. Bugbot is set up for automated code reviews on this repo. Configure here.