Skip to content

refactor(wallet-cli): enforce RPC param validation at dispatch time via superstruct - #8846

Merged
sirtimid merged 13 commits into
mainfrom
sirtimid/wallet-cli-rpc-handler-structs
Jul 14, 2026
Merged

refactor(wallet-cli): enforce RPC param validation at dispatch time via superstruct#8846
sirtimid merged 13 commits into
mainfrom
sirtimid/wallet-cli-rpc-handler-structs

Conversation

@sirtimid

@sirtimid sirtimid commented May 18, 2026

Copy link
Copy Markdown
Member

Summary

Resolves #8777.

  • Adds RpcHandler<TParams, TResult>, RpcHandlerDefinition<TParams, TResult> ({ paramsStruct, run }), and a defineHandler(paramsStruct, run) helper in daemon/types.ts. RpcHandlerMap now maps method names to definitions.
  • rpc-socket-server.ts validates params against the handler's struct via superstruct.validate before dispatch and returns -32602 invalidParams on shape mismatch. Each handler body can now trust the shape of its argument.
  • daemon-entry.ts:
    • getStatus uses literal(null).
    • call uses a defined struct that enforces a non-empty [string, ...Json[]] tuple, dropping the ad-hoc Array.isArray / typeof checks from the handler body.
    • Replaces the wallet.messenger.call as any cast with a single typed RpcDispatcher = messenger.call.bind(messenger) escape hatch.
  • Adds @metamask/superstruct ^3.1.0 to wallet-cli (already used across the monorepo at this version).

Test plan

  • yarn workspace @metamask/wallet-cli run test — 248 tests pass, 100% line/branch/function/statement coverage on the touched files.
  • yarn lint:eslint packages/wallet-cli/src — clean.
  • yarn constraints — clean.
  • yarn workspace @metamask/wallet-cli run changelog:validate — clean.
  • New server test covers the -32602 invalidParams path and verifies run is not invoked when the struct rejects.
  • New struct tests on the call handler cover both the rejection cases (null, empty, non-string head, non-array) and the accepting case.

🤖 Generated with Claude Code


Note

Medium Risk
Changes the wallet daemon JSON-RPC dispatch path and how malformed params are surfaced (standard invalidParams vs handler errors), on a local socket that still exposes the full messenger surface.

Overview
Daemon RPC handlers are now { paramsStruct, run } definitions (via defineHandler) instead of raw functions. rpc-socket-server validates params with superstruct before calling run and responds with -32602 invalidParams when the shape does not match, so handlers no longer need ad-hoc checks.

daemon-entry wires getStatus / listActions with literal(null), call with a struct for a non-empty [string, ...] action tuple, and routes messenger dispatch through a single RpcDispatcher bind instead of inline casts. Shutdown now logs and continues if dispose() fails during teardown.

Adds @metamask/superstruct and expands tests for invalid-params paths and struct acceptance/rejection.

Reviewed by Cursor Bugbot for commit 0c971a5. Bugbot is set up for automated code reviews on this repo. Configure here.

@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-rpc-handler-structs branch from 17d10e8 to c591c6e Compare July 1, 2026 11:31
@sirtimid
sirtimid changed the base branch from rekm/wallet-cli to sirtimid/wallet-cli-daemon-e2e July 1, 2026 11:31
@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-daemon-e2e branch 2 times, most recently from 9b0489c to 56d3cf9 Compare July 1, 2026 14:02
Base automatically changed from sirtimid/wallet-cli-daemon-e2e to main July 1, 2026 14:11
@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-rpc-handler-structs branch from c591c6e to 6cf1fde Compare July 2, 2026 09:56
@sirtimid
sirtimid marked this pull request as ready for review July 2, 2026 10:12
@sirtimid
sirtimid requested review from a team as code owners July 2, 2026 10:12
@sirtimid
sirtimid temporarily deployed to default-branch July 2, 2026 10:12 — with GitHub Actions Inactive
@sirtimid sirtimid changed the title refactor(wallet-cli): Parameterise RpcHandler with struct-validated dispatch refactor(wallet-cli): enforce RPC param validation at dispatch time via superstruct Jul 2, 2026
sirtimid and others added 12 commits July 13, 2026 14:33
Add an end-to-end suite that spawns the built `mm` CLI as real child
processes and drives the full daemon lifecycle over the Unix socket —
the gap left by the in-process suites (`socket-integration.test.ts`
exercises the transport in-realm; `wallet-factory.e2e.test.ts`
constructs a `Wallet` in-process). Covers: `start` (and the
already-running guard on a second `start`), `call` returning the
SRP-derived account, `status`, `stop`, persistence across a restart
(the resume path: the wallet comes back locked rather than re-importing
the SRP), `purge`, and the owner-only socket (0600) / data dir (0700).

Because it needs the built `dist/` and the native better-sqlite3 addon,
it runs as its own jest project (`jest.config.e2e.js`) via a new
`test:e2e` script and is excluded from the fast unit `test` run and its
100%-coverage gate. A dedicated `test-wallet-cli-e2e` CI job (Node 20.x
and 24.x) builds the dependency subtree and runs it; the per-package
`test-*` matrix can't host it because it runs against source with no
build.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename `fd` to `file descriptor` throughout the daemon spawn code and tests
- Narrow `stopDaemon` stale-cleanup to `absent`/`refused` sockets only: a
  `permission`/`timeout`/`protocol` socket may be a wedged or foreign-user
  daemon, so it is no longer deleted or reported as a successful stop
- Add a compile-time exhaustiveness guard to `ensureDaemon`'s ping handling so
  spawning is reachable only for a positive `absent` result
- Replace the two mutable `{ value: T | null }` boxes with a single
  `StartupOutcome` discriminated union
- Simplify/de-duplicate daemon comments; use `as unknown as ChildProcess` in
  the spawn mocks; add tests for the new branches

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Generalize the e2e jest config to discriminate by folder instead of the
feature-specific `daemon-e2e` filename suffix, so future subprocess e2e tests
just drop into `tests/` with no config change.

- jest.config.e2e.js: match `roots: ['<rootDir>/tests']` instead of
  `**/*.daemon-e2e.test.ts`
- jest.config.js: ignore `<rootDir>/tests/`; drop the redundant daemon-e2e
  coverage exclusion (coverage is collected from `./src/**` only)
- Rename lifecycle.daemon-e2e.test.ts -> lifecycle.e2e.test.ts
- README: describe the suite by folder

The in-process `wallet-factory.e2e.test.ts` stays in `src/` (unit suite): it
needs the Web-Crypto polyfill env and is coverage-visible, unlike the
subprocess suite.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ispatch

Adds a `defineHandler(paramsStruct, run)` helper plus a generic
`RpcHandler<TParams, TResult>` / `RpcHandlerDefinition` type so each daemon
RPC method owns its params struct. `rpc-socket-server` now validates params
via `superstruct.validate` once per request and returns
`-32602 invalidParams` on shape mismatch, so handler bodies can trust their
input. Rewrites `getStatus` and `call` against the new shape and replaces
the `wallet.messenger.call as any` cast with a single labelled
`RpcDispatcher` bind. Tests updated; coverage stays at 100%.

Closes #8777.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove duplicate `test-wallet-cli-e2e` CI job (stale v2 entry merged in),
duplicate `testPathIgnorePatterns` key in jest.config.js, and stale
eslint-suppressions entry removed by lint:fix.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Guard activeDispose() in shutdown closure with try-catch + log; a
  throwing dispose during SIGTERM/SIGINT previously skipped PID/socket
  cleanup silently
- Change callParamsStruct type annotation from Json[] to unknown[] for
  the tail elements, matching what the runtime validator actually checks
- Simplify RpcDispatcher return type to Promise<Json> (always awaited)
- Fix "narrow" → "consolidate the unsafe cast to" in RpcDispatcher JSDoc
- Fix missed handlerDefinition() migration in socket-integration test
- Reduce asHandler/handlerDefinition JSDoc blocks to single inline
  comments
- Add tests: getStatus/listActions struct rejection, params-absent
  → -32602, log-on-handler-throw, dispose-error-during-shutdown

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-rpc-handler-structs branch from 6dcb9c9 to 99e87a9 Compare July 13, 2026 12:33

@rekmarks rekmarks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Just one thing.

Comment thread packages/wallet-cli/src/daemon/types.ts
@rekmarks

Copy link
Copy Markdown
Member

@sirtimid actually, since we would have returned different errors prior to this work, we should add a changelog entry for the package.

… in RpcHandlerMap

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sirtimid
sirtimid requested a review from rekmarks July 14, 2026 13:43
@sirtimid
sirtimid enabled auto-merge July 14, 2026 14:00
@sirtimid
sirtimid added this pull request to the merge queue Jul 14, 2026
Merged via the queue into main with commit 388878c Jul 14, 2026
421 checks passed
@sirtimid
sirtimid deleted the sirtimid/wallet-cli-rpc-handler-structs branch July 14, 2026 16:06
sirtimid added a commit that referenced this pull request Jul 14, 2026
Reconcile the auto-unlock (optional `--password`) work on this branch with
main's opaque `Password`/`Srp` daemon-secret types (#8863) and dispatch-time
RPC param validation (#8846):

- `DaemonSpawnConfig`/`CreateWalletConfig` password is now `password?: Password`
  (optional) with `srp: Srp`.
- daemon-entry wraps the env password only when present
  (`passwordRaw ? Password.from(passwordRaw) : undefined`); empty/absent stays
  "start locked".
- daemon-spawn unwraps `Password`/`Srp` at the child-env boundary while keeping
  the strip-inherited-password logic for the no-password case.
- wallet-factory drops the now-redundant `blankToUndefined` (opaque `Password`
  is validated non-empty at construction) and unwraps at the keyring boundary.
- start command builds `password` as `Password | undefined` so `--password`
  stays optional.
- Tests updated to opaque types; dropped the empty-string-password cases that
  can no longer reach `createWallet`. Old `wallet-factory-integration.test.ts`
  removed in favour of this branch's renamed `wallet-factory.integration.test.ts`.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wallet-cli: Parameterise RpcHandler with struct-validated dispatch

2 participants