fix(wallet-cli): Auto-unlock keyring on daemon restart + add mm wallet unlock#8821
Draft
sirtimid wants to merge 3 commits into
Draft
fix(wallet-cli): Auto-unlock keyring on daemon restart + add mm wallet unlock#8821sirtimid wants to merge 3 commits into
mm wallet unlock#8821sirtimid wants to merge 3 commits into
Conversation
rekm/wallet-cli is behind main on the keyring-controller package by two releases (25.4.0, 25.5.0). This brings the keyring-controller src, package.json, and CHANGELOG into byte-identical alignment with main (verified via `git diff main` returning empty for this directory) so the reconciliation disappears as a clean no-op when rekm/wallet-cli is eventually rebased onto main. This is a prerequisite for the companion wallet-cli changes in this PR: `KeyringController:submitPassword` (exposed via the messenger on main in #8674) is what the daemon auto-unlock and `mm wallet unlock` command dispatch through. Without this mirror the integration tests added in the next commit would fail with "A handler for KeyringController:submitPassword has not been delegated to Wallet". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Today the daemon imports the SRP on first run (which leaves the keyring unlocked) but on subsequent runs only hydrates state from `<dataDir>/wallet.db` and constructs the Wallet. The persisted KeyringController vault is reused, but `KeyringController.isUnlocked` is `persist: false` and defaults back to `false`, so any messenger action touching keyring-bound state (signing, `AccountsController:listAccounts`, etc.) failed after a daemon restart even though the password was already supplied via `--password` / `MM_WALLET_PASSWORD`. Changes: - `wallet-factory.ts` now calls `KeyringController:submitPassword` on the subsequent-run branch when a password is supplied, unlocking the keyring before returning. Wrong-password rejections surface as the rejection from `submitPassword`; the existing `catch` destroys the partial wallet and closes the store. First-run + no-password is rejected with a clear error *before* the Wallet is constructed, so a doomed startup doesn't build then tear down a Wallet. - `--password` / `MM_WALLET_PASSWORD` are now optional on `mm daemon start`. On subsequent runs, omitting them leaves the keyring locked; the companion `mm wallet unlock` command (next commit) is the affordance to unlock later. First-run still requires a password (enforced by `wallet-factory.ts`). - Empty-string password is normalised to `undefined` at the wallet-factory boundary so `--password ''` is treated as "no password supplied" rather than "the empty string is the password" (which the controller would reject as wrong). - `daemon-spawn.ts` only forwards `MM_WALLET_PASSWORD` to the child env when the caller explicitly supplied one; it deletes the variable from the spread otherwise so a stray inherited value doesn't override an explicit omission. Tests: - Unit tests added in `wallet-factory.test.ts` covering all four branches (first-run +/- password, subsequent +/- password), the empty-string normalisation, and the wrong-password cleanup path. - New `wallet-factory.integration.test.ts` (no mocks of `@metamask/wallet`) covers the full first-run -> destroy -> restart lifecycle, the auto-unlock path, the locked-then-unlock-via- submitPassword path, and that a wrong password rejects without corrupting the DB (a retry with the right password succeeds). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Companion to the auto-unlock fix in the previous commit. With `--password` now optional on `mm daemon start`, a daemon can be started with the keyring locked; this command is the affordance to unlock it later. Also covers the general case of a daemon that was unlocked, then locked via `mm daemon call KeyringController:setLocked`. `packages/wallet-cli/src/commands/wallet/unlock.ts` Dispatches `KeyringController:submitPassword` over the daemon socket via `sendCommand`. `--password` flag is optional with `MM_WALLET_PASSWORD` env fallback; when neither is supplied, the command prompts interactively via the new `promptPassword` helper (masked input). Empty `--password ''` is treated as "no flag supplied" so the prompt fires instead of sending an empty string the controller would reject. Errors surface with friendly messages: ENOENT/ECONNREFUSED -> "Daemon is not running" hint; EACCES -> permission-denied hint with an `MM_DAEMON_DATA_DIR` pointer; JSON-RPC failures -> "Failed to unlock: <msg> (code <n>) data=<json>". `packages/wallet-cli/src/daemon/prompts.ts` New `promptPassword` helper using the same dynamic-import + ESM-interop pattern as the existing `confirmPurge`. Adds `@inquirer/password@^4.0.16` as a dependency. Placement under the new `wallet` oclif topic (rather than `daemon`) because unlocking is a wallet/keyring-lifecycle operation, not a daemon-lifecycle one. oclif auto-registers the topic from the directory structure; no extra config needed. Tests (14 cases) cover: happy path, env-var fallback, interactive prompt when neither flag nor env is supplied, prompt when `--password ''` is supplied, ENOENT/ECONNREFUSED/EACCES/other socket errors, JSON-RPC failure rendering (including the `data` field), non-Error throws, timeout flag, idempotent re-unlock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution MetaMask internal reviewing guidelines:
|
sirtimid
changed the title
mm wallet unlock (#8776, #8780)mm wallet unlock
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the "daemon restart leaves wallet locked" gap from PR #8446, resolving #8776 (auto-unlock on subsequent daemon starts) and #8780 (
mm wallet unlockcommand). Together these are the two halves of the same fix.Targets
rekm/wallet-cli— not for core-team review yet per the wallet-library landing plan. Posting as draft so accounts-engineers/core-platform aren't auto-pinged by CODEOWNERS.Commit-by-commit
chore(keyring-controller): Mirror state from main onto rekm/wallet-cli—rekm/wallet-cliwas two releases behind main on keyring-controller. Brings the package's src + package.json + CHANGELOG to byte-identical alignment with main (git diff mainforpackages/keyring-controller/returns empty). Required becauseKeyringController:submitPassword(exposed via the messenger on main in chore: Expose missingKeyringControllermethods through the messenger #8674) is what the next two commits dispatch through. Disappears as a no-op when this branch eventually rebases onto main.fix(wallet-cli): Auto-unlock keyring on subsequent daemon starts (#8776)— On the not-first-run branch,wallet-factory.tsnow callsKeyringController:submitPasswordto unlock the persisted vault before returning.--password/MM_WALLET_PASSWORDare now optional onmm daemon start; first-run still requires them (enforced before Wallet construction so a doomed startup doesn't build then tear down a Wallet). Empty-string password is normalised to "no password" at the boundary so--password ''doesn't get sent to the controller as a wrong password.feat(wallet-cli): Add 'mm wallet unlock' command (#8780)— New oclif command under thewallettopic. Optional--passwordflag withMM_WALLET_PASSWORDenv fallback; prompts interactively via@inquirer/passwordwhen neither is supplied. Empty--password ''triggers the prompt. ENOENT/ECONNREFUSED → "Daemon is not running"; EACCES → permission-denied hint withMM_DAEMON_DATA_DIRpointer; JSON-RPC failure →Failed to unlock: <msg> (code <n>) data=<json>. Placed underwalletnotdaemonbecause unlocking is a wallet/keyring-lifecycle operation.Test plan
wallet-factory.ts,unlock.ts,daemon-spawn.ts,daemon-entry.ts,prompts.ts.@metamask/walletmocks) — 4 cases covering: first-run SRP import + listAccounts; auto-unlock on restart with password; restart without password → locked → unlock viasubmitPassword→ listAccounts works; wrong password rejects without corrupting the DB (retry with correct password succeeds). These caught a real bug during review: the issue's original proposed fix would have failed becausesubmitPasswordwasn't messenger-exposed on this branch's base (motivating the keyring-controller mirror in commit 1).yarn lint:eslintclean for both packages.yarn lint:misc:checkclean.yarn changelog:validateclean.mm daemon start --password X, kill it,mm daemon start --password X,mm daemon call AccountsController:listAccountsreturns accounts.mm daemon start(no password, after first-run),mm daemon call AccountsController:listAccountsfails,mm wallet unlock --password X,mm daemon call AccountsController:listAccountsreturns accounts.mm wallet unlockwith no flag and no env var prompts interactively.Known limitations (deferred technical debt)
yarn constraintsreports 24 violations: consumers of@metamask/keyring-controllerstill reference^25.2.0while this branch's keyring-controller is now^25.5.0. Main's consumers already reference^25.5.0, so these resolve automatically whenrekm/wallet-clieventually rebases onto main. Bundling those 24 package.json bumps into this PR was rejected as scope creep — the fix is "rebase onto main" not "ship 24 bumps in a feature PR".yarn workspace @metamask/wallet-cli run build:allfails with a pre-existing ts-bridge--clean+ transitive-deps ordering bug (same failure on@metamask/wallet's ownbuild:allon this branch). Rootyarn buildworks; workspaceyarn build(without:all) works. Not introduced by this PR.🤖 Generated with Claude Code