feat(wallet-cli): add daemon JSON-RPC socket transport#9108
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.
|
b9854de to
fbcddc5
Compare
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4a42757 to
57b77a3
Compare
grypez
left a comment
There was a problem hiding this comment.
A nit and a comment, nothing blocking. Approved!
| /** | ||
| * Check whether an unknown error is a Node.js system error with the given code. | ||
| * | ||
| * @param error - The error to check. | ||
| * @param code - The expected error code (e.g. 'ENOENT', 'EPERM'). | ||
| * @returns True if the error matches the code. | ||
| */ | ||
| export function isErrorWithCode(error: unknown, code: string): boolean { | ||
| // Duck-typed rather than `instanceof Error` so error values that crossed a | ||
| // realm boundary (e.g. from Node built-ins under jest's | ||
| // `--experimental-vm-modules`) still pass. | ||
| return ( | ||
| typeof error === 'object' && | ||
| error !== null && | ||
| hasProperty(error, 'code') && | ||
| error.code === code | ||
| ); | ||
| } |
There was a problem hiding this comment.
Nit: I think @metamask/utils already has a duck-typed isErrorWithCode implementation, though it doesn't apply the last error.code === code check.
if isErrorWithCode(error) {
switch (error.code) {
case 'ENOENT':
...
default:
throw error;
}
}
if isErrorWithCode(error) && error.code === 'ENOENT' { ... }
| // Restrict the socket to its owner. The daemon hosts an unlocked wallet, so | ||
| // a world-connectable socket would let any local user drive it. listen() | ||
| // creates the socket with umask-derived (typically world-accessible) perms. | ||
| await chmod(socketPath, 0o600); |
There was a problem hiding this comment.
Flagging, but out of scope for this PR: there is real time between when server.listen acquires the socket and when the socket permissions are set to user-read-write. The associated test shows that this line significantly improves the situation. To create the socket with appropriate permissions, we could ensure / rely upon either the directory containing socketPath to be 0o700 or umask to be 0o077 before socket creation. There may be even better ways.
There was a problem hiding this comment.
Agreed, and good catch. Let's apply the right fix on a follow-up.
Add the daemon's IPC transport layer for `@metamask/wallet-cli`. This is purely additive plumbing — no `mm` subcommand consumes it yet (commands land in a later slice), so there is no user-visible behavior change. - socket-line: newline-delimited read/write framing over a `net.Socket`. - daemon-client: `sendCommand` (one request per connection, id-correlated, retries once on transient connection errors) and `pingDaemon` (a `getStatus` health probe that classifies unreachable daemons by failure mode: refused / timeout / permission / protocol / other). - rpc-socket-server: `startRpcSocketServer` listens on a Unix socket and dispatches one JSON-RPC request per connection to a handler map, with a built-in `shutdown` method and idle-connection timeout. - daemon-spawn: `ensureDaemon` spawns a detached daemon and polls until the socket is responsive, refusing to take over a wedged or foreign socket. - stop-daemon: `stopDaemon` escalates from a graceful `shutdown` RPC through SIGTERM to SIGKILL, then cleans up the PID and socket files. - prompts: `confirmPurge` wraps the ESM-only `@inquirer/confirm`. - types: add `RpcHandler`, `RpcHandlerMap`, `DaemonStatusInfo`, and `DaemonSpawnConfig`. Adds `@inquirer/confirm` and `@metamask/rpc-errors` dependencies. All daemon modules are covered to the package's 100% thresholds, plus an end-to-end socket integration test exercising both halves over a real Unix socket. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collapse duplicate @inquirer/* entries introduced by the daemon transport dependencies to their highest resolved versions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address issues found in review of the ported daemon transport, before any command consumes it. Deviations from the verbatim rekm/wallet-cli modules: - rpc-socket-server: chmod the Unix socket to 0600 after listen(). It was created with umask-derived (typically world-accessible) permissions, so any local user could connect and drive the wallet-hosting daemon. - daemon-spawn: capture the ChildProcess 'error' and fail fast. It was only logged to stderr, so a failed spawn (bad interpreter, EACCES, ENOENT) made the readiness loop poll for the full 30s and throw a generic timeout that discarded the real cause. - socket-line: attach a one-shot 'error' listener for the duration of a writeLine. A socket 'error' arriving mid-write had no listener and would crash the process instead of rejecting the promise. - socket-integration.test: use a static node:net import so the file runs standalone without --experimental-vm-modules. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Delegate the duck-typed error-shape check to @metamask/utils' isErrorWithCode guard instead of hand-rolling it, keeping our two-arg wrapper to add only the specific-code comparison the library omits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
df56d1d to
60742bf
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ 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 60742bf. Configure here.
- Tear the listener down (and remove the socket file) if the post-listen chmod to 0600 fails, rather than leaving a possibly world-accessible socket accepting connections with no handle to close. - Destroy the socket on a failed connect in connectSocket so failed attempts (including the retry) no longer leak file descriptors. - Require the recorded pid to be gone before treating a graceful stop as successful, so a daemon that drops its socket but keeps running falls through to SIGTERM/SIGKILL instead of being orphaned. Also simplify the (unreleased) changelog and drop obvious comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#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>
…oke test (MetaMask#9255) ## Explanation The final landing slice for `@metamask/wallet-cli`, after the scaffold (MetaMask#9065), persistence (MetaMask#9067), transport (MetaMask#9108), and factory (MetaMask#9226) slices. It adds the user-facing command surface so the package is runnable: - **`src/commands/daemon/{start,stop,status,purge,call}.ts`** (+ tests) — the `mm daemon` command suite over the daemon layers already on `main`: `start` spawns the daemon and reports its socket, `status`/`stop` ping and tear it down, `purge` deletes the daemon state files, and `call` dispatches an arbitrary `Controller:method` messenger action. - **`bin/dev.mjs` + `bin/dev.cmd`** — the dev-mode launchers deferred from the scaffold slice, run via the `tsx` loader (`knip` `ignoreDependencies: ['tsx']`). - **`wallet-factory.e2e.test.ts`** — a smoke test that feeds the wired `instanceOptions` into a **real** `Wallet` (no mock) against `:memory:`, imports the SRP, and dispatches a messenger action — closing the gap flagged in MetaMask#9226 review. Offline/CI-safe (neither construction nor `wallet.init()` reaches the network). - **`src/test/run-command.ts`** + a README **Usage** section. ## Checklist - [x] `build`, `test` (100% coverage), `lint`, and `changelog:validate` pass. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > New CLI paths accept wallet secrets and can invoke arbitrary messenger actions over a local socket; `purge` deletes persisted wallet state, though safeguards limit deletion when the daemon is still responsive. > > **Overview** > Adds the runnable **`mm daemon`** command surface on top of the existing spawn/client/stop layers: **`start`** (Infura/password/SRP via flags or env), **`stop`**, **`status`** (orphan-PID and stale-PID warnings), **`purge`** (confirm/`--force`, whitelist-only file deletion, refuses purge while the daemon still responds), and **`call`** (JSON-RPC `call` with optional JSON-array args and timeout, TTY vs piped JSON output). > > Also ships **`bin/dev.mjs`/`dev.cmd`** (oclif dev mode via **`tsx`**), a **`runCommand`** oclif test harness, a **real `Wallet` `:memory:` e2e** for `createWallet`, Jest **Web Crypto polyfill** environment for KeyringController tests, README **Usage**, and knip/tsconfig tweaks for **`wallet-cli`**. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 15fbe92. 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>

Explanation
@metamask/wallet-cliruns a background daemon that hosts a@metamask/walletinstance; the CLI talks to it over a Unix socket. This PR adds that transport layer. It is purely additive plumbing — nommsubcommand consumes it yet (commands land in a follow-up), so there is no user-visible behavior change.socket-line—writeLine/readLinefor newline-delimited framing over anet.Socket, with an optional read timeout and listener cleanup.daemon-client—sendCommandopens a connection, writes one JSON-RPC request, reads one response, and closes; it correlates the responseidwith the request and retries once on transient connection errors (ECONNREFUSED/ECONNRESET).pingDaemonis a lightweightgetStatushealth probe that distinguishes "no daemon" (absent) from "daemon present but unreachable", classifying the latter by failure mode (refused/timeout/permission/protocol/other).rpc-socket-server—startRpcSocketServerlistens on a Unix socket and dispatches one JSON-RPC request per connection to a handler map. It intercepts a built-inshutdownmethod, enforces one-request-per-connection, times out idle connections, and returns aclose()handle.daemon-spawn—ensureDaemonspawns a detached daemon process and polls until the socket is responsive, refusing to take over a wedged or foreign-owned socket. Resolves the entry point fromdist(prod) orsrcviatsx(dev).stop-daemon—stopDaemonescalates from a gracefulshutdownRPC throughSIGTERMtoSIGKILL, then removes the PID and socket files best-effort.prompts—confirmPurgewraps the ESM-only@inquirer/confirmvia dynamic import.types— addsRpcHandler,RpcHandlerMap,DaemonStatusInfo, andDaemonSpawnConfig.Adds
@inquirer/confirmand@metamask/rpc-errorsdependencies. Every daemon module is covered to the package's 100% coverage thresholds, plus asocket-integration.test.tsthat exercises the client and server together over a real Unix socket (framing, id correlation, the one-request-per-connection invariant, and real shutdown timing).Hardening applied on top of the original branch
A multi-agent review of the ported code surfaced a few issues worth fixing before any command consumes this transport. These are the only deviations from the original
rekm/wallet-climodules (otherwise lifted verbatim):rpc-socket-server.ts) —listen()created the Unix socket with umask-derived (typically world-accessible) permissions. Since the daemon hosts an unlocked wallet, any local user could connect and drive it. The server nowchmods the socket to0600immediately after binding.daemon-spawn.ts) — aChildProcess'error'(bad interpreter,EACCES,ENOENT) was logged to stderr but never recorded, so the readiness loop polled for the full 30s and then threw a generic timeout, discarding the real cause. The error is now captured and surfaced immediately.'error'during a write (socket-line.ts) —writeLinerelied solely on the write callback; a socket'error'arriving mid-write had no listener and would crash the process instead of rejecting. It now attaches a one-shot'error'listener for the duration of the write.socket-integration.test.ts) — replaced anawait import('node:net')with a static import so the file runs standalone without--experimental-vm-modules.References
rekm/wallet-cli, feat(wallet-cli): Add wallet CLI with daemon support #8446). They are lifted essentially verbatim from there, split out into this standalone, additive PR and adjusted to apply on the currentmain(see the hardening deltas above).Checklist
🤖 Generated with Claude Code
Note
Medium Risk
Touches local IPC and process lifecycle around an unlocked-wallet daemon, including owner-only socket permissions and signal-based stop; additive only with no CLI wiring yet, but mistakes could affect security or orphan processes once commands land.
Overview
Adds additive daemon IPC plumbing to
@metamask/wallet-cliso the CLI can talk to a background wallet daemon over a Unix socket. Nothing in this PR wires it intommsubcommands yet.The stack is newline-delimited JSON-RPC:
socket-linefor framing,daemon-client(sendCommandwith request/response id checks and one retry on transient connect errors;pingDaemonwithabsent/responsive/ categorizedunreachable), andstartRpcSocketServer(one request per connection, handler map, built-inshutdown, idle timeouts).ensureDaemonspawns a detached process and polls readiness while refusing to replace a wedged or other-user socket;stopDaemonescalates shutdown RPC → SIGTERM → SIGKILL and cleans up PID/socket files. Sharedutilscover PID files and signaling;confirmPurgedynamically imports@inquirer/confirm.types.tsgains RPC and spawn config types.Security / robustness hardening in this PR: socket files are
chmod0600 after bind (wallet daemon must not be world-connectable), failed childspawnerrors fail fast instead of a 30s timeout, andwriteLinehandles mid-write socket errors. Dependencies@metamask/rpc-errorsand@inquirer/confirmare added; changelog documents the transport layer. Broad unit tests plussocket-integration.test.tsexercise real sockets end-to-end.Reviewed by Cursor Bugbot for commit 556cbd7. Bugbot is set up for automated code reviews on this repo. Configure here.