Skip to content

feat(wallet-cli): add SQLite-backed controller-state persistence#9067

Open
sirtimid wants to merge 4 commits into
mainfrom
sirtimid/wallet-cli-persistence
Open

feat(wallet-cli): add SQLite-backed controller-state persistence#9067
sirtimid wants to merge 4 commits into
mainfrom
sirtimid/wallet-cli-persistence

Conversation

@sirtimid

@sirtimid sirtimid commented Jun 9, 2026

Copy link
Copy Markdown
Member

Explanation

@metamask/wallet-cli runs a background daemon that hosts a @metamask/wallet instance. 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 synchronous better-sqlite3-backed key-value store (a single kv table 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 the Wallet constructor's state option.
  • subscribeToChanges(messenger, controllerMetadata, store, log?) — subscribes to every <Controller>:stateChanged event and writes persist-flagged top-level properties through to the store (applying StateDeriver persist functions, and deleting a key when its property is removed from state). It returns an unsubscribe handle; teardown is owned by the caller (a dispose() handle in a later PR), so there is no package-level teardown event.

better-sqlite3 native addon

better-sqlite3 ships a native C addon. The monorepo runs Yarn with enableScripts: false, so the addon is not built during yarn install. To handle this:

  • a test:prepare step (scripts/install-binaries.sh) fetches a matching prebuild via prebuild-install before tests run;
  • engines.node is set to >=20 (better-sqlite3 v12 only ships prebuilt binaries for Node 20+);
  • @metamask/wallet-cli is excluded from the Node 18.x CI test matrix;
  • yarn.config.cjs exempts the package from the standard test-script and engines.node constraints accordingly.

References

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
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

🤖 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-cli so wallet controller state can survive restarts (not yet wired into the daemon).

A synchronous KeyValueStore (better-sqlite3, JSON values in a kv table) is the storage backend. loadState rebuilds { controllerName: { property: value } } for Wallet construction, honoring only properties still marked persist in controller metadata so stale rows are not resurrected. subscribeToChanges listens on <Controller>:stateChanged, writes persist-flagged top-level fields from Immer patches (including StateDeriver transforms and deletes when properties are removed), logs write failures without stopping the process, and returns an unsubscribe handle.

better-sqlite3 is added with engines.node >=20, a test:prepare / install-binaries.sh prebuild step (monorepo enableScripts: false), CI matrix exclusion for Node 18 on this package, and yarn.config.cjs exceptions for the custom test script and engines field. Dependencies on @metamask/wallet, @metamask/base-controller, and immer are 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.

sirtimid added a commit that referenced this pull request Jun 9, 2026
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@socket-security

socket-security Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​types/​better-sqlite3@​7.6.131001007180100
Addedbetter-sqlite3@​12.10.08810010091100

View full report

@socket-security

socket-security Bot commented Jun 9, 2026

Copy link
Copy Markdown

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:

  • better-sqlite3@12.10.0
  • prebuild-install@7.1.3
  • detect-libc@2.1.2
  • simple-get@4.0.1
  • tunnel-agent@0.6.0
  • chownr@1.1.4

View full report

@sirtimid

sirtimid commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@SocketSecurity ignore npm/better-sqlite3@12.10.0

@sirtimid

sirtimid commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

@SocketSecurity ignore npm/detect-libc@2.1.2
Shell access = spawning getconf/ldd to detect glibc vs musl for prebuild selection. Expected & necessary.

@SocketSecurity ignore npm/simple-get@4.0.1
Network access = the HTTP(S) client prebuild-install uses to fetch the binary from GitHub releases.

@SocketSecurity ignore npm/tunnel-agent@0.6.0
Network access = HTTPS-over-proxy tunneling for the prebuild download; "code anomaly" is a false positive.

@SocketSecurity ignore npm/chownr@1.1.4
"Code anomaly" false positive on isaacs' recursive-chown util, used via tar-fs to unpack the prebuild tarball.

@SocketSecurity ignore npm/prebuild-install@7.1.3
Socket flags as deprecated; it's better-sqlite3 v12's bundled prebuild fetcher, runs only at test-prep, not shipped.

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.

@sirtimid sirtimid force-pushed the sirtimid/wallet-cli-scaffold branch 2 times, most recently from 94090fe to 5ef15ec Compare June 11, 2026 15:25
Base automatically changed from sirtimid/wallet-cli-scaffold to main June 11, 2026 16:44
sirtimid and others added 3 commits June 11, 2026 18:31
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>
@sirtimid sirtimid force-pushed the sirtimid/wallet-cli-persistence branch from a238e5d to fb00cf0 Compare June 11, 2026 17:40
@sirtimid sirtimid marked this pull request as ready for review June 11, 2026 17:41
@sirtimid sirtimid requested review from a team as code owners June 11, 2026 17:41
@sirtimid sirtimid temporarily deployed to default-branch June 11, 2026 17:41 — with GitHub Actions Inactive

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread packages/wallet-cli/src/persistence/persistence.ts
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>

@grypez grypez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Familiar! Approved

@sirtimid sirtimid requested a review from FrederikBolding June 12, 2026 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wallet-cli: Move sqlite persistence from wallet to wallet-cli

2 participants