Skip to content

refactor(wallet-cli): Replace { wallet, store } with { wallet, dispose }#8847

Closed
sirtimid wants to merge 1 commit into
rekm/wallet-clifrom
sirtimid/wallet-cli-dispose-handle
Closed

refactor(wallet-cli): Replace { wallet, store } with { wallet, dispose }#8847
sirtimid wants to merge 1 commit into
rekm/wallet-clifrom
sirtimid/wallet-cli-dispose-handle

Conversation

@sirtimid

Copy link
Copy Markdown
Member

Summary

  • createWallet in daemon/wallet-factory now returns { wallet, dispose } instead of { wallet, store }. dispose is the single owner of teardown — it runs await wallet.destroy() then store.close(), with per-step failures forwarded to the supplied logger (or console.error if none).
  • The two daemon cleanup ladders in daemon-entry.ts (startup-failure catch block + SIGTERM/SIGINT shutdown closure) now both call dispose instead of inlining the order, so a future third call site or extra teardown step can't drift.
  • The disposer is idempotent (concurrent and sequential calls coalesce onto a single teardown promise), closes the store even when wallet.destroy() rejects, and survives a throwing user-supplied logger.
  • The pre-resolution catch block inside createWallet was tightened to route through the same reporter, so a store.close() throw during first-run cleanup can no longer mask the original failure (e.g. bad SRP).

Closes #8779. Builds on #8446 (the deferred refactor was tracked there).

Test plan

  • yarn workspace @metamask/wallet-cli run test — 250 passing, 100% statements/branches/funcs/lines on wallet-factory.ts and daemon-entry.ts.
  • yarn workspace @metamask/wallet-cli run build — clean.
  • yarn eslint packages/wallet-cli/src — clean.
  • yarn workspace @metamask/wallet-cli run changelog:validate — clean (no entry; the package is unreleased and this is internal).
  • New tests cover: destroy-before-close ordering, concurrent + sequential coalescence, store-closes-even-when-destroy-rejects, log routing for each step, console.error fallback when no logger supplied, console.error fallback when the supplied logger throws, and store.close throwing during first-run cleanup not masking the original error.

🤖 Generated with Claude Code

`createWallet` now returns a `dispose` callback that owns the
`wallet.destroy()` → `store.close()` ordering and reports per-step
failures through the supplied logger. Both daemon-entry cleanup
ladders (startup-failure catch and SIGTERM/SIGINT shutdown) call
`dispose` instead of inlining the order, so a future third call site
or extra teardown step (e.g. SQLite WAL flush) can't drift.

The disposer is idempotent across concurrent and sequential calls,
closes the store even when destroy rejects, and falls back to
`console.error` if no logger (or a throwing one) is supplied. The
pre-resolution catch block in `createWallet` was also tightened to
route through the same reporter so a `store.close()` throw can't
mask the original failure (e.g. bad SRP).

Closes #8779

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sirtimid

Copy link
Copy Markdown
Member Author

Superseded by #9226, which lands createWallet + daemon-entry directly on main and folds in this { wallet, dispose } refactor (idempotent, resilient teardown). It additionally adds the unsubscribe step that this branch's older rekm/wallet-cli base didn't need (on main, post-#9067 subscribeToChanges returns an unsubscribe fn rather than relying on a Wallet:destroyed event). #8779 is closed by #9226.

@sirtimid sirtimid closed this Jun 23, 2026
pull Bot pushed a commit to dmrazzy/core that referenced this pull request Jun 25, 2026
…#9226)

## Explanation

Adds the wallet factory and daemon entry point for
`@metamask/wallet-cli`, after the scaffold (MetaMask#9065), persistence (MetaMask#9067),
and transport (MetaMask#9108) slices:

- **`wallet-factory.ts`** — `createWallet()` builds a `@metamask/wallet`
`Wallet` over the SQLite `KeyValueStore`, hydrates it from persisted
state, imports the SRP on first run, and returns `{ wallet, dispose }`.
`dispose` (idempotent, resilient) runs unsubscribe → `wallet.destroy()`
→ `store.close()`.
- **`daemon-entry.ts`** — the daemon process: claims the PID slot,
builds the wallet, serves the messenger over the owner-only Unix socket,
and tears down via `dispose` on shutdown.

It uses the current `@metamask/wallet` `instanceOptions` API —
`storageService` (in-memory adapter), `connectivityController`
(`AlwaysOnlineAdapter`), and `remoteFeatureFlagController`.
`NetworkController` and `TransactionController` slots are left as TODOs
until they're wired on `main`.

Supersedes draft MetaMask#8847 (the `{ wallet, dispose }` refactor), folded in
here on top of `main`.

Closes MetaMask#8779.

## Checklist

- [x] `build`, `test` (100% coverage on both new files), `lint`,
`changelog:validate`, and `constraints` pass.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **High Risk**
> Touches keyring/SRP import, encrypted vault persistence, and exposes
the full wallet messenger over a Unix socket with only filesystem
permissions as the access boundary.
> 
> **Overview**
> Adds **`createWallet`** and **`daemon-entry`** so the CLI can run a
long-lived background wallet process backed by SQLite persistence.
> 
> **`createWallet`** opens a `KeyValueStore`, hydrates controller state
(via a short-lived metadata probe `Wallet`), wires headless
`instanceOptions` (in-memory `storageService`, `AlwaysOnlineAdapter`,
Infura + remote feature flags), subscribes persistence, runs
**`wallet.init()`** (startup aborts if any step fails), imports the SRP
on first run when no vault exists, and returns **`{ wallet, dispose }`**
with ordered, idempotent teardown. First-run failures can delete on-disk
DB files.
> 
> **`daemon-entry`** validates required env vars, **removes password/SRP
from `process.env`** after capture, locks the data dir to **`0o700`**,
runs **`claimDaemonSlot`** (stale socket/PID cleanup, refuse live
siblings), claims the daemon with an **exclusive PID write before
opening the DB**, builds the wallet, and serves **`getStatus`** plus a
**`call`** RPC that forwards arbitrary messenger actions. Shutdown/SIG
handlers coalesce teardown and only remove PID files the process owns.
> 
> Also adds **`remote-feature-flag-controller`** and
**`storage-service`** dependencies and a small **`Readonly`** typing
tweak in persistence subscriptions.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
180ffc1. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

1 participant