Repository navigation
feat(wallet-cli): nested command grammar, structured output, confirmations, wizards, exec mode and a read-only mcp server - #2147
nullPointerEnjoyer wants to merge 52 commits into
Conversation
|
Addressed the latest review round (commit 066840f):
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if std::io::stdin().is_terminal() { | ||
| label_password_errors( | ||
| tokio::task::spawn_blocking(move || prompt_password_hidden(prompt_mode)), | ||
| "Failed to read the password from the terminal".to_owned(), | ||
| ) | ||
| .await? | ||
| } else { |
There was a problem hiding this comment.
The hidden prompt is gated on std::io::stdin().is_terminal(), but rpassword::prompt_password prompts on the terminal. In environments where stdin is not a tty yet a controlling terminal exists (wrappers, GUI workers spawning the CLI), the user gets an error telling them interactive input requires a terminal on stdin even though one could serve the prompt — and conversely a caller driving the process over pipes can never use the prompt path. Consider gating on the tty the prompt will actually use (or document the stdin-tty requirement prominently), since this decides between a hanging prompt, a clear error, and an unusable wallet encrypt/unlock.
There was a problem hiding this comment.
Keeping the stdin gate as is, with the reasoning documented on prompt_password_hidden: the hidden prompt itself reads from the controlling terminal, but gating on stdin keeps piped automation (commands files, --exec) failing fast with an explicit error instead of hanging on an unseen prompt. Happy to revisit if a real GUI-worker use case shows up.
| let mut password = String::new(); | ||
| file.read_to_string(&mut password)?; | ||
| Ok(password) |
There was a problem hiding this comment.
The whole password file is read into memory with no size cap, and only trailing line terminators are stripped: a file saved with a trailing space, or a CRLF split so a stray \r lands mid-string, silently produces a wrong password that encrypts the wallet irrecoverably (no way to detect until unlock fails). Consider capping the file size (e.g. a few KiB, rejecting larger files as a likely wrong-file mistake) and rejecting passwords containing embedded whitespace or NUL bytes, or at least warning when the stripped password differs non-trivially from the raw content.
There was a problem hiding this comment.
Fixed: password files are now rejected when they are larger than 4 KiB, contain a null byte, contain more than one line, or are empty, and the read itself is bounded with Read::take so a file that grows (or a special file that reports length 0) cannot make the read unbounded. Note this is a deliberate compatibility break: a password file with an interior line terminator used to yield a multi-line password and now fails fast — a wallet encrypted that way must be unlocked with the original file before upgrading.
| startup_command_futures: Vec<oneshot::Receiver<Result<ConsoleCommand, WalletCliError<N>>>>, | ||
| ) -> Result<(), WalletCliError<N>> { | ||
| for res_rx in startup_command_futures { | ||
| let res = res_rx.blocking_recv().expect("Channel must be open")?; |
There was a problem hiding this comment.
The startup-command path panics via blocking_recv().expect("Channel must be open") if the event loop shuts down early, while the new exec path (repl/exec.rs) handles the identical failure gracefully with WalletCliError::EventLoopShutdown. For --commands-file runs this converts an early event-loop shutdown into a panic instead of a clean non-zero exit, undermining the machine-parseable exit-status contract this change introduces. Reuse the same error mapping here.
Suggestion:
| let res = res_rx.blocking_recv().expect("Channel must be open")?; | |
| let res = match res_rx.blocking_recv() { | |
| Ok(inner) => inner?, | |
| Err(_recv_error) => { | |
| return Err(WalletCliError::<N>::EventLoopShutdown( | |
| "startup commands did not finish".to_owned(), | |
| )); | |
| } | |
| }; |
There was a problem hiding this comment.
Fixed: the startup loop now maps a closed channel to WalletCliError::EventLoopShutdown instead of panicking, matching the exec path. The shared run_command_blocking helper got the same treatment.
| } else { | ||
| unreachable!("clap's `auth` arg group guarantees exactly one auth source") | ||
| } |
There was a problem hiding this comment.
auth_data() ends in unreachable!() with the safety argument living in clap's ArgGroup configuration. If a future auth option is added to the required group but not to this if-chain, the binary panics at startup instead of failing gracefully. Prefer returning a Result (clap's group ensures it is practically always Ok) or restructuring as a match over a single parsed enum built in the argument parser.
Suggestion:
| } else { | |
| unreachable!("clap's `auth` arg group guarantees exactly one auth source") | |
| } | |
| } else { | |
| return Err(anyhow::anyhow!( | |
| "no authentication source configured; specify --rpc-cookie-file, --rpc-username/--rpc-password, or --rpc-no-auth" | |
| )); | |
| } |
There was a problem hiding this comment.
Fixed: auth_data() now returns Result<RpcAuthData, String>; the impossible all-absent case exits with a message instead of an unreachable! panic.
| let info = WalletInterface::wallet_info(&self.0).await.map_err(|err| err.to_string())?; | ||
| serde_json::to_value(&info).map_err(|err| err.to_string()) |
There was a problem hiding this comment.
All adapter methods collapse typed wallet-RPC errors to strings via map_err(|err| err.to_string()) before they reach the MCP layer. The original error type (and any structure it carries, e.g. status codes) is lost, and the raw daemon message is surfaced verbatim to the LLM-facing tool result. Consider wrapping with context (e.g. format!("wallet RPC failed: {err}")) or preserving the typed error in the trait's associated error type, so failures stay actionable and internal details can be filtered at the boundary.
Suggestion:
| let info = WalletInterface::wallet_info(&self.0).await.map_err(|err| err.to_string())?; | |
| serde_json::to_value(&info).map_err(|err| err.to_string()) | |
| let info = WalletInterface::wallet_info(&self.0) | |
| .await | |
| .map_err(|err| format!("wallet RPC wallet_info failed: {err}"))?; | |
| serde_json::to_value(&info).map_err(|err| format!("failed to serialize wallet_info: {err}")) |
There was a problem hiding this comment.
Fixed: the adapter labels failures through a shared rpc_error helper (wallet RPC failed: {err}) so tool results name their provenance. A typed error hierarchy would ripple through the read-only trait for little gain at this surface.
| ColdWalletCommand::EncryptPrivateKeys { password } => { | ||
| logging::log::warn!( | ||
| "The password was passed on the command line, which is insecure; \ | ||
| prefer `wallet encrypt --password-file <file>`" | ||
| ); |
There was a problem hiding this comment.
The password read from the file/stdin/terminal is kept as a plain String for the rest of the command and is never zeroized after it is passed to encrypt_private_keys/unlock_private_keys. Since the file-based path was introduced precisely to reduce secret exposure, consider using zeroize::Zeroizing (or dropping the buffer immediately after use) so copies do not linger in freed heap memory. Relatedly, the log::warn! added to the legacy EncryptPrivateKeys/UnlockPrivateKeys handlers may not surface on a default console configuration, leaving users unaware the positional password is visible in shell history and ps; a ConsoleCommand::Warn (the new variant) would route it through the user-visible output channel.
Suggestion:
| ColdWalletCommand::EncryptPrivateKeys { password } => { | |
| logging::log::warn!( | |
| "The password was passed on the command line, which is insecure; \ | |
| prefer `wallet encrypt --password-file <file>`" | |
| ); | |
| ColdWalletCommand::EncryptPrivateKeys { password } => { | |
| // Route through ConsoleCommand::Warn so the user actually sees it. | |
| return Ok(ConsoleCommand::Warn( | |
| "The password was passed on the command line, which is insecure; \ | |
| prefer `wallet encrypt --password-file <file>`." | |
| .to_owned(), | |
| )); |
There was a problem hiding this comment.
Fixed: the password read path now returns Zeroizing<String> from every source (terminal prompt, stdin, file), including the confirmation entry, so all intermediate buffers wipe on every exit path. The one remaining plain copy is the clap String grammar field; it is noted at the dispatch site.
| let best_block_timestamp = if timestamp || show_all { | ||
| Some(format_block_timestamp( | ||
| wallet.chainstate_info().await?.best_block_timestamp, | ||
| )) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
When a subset of flags is requested, the id and height come from the node RPCs while the timestamp comes from chainstate_info(). If more than one flag is passed (labeled output), the values can straddle a block boundary and present an id/height/timestamp tuple that never existed as a consistent best-block state, which scripts may consume as atomic. Either document the non-atomicity in the command help, or fetch all three values from the same source when more than one is requested.
There was a problem hiding this comment.
Fixed: when more than one flag (or no flag) is passed, all displayed values now come from a single chainstate_info snapshot, so the printed tuple describes one real block state. Single-flag runs keep the legacy per-value RPCs for error parity, and the snapshot path first probes the node RPCs so a cold wallet still fails with "Method is not available in cold wallet mode" instead of printing fabricated genesis values. The single-source-of-truth test gap is noted with a TODO on the arm.
| Err(WizardError::Io(err)) => Err(WalletCliError::InvalidInput(format!( | ||
| "the wizard could not read the answer: {err}" | ||
| ))), |
There was a problem hiding this comment.
The wizard interaction and the confirmation prompt bypass the ConsoleOutput abstraction and the reedline external printer by using println! and dialoguer directly. This can interleave badly with the line editor's rendering (e.g. after the external printer has queued output) and, when stdin is not a TTY, Confirm::interact() fails with an io error that is then flattened into a generic InvalidInput string, discarding the original error kind. Consider routing output through console and preserving the io error (e.g. a dedicated WalletCliError variant).
Suggestion:
| Err(WizardError::Io(err)) => Err(WalletCliError::InvalidInput(format!( | |
| "the wizard could not read the answer: {err}" | |
| ))), | |
| Err(WizardError::Io(err)) => Err(WalletCliError::Io(err)), // keep the original error kind for non-TTY diagnosis |
There was a problem hiding this comment.
Partially fixed: the io error is now preserved as a typed WalletCliError::TerminalIo variant (both for the wizard backend and the dialoguer confirm), so non-TTY contexts get the precise underlying message instead of a generic "invalid input" string. The println!/dialoguer output routing is kept as is for now: both run in the interactive REPL where the line editor is not active during the prompt, and dialoguer needs direct terminal control for its hidden/confirm prompts.
| debug_assert!( | ||
| root.get_arguments().any(|arg| arg.get_long() == Some("help")), | ||
| "a shared clap Command must already be built" | ||
| ); |
There was a problem hiding this comment.
The completion candidates depend on the shared Arc<Command> having been built, but in release builds the debug_assert! is compiled out: if a caller hands over an unbuilt shared Arc, complete() silently returns no candidates with no signal. The help-flag-presence heuristic is also a proxy, not a guarantee. Consider returning a Result (or logging once) instead of relying on a debug-only assertion for a silent-failure mode.
Suggestion:
| debug_assert!( | |
| root.get_arguments().any(|arg| arg.get_long() == Some("help")), | |
| "a shared clap Command must already be built" | |
| ); | |
| if !root.get_arguments().any(|arg| arg.get_long() == Some("help")) { | |
| debug_assert!(false, "a shared clap Command must already be built"); | |
| log::warn!("WalletCompletions received an unbuilt shared clap Command; completions will be empty"); | |
| } |
There was a problem hiding this comment.
Fixed: in release builds the unbuilt-tree case now also emits a log::warn! ("completions will be empty") next to the debug assertion, so a miswired shared Arc<Command> is diagnosable instead of silently producing no candidates.
| let normalized = | ||
| crate::command_handler::normalize_nested_wallet_command(command.clone()); | ||
| wallet_confirmation(&normalized) |
There was a problem hiding this comment.
This clones the entire ManageableWalletCommand (including potentially large payloads such as hex-encoded transactions, utxo lists, and block data) on every state-changing command just to classify it for the confirmation prompt. normalize_nested_wallet_command consumes the value only to rebuild a nearly identical one; consider adding a by-reference translation helper (returning a borrowed flat view, a Cow, or only the classification-relevant discriminant) so the hot REPL/GUI path avoids a full deep copy per command.
Suggestion:
| let normalized = | |
| crate::command_handler::normalize_nested_wallet_command(command.clone()); | |
| wallet_confirmation(&normalized) | |
| let normalized = | |
| crate::command_handler::normalize_nested_wallet_command_ref(command); | |
| wallet_confirmation(&normalized) |
There was a problem hiding this comment.
Fixed: flat spellings are classified by reference; only nested ones are normalized (the clone). The per-call rebuild is gone.
| std::thread::Builder::new() | ||
| .stack_size(LARGE_STACK_SIZE) | ||
| .spawn(task) | ||
| .expect("failed to spawn the grammar thread") | ||
| .join() | ||
| .unwrap_or_else(|payload| { | ||
| let message = payload | ||
| .downcast_ref::<&str>() | ||
| .copied() | ||
| .or_else(|| payload.downcast_ref::<String>().map(String::as_str)) | ||
| .unwrap_or("<non-string panic payload>"); | ||
| panic!("the grammar thread panicked: {message}") | ||
| }) |
There was a problem hiding this comment.
This spawns a fresh thread with a 64 MiB stack for every call, and callers in wallet-cli-lib (repl/exec.rs, repl/mod.rs) invoke it once per parsed command in the REPL loop. For parse-heavy loops this is a per-command thread spawn of a very large stack. Consider caching the built grammar (build once, reuse the Command) or reusing a dedicated grammar thread, and lowering the stack size if feasible. Additionally, the join handler converts a worker panic into a new panic!, which drops the original panic location/backtrace; using resume_unwind on the payload would preserve it.
Suggestion:
| std::thread::Builder::new() | |
| .stack_size(LARGE_STACK_SIZE) | |
| .spawn(task) | |
| .expect("failed to spawn the grammar thread") | |
| .join() | |
| .unwrap_or_else(|payload| { | |
| let message = payload | |
| .downcast_ref::<&str>() | |
| .copied() | |
| .or_else(|| payload.downcast_ref::<String>().map(String::as_str)) | |
| .unwrap_or("<non-string panic payload>"); | |
| panic!("the grammar thread panicked: {message}") | |
| }) | |
| std::thread::Builder::new() | |
| .stack_size(LARGE_STACK_SIZE) | |
| .spawn(task) | |
| .expect("failed to spawn the grammar thread") | |
| .join() | |
| .unwrap_or_else(|payload| std::panic::resume_unwind(payload)) |
There was a problem hiding this comment.
Applied the resume_unwind suggestion so the original panic payload, location, and backtrace survive. Declining the grammar caching for now: callers already build the tree once and reuse it; only the per-line parse runs on the big-stack thread, which is a single ~100µs thread spawn against a human-paced REPL (or one-shot exec), so the churn of a persistent grammar worker does not pay for itself here.
| (CliOutputFormat::Json, ExecOutput::Text(text)) => output.print_line( | ||
| &serde_json::to_string(&serde_json::json!({ "output": text })) | ||
| .map_err(WalletCliError::SerdeJsonFormatError)?, | ||
| ), |
There was a problem hiding this comment.
With --output json, the success payload shape is inconsistent: migrated commands (ConsoleCommand::Json) emit their raw value (or {"text", "value"}), while unmigrated text commands emit {"output": text}. Consumers cannot uniformly distinguish payload kinds, and failures produce no structured JSON at all (plain text on stderr + exit status 1). Consider a stable envelope such as {"status": "ok"|"error", "result": ...} or at least documenting the contract for each case.
There was a problem hiding this comment.
Documented rather than redesigned for now: run() in repl/exec.rs now spells out the --output json contract (migrated commands print their raw value or {"text", "value"}; text commands print {"output": ...}; failures are plain text on stderr with a non-zero exit). A uniform {status, result} envelope would be a breaking change to the shape early adopters already consume — better suited to the json-hardening pass planned for the next release train.
| None => return Ok(LineOutput::None), | ||
| }; | ||
|
|
||
| super::confirmation_gate(&command, assume_yes, "non-interactive mode")?; |
There was a problem hiding this comment.
The new confirmation gate makes every state-changing command fail in --commands-file / piped-stdin runs unless --yes is passed (with exit_on_error defaulting to true, one gated command aborts the whole file). The in-repo harness was updated (wallet_cli_controller.py now adds --yes), but any external automation or user scripts relying on commands_file will silently start failing with 'Confirmation required'. Consider keeping the old behavior as the default for --commands-file, or at least documenting this as a breaking change / adding a deprecation note in the release notes.
There was a problem hiding this comment.
Keeping the gate as the default for --commands-file: the whole point is that a script that moves funds must opt in explicitly with --yes rather than silently proceeding when the wallet gains a new safety behavior. The in-repo functional harness passes --yes, and the --yes help text and the PR description call out the break for external automation. If this turns out to bite real users we can add a scoped escape hatch (e.g. --commands-file --assume-yes-legacy) in a follow-up.
| let mut password = String::new(); | ||
| file.take(MAX_PASSWORD_FILE_BYTES).read_to_string(&mut password)?; |
There was a problem hiding this comment.
The metadata size check and the file.take(MAX_PASSWORD_FILE_BYTES) read are separate steps, so a file that grows (or is swapped) between them can still be silently truncated mid-password by take, yielding a valid-looking prefix password. Reading with take(MAX + 1) and rejecting when the read exceeds MAX closes the race entirely without relying on metadata.len().
Suggestion:
| let mut password = String::new(); | |
| file.take(MAX_PASSWORD_FILE_BYTES).read_to_string(&mut password)?; | |
| let mut password = String::new(); | |
| file.take(MAX_PASSWORD_FILE_BYTES + 1) | |
| .read_to_string(&mut password)?; | |
| if password.len() as u64 > MAX_PASSWORD_FILE_BYTES { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::InvalidInput, | |
| "the password file is larger than 4 KiB; this does not \ | |
| look like a password file", | |
| )); | |
| } |
There was a problem hiding this comment.
Fixed: the read now takes MAX_PASSWORD_FILE_BYTES + 1 and rejects when more than the cap arrives, so a file that grows or is swapped after the metadata check fails closed instead of yielding a truncated prefix password. The metadata check stays as a fast path, and both size rejections share one error constructor.
| | WalletCommand::Node { .. } | ||
| | WalletCommand::Peer { .. } | ||
| ) | ||
| } |
There was a problem hiding this comment.
is_nested_wallet_command is missing WalletCommand::Config { .. } even though the dispatch or-pattern above explicitly includes it and both sites carry a "keep in sync" comment. The predicate is the safety net that turns translation drift into a compile/assert-detectable failure; leaving Config out weakens the guard the comment promises. Note Config also appears in the confirm.rs no-prompt or-pattern, so all three lists must stay aligned when groups are added.
Suggestion:
| | WalletCommand::Node { .. } | |
| | WalletCommand::Peer { .. } | |
| ) | |
| } | |
| | WalletCommand::Node { .. } | |
| | WalletCommand::Peer { .. } | |
| | WalletCommand::Config { .. } | |
| ) | |
| } |
There was a problem hiding this comment.
Fixed: WalletCommand::Config { .. } is now in the is_nested_wallet_command or-pattern, so the guard matches the dispatch list again.
| // dialoguer errors are io errors underneath; keep them typed so | ||
| // non-TTY contexts get a precise diagnosis instead of a generic | ||
| // "invalid input" message. | ||
| .map_err(|err| WalletCliError::TerminalIo(std::io::Error::other(err.to_string())))?; |
There was a problem hiding this comment.
In interactive mode, a confirmation prompt hit while stdin is not a TTY (e.g. piped input driving the "interactive" REPL) makes dialoguer fail and the whole REPL session abort with a TerminalIo error, instead of falling back to the safer non-interactive behavior (treat as declined/aborted). Also, wrapping the dialoguer error via std::io::Error::other(err.to_string()) stringifies it and discards the typed error source. Consider treating a failed prompt as false (abort) with a notice, or at least preserving the original error.
Suggestion:
| .map_err(|err| WalletCliError::TerminalIo(std::io::Error::other(err.to_string())))?; | |
| .map_err(|err| WalletCliError::TerminalIo(std::io::Error::other(err)))?; |
There was a problem hiding this comment.
Fixed: a failed prompt is now treated as a decline for that command ("treating as declined", then the usual "Aborted."), so a piped "interactive" session keeps running instead of being torn down. The match also dropped the variant pattern in favor of matching the whole dialoguer::Error, so upstream adding variants cannot break the build, and the printed message still comes from the error's Display.
| let balances = WalletInterface::get_balance( | ||
| &self.0, | ||
| account_index, | ||
| utxo_states, | ||
| with_locked_filter(with_locked), | ||
| ) | ||
| .await | ||
| .map_err(|err| err.to_string())?; |
There was a problem hiding this comment.
Every wallet RPC error is collapsed to a bare String via err.to_string() at this adapter boundary, and in lib.rs ToolCallError::Execution renders that string directly into the MCP tool result shown to the LLM client. RPC error types may embed connection details, URLs, or auth hints, so this both erases typed context needed for error handling/testing and can leak internals to the client. Consider mapping to a sanitized, categorized error at this boundary (e.g. an enum of user-facing failure kinds), or at minimum scrub/summarize the message before it is echoed into tool_result.
There was a problem hiding this comment.
Documented: the double-entry guard exists only on the interactive prompt; file and stdin sources are read exactly once, as noted on read_password_from_source.
| Some("-") => { | ||
| label_password_errors( | ||
| tokio::task::spawn_blocking(read_password_from_stdin), | ||
| "Failed to read the password from standard input".to_owned(), | ||
| ) | ||
| .await? | ||
| } |
There was a problem hiding this comment.
The function documentation states that in PromptMode::Create the password is asked twice as a typo guard, but that guard only exists in the hidden-prompt path (prompt_password_hidden). The --password-file - (stdin) and --password-file <file> paths in Create mode accept a single reading; a typo when piping the password for wallet encrypt will lock the wallet with an unknown password — exactly the failure mode the double-entry guard is meant to prevent. Consider echoing back nothing but requiring the stdin path to read the password twice in Create mode, or documenting the asymmetry.
Suggestion:
| Some("-") => { | |
| label_password_errors( | |
| tokio::task::spawn_blocking(read_password_from_stdin), | |
| "Failed to read the password from standard input".to_owned(), | |
| ) | |
| .await? | |
| } | |
| Some("-") => { | |
| let password = label_password_errors( | |
| tokio::task::spawn_blocking(read_password_from_stdin), | |
| "Failed to read the password from standard input".to_owned(), | |
| ) | |
| .await?; | |
| if prompt_mode == PromptMode::Create { | |
| // Re-read and compare so a typo in a piped password cannot lock the wallet. | |
| let confirmed = label_password_errors( | |
| tokio::task::spawn_blocking(read_password_from_stdin), | |
| "Failed to read the password confirmation from standard input".to_owned(), | |
| ) | |
| .await?; | |
| if password != confirmed { | |
| return Err(WalletCliCommandError::InvalidInput( | |
| "the two passwords do not match".to_owned(), | |
| )); | |
| } | |
| } | |
| password | |
| } |
There was a problem hiding this comment.
Fixed: shared rpc_error labeling at the adapter boundary.
| // The startup command output is the wallet opening report; keep it off | ||
| // standard output so the requested output format stays machine-parseable. | ||
| let line_out = to_startup_text(res); | ||
| eprintln!("{line_out}"); |
There was a problem hiding this comment.
All startup-command output is unconditionally demoted to stderr with the comment assuming it is the wallet opening report. If a startup future ever yields a command with structured data (ConsoleCommand::Json), that JSON is stringified via json_display_text and merged into an unstructured stderr line, breaking the documented --output json contract for stdout with no machine-readable trace of the content. Consider printing the value itself (e.g. as a JSON line prefixed with a marker) or at least documenting this limitation at the --exec flag definition.
There was a problem hiding this comment.
Fixed: in json mode stderr stays a single parseable JSON line (text folded into the payload); text mode keeps the human line.
| Err(WizardError::Cancelled) => { | ||
| println!("Wizard cancelled."); | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
When the user declines the wizard's summary confirmation (WizardError::Cancelled), this returns Ok(None) and falls through to parse_input(&line) on the original line (e.g. bare token issue). That line cannot parse on its own, so the user gets a confusing clap 'missing arguments' error immediately after a deliberate cancel, instead of a clean return to the prompt. Consider returning a sentinel that makes process_line return Ok(None) on cancellation.
Suggestion:
| Err(WizardError::Cancelled) => { | |
| println!("Wizard cancelled."); | |
| Ok(None) | |
| } | |
| Err(WizardError::Cancelled) => { | |
| println!("Wizard cancelled."); | |
| return Ok(None); // do not fall through to parsing the bare command prefix | |
| } |
There was a problem hiding this comment.
Fixed: cancellation is a distinct WizardOutcome that returns to the prompt. The produced one-liner is validated on re-parse, and grammar drift reports an error instead of crashing.
| if !message.is_object() { | ||
| return Some(error_response( | ||
| None, | ||
| -32600, | ||
| "Invalid Request: only single request objects are supported".to_owned(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Batched JSON-RPC requests (arrays) are rejected with a single error even though the advertised protocol revision (2024-11-05) still permits batching. Conforming clients that batch requests will only ever get one protocol-level rejection instead of per-message responses. If intentional, consider documenting this limitation in the tool/server docs; otherwise split the array and answer each entry (empty batches should yield a -32600 error per the JSON-RPC spec).
There was a problem hiding this comment.
Documented: the batched-request rejection is noted where the message is produced in handle_message.
| if buffer.is_empty() { | ||
| return Ok(None); | ||
| } | ||
| let bytes = std::mem::take(buffer); |
There was a problem hiding this comment.
The EOF path delivers the accumulated buffer without re-checking MAX_MESSAGE_BYTES, unlike both newline branches. An oversized message terminated only by end-of-stream (no trailing newline) is returned in full, bypassing the size bound the buffer is supposed to enforce.
Suggestion:
| if buffer.is_empty() { | |
| return Ok(None); | |
| } | |
| let bytes = std::mem::take(buffer); | |
| if buffer.is_empty() { | |
| return Ok(None); | |
| } | |
| if buffer.len() > MAX_MESSAGE_BYTES { | |
| buffer.clear(); | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::InvalidData, | |
| "message exceeds the maximum size", | |
| )); | |
| } | |
| let bytes = std::mem::take(buffer); |
There was a problem hiding this comment.
Removed: the EOF recheck was unreachable because the loop-level size guard precedes every extend. A comment on the loop records the invariant.
| let password = strip_line_terminator(&password); | ||
| if password.is_empty() { | ||
| return Err(WalletCliCommandError::InvalidInput( | ||
| "the password is empty; expected a non-empty password".to_owned(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The wallet encryption password is returned as a plain String and held in ordinary heap memory; neither the file/stdin read buffer nor the returned value is zeroized after use, so the secret can linger in freed heap pages and core dumps. The workspace already depends on zeroize and uses it for exactly this purpose elsewhere (e.g. wallet/src/key_chain/master_key_chain/mod.rs wraps passphrases in zeroize::Zeroizing). Consider wrapping the password in zeroize::Zeroizing<String> (or crypto's existing secret-buffer types) for the whole path, including the prompt_password_hidden and file-read buffers.
Suggestion:
| let password = strip_line_terminator(&password); | |
| if password.is_empty() { | |
| return Err(WalletCliCommandError::InvalidInput( | |
| "the password is empty; expected a non-empty password".to_owned(), | |
| )); | |
| } | |
| let password = zeroize::Zeroizing::new(strip_line_terminator(&password).to_owned()); | |
| if password.is_empty() { | |
| return Err(WalletCliCommandError::InvalidInput( | |
| "the password is empty; expected a non-empty password".to_owned(), | |
| )); | |
| } |
There was a problem hiding this comment.
Fixed: Zeroizing end-to-end on the password path (see the password-file thread).
| WalletCommand::SpendUtxo { | ||
| utxo, | ||
| address, | ||
| htlc_secret: _, | ||
| } => Some(format!( | ||
| "Spend the outpoint {utxo:?} (source and output index) to {address}?" | ||
| )), |
There was a problem hiding this comment.
The confirmation prompt for SendToAddress omits the --utxos selection, and SpendUtxo renders the outpoint with the Debug formatter ({utxo:?}), which produces Rust tuple/struct debug syntax rather than the CLI's own outpoint notation (tx(<id>,<index>)). Users are asked to confirm an asset-moving operation with incomplete or developer-format input information. Consider formatting the outpoints with the same human-readable notation the CLI accepts, and listing the selected UTXOs in the SendToAddress prompt when --utxos is given.
Suggestion:
| WalletCommand::SpendUtxo { | |
| utxo, | |
| address, | |
| htlc_secret: _, | |
| } => Some(format!( | |
| "Spend the outpoint {utxo:?} (source and output index) to {address}?" | |
| )), | |
| WalletCommand::SpendUtxo { | |
| utxo, | |
| address, | |
| htlc_secret: _, | |
| } => Some(format!( | |
| "Spend the outpoint {utxo} (source and output index) to {address}?" | |
| )), |
There was a problem hiding this comment.
Fixed: the send recap lists UTXOs with a cap ("and N more"), and outpoints render via Display, which round-trips through the parser and no longer elides hashes.
| .collect::<Vec<_>>(); | ||
| // The REPL thread is sized for grammar construction, so build the | ||
| // grammar directly instead of nesting another large-stack spawn. | ||
| let repl_command = Arc::new(build_repl_command(cold_wallet, true)); |
There was a problem hiding this comment.
Here the Arc is cloned before being moved into WalletCompletions::new, so Arc::get_mut inside new fails and Command::build() is never called. build_repl_command assembles the tree via augment_subcommands but does not call build(), meaning the help flag is not injected and the completer's own heuristic check (get_long() == Some("help")) may fail — in release builds this degrades to a warning log and empty completions. Build the tree before sharing it, e.g. let mut cmd = build_repl_command(cold_wallet, true); cmd.build(); let repl_command = Arc::new(cmd);.
Suggestion:
| let repl_command = Arc::new(build_repl_command(cold_wallet, true)); | |
| let mut command = build_repl_command(cold_wallet, true); | |
| command.build(); | |
| let repl_command = Arc::new(command); |
There was a problem hiding this comment.
False positive: build_repl_command ends with .build(), so the Arc it hands over is always built. A from_built constructor was added anyway so the shared path states the contract, and the convenience path keeps its own guard.
| if let Some(question) = wallet_cli_commands::confirm::confirmation_prompt(command) | ||
| && !assume_yes | ||
| { | ||
| return Err(WalletCliError::ConfirmationRequired(format!( |
There was a problem hiding this comment.
The confirmation gate depends entirely on confirmation_prompt classifying commands. In wallet-cli-commands/src/confirm.rs, WalletCommand::GenerateBlock/GenerateBlocks are listed among the read-only variants returning None, yet generating blocks produces and submits blocks to the chain on the connected node — a real state change that will execute without consent in --exec/--commands-file/wizard flows even without --yes. Please verify whether these (and similar node-mutating commands like Ban/AddReservedPeer) should prompt, or document why they are exempt.
There was a problem hiding this comment.
Fixed: node-generate-block/node-generate-blocks are classified as state-changing and prompt; a test pins the generated-blocks recap.
| # Bundled root certificates include distrusted emSign intermediates. The | ||
| # 1.x copy of `webpki-root-certs` was updated past the affected versions; |
There was a problem hiding this comment.
The ignores for RUSTSEC-2025-0141 (bincode unmaintained) and RUSTSEC-2026-0097 (rand 0.7 via probabilistic-collections) are removed, but no change in this PR visibly removes or upgrades those vulnerable dependencies. If bincode or probabilistic-collections 0.7 (which pins rand 0.7) is still present in Cargo.lock, cargo deny check will now fail. Please confirm the vulnerable crates are actually gone from the dependency tree (e.g., via cargo deny check output or an updated lockfile in this PR), or keep the ignores with a note until the underlying upgrade lands.
There was a problem hiding this comment.
Confirmed: bincode and probabilistic-collections are absent from Cargo.lock on this branch (they were removed upstream on master), and cargo deny check passes in the repository checks.
| output = await self._write_command("transaction-list-pending\n") | ||
| pattern = r'Id<Transaction>\{([^}]*)\}' | ||
| return re.findall(pattern, output) | ||
| return re.findall(r'^\s*([0-9a-f]{64})\s*$', output, re.MULTILINE) |
There was a problem hiding this comment.
The new regex matches any line consisting of exactly 64 lowercase hex characters, whereas the previous pattern matched only Id<Transaction>{...} wrappers. The command output can contain other bare 64-hex values besides pending transaction ids (e.g. block hashes printed by preceding/successive commands, since the captured output is a raw dump of the wallet's stdout stream that may include leftover output from other commands or async wallet messages). Such lines would be incorrectly returned as pending transaction ids, causing flaky or wrong test results. Consider keeping a more specific anchor (e.g. require the surrounding Id<Transaction>{...} format in the new output, or strip only the body section after the "Pending transactions" header before applying the hex regex).
Suggestion:
| return re.findall(r'^\s*([0-9a-f]{64})\s*$', output, re.MULTILINE) | |
| return re.findall(r'^\s*Id<Transaction>\{([^}]*)\}\s*$', output, re.MULTILINE) |
There was a problem hiding this comment.
Addressed: the extraction is scoped to the section after the last Pending transactions header, so stray 64-hex lines from earlier output are ignored.
| async fn version(&self) -> Result<String, String> { | ||
| WalletInterface::version(&self.0).await.map_err(|err| err.to_string()) | ||
| } |
There was a problem hiding this comment.
All wallet/RPC failures are flattened to String at this boundary (trait signature Result<_, String> plus map_err(|err| err.to_string()) here). This discards error type and context early: consumers of the MCP result text cannot distinguish auth failures, connection errors, or daemon errors, and tests can only string-match. Consider a small typed error enum (or anyhow::Error/Box<dyn Error> internally) converted to a user-facing message only at the tool_result rendering boundary.
There was a problem hiding this comment.
Fixed: shared rpc_error labeling at the adapter boundary.
| Err(err) if err.kind() == std::io::ErrorKind::InvalidData => { | ||
| let encoded = serde_json::to_string(&serde_json::json!({ | ||
| "jsonrpc": JSONRPC_VERSION, | ||
| "id": null, | ||
| "error": { | ||
| "code": -32700, | ||
| "message": format!("Parse error: {err}") | ||
| } | ||
| }))? + "\n"; |
There was a problem hiding this comment.
The recovery contract (every InvalidData from next_message has already consumed/discarded the offending line) is enforced only by this comment; nothing in the type system or code enforces it, and the recovery branch has no bound on consecutive errors. If any future InvalidData path is added without resync (or an existing path is refactored), serve will emit -32700 responses in an unbounded busy loop over the pipe. Consider returning a dedicated enum (e.g. FramingError::{Oversized, Utf8}) whose variants provably imply the line was discarded, and/or capping consecutive framing errors per session.
There was a problem hiding this comment.
Declined: a dedicated framing-error type would add public surface for one internal invariant. The recovery contract is documented and pinned by the resync tests.
| exec: None, | ||
| output: wallet_cli_lib::config::CliOutputFormat::Text, | ||
| yes: true, |
There was a problem hiding this comment.
The test framework now hardcodes yes: true for both boot and run options. This means every test runs with confirmation prompts silently auto-accepted, so the new confirm.rs flow (which aborts non-interactive runs without --yes, per the CliArgs docs) is never exercised by these tests, and any test asserting on a confirmation prompt's behavior/output would pass/fail without actually covering it. Consider making this configurable per-test (e.g., a field on CliTestFramework) or at least adding dedicated tests that run with yes: false to cover the abort-on-unconfirmed path.
Suggestion:
| exec: None, | |
| output: wallet_cli_lib::config::CliOutputFormat::Text, | |
| yes: true, | |
| exec: None, | |
| output: wallet_cli_lib::config::CliOutputFormat::Text, | |
| yes: self.auto_confirm, |
There was a problem hiding this comment.
Fixed: the harness supports per-run --yes (setup_with), and a test covers the confirmation gate rejecting without it.
6319d67 to
5db1d81
Compare
|
|
||
| impl ReadonlyWallet for RpcWallet { | ||
| async fn version(&self) -> Result<String, String> { | ||
| WalletInterface::version(&self.0).await.map_err(|err| err.to_string()) |
There was a problem hiding this comment.
All wallet RPC and serialization errors are flattened to strings via map_err(|err| err.to_string()) at the adapter boundary and then surfaced verbatim to the MCP client (an LLM-facing surface). This discards the typed error and can leak internal details (connection addresses, auth-related messages from the daemon or HTTP layer) into tool result text. Consider mapping to a small set of sanitized, categorized error messages (e.g. "wallet RPC unreachable", "wallet returned an error") while logging the full error to stderr for diagnostics.
There was a problem hiding this comment.
Partially applied: the full daemon error is echoed to the server's stderr for diagnostics while the tool result keeps the labeled text. The daemon and this server run for the same local operator, so categorizing further would hide the actionable part of the message.
| command @ (WalletCommand::Account { .. } | ||
| | WalletCommand::Address { .. } | ||
| | WalletCommand::Send { .. } | ||
| | WalletCommand::Data { .. } | ||
| | WalletCommand::Staking { .. } | ||
| | WalletCommand::Delegation { .. } | ||
| | WalletCommand::Token { .. } | ||
| | WalletCommand::Order { .. } | ||
| | WalletCommand::Htlc { .. } | ||
| | WalletCommand::Transaction { .. } | ||
| | WalletCommand::Node { .. } | ||
| | WalletCommand::Peer { .. } | ||
| | WalletCommand::Config { .. }) => { | ||
| // Nested groups translate to their flat equivalents. The recursive call | ||
| // is boxed to keep the future size finite; translation always produces a | ||
| // flat command, so the recursion never goes deeper than one level. | ||
| let normalized = normalize_nested_wallet_command(command); | ||
| // Translation must terminate: a nested command fed back into this handler | ||
| // would recurse unboundedly through the boxed call. | ||
| assert!( | ||
| !is_nested_wallet_command(&normalized), | ||
| "translation must produce a flat command, otherwise this recursion is unbounded" | ||
| ); | ||
| Box::pin(self.handle_wallet_command(chain_config, normalized)).await | ||
| } |
There was a problem hiding this comment.
The catch-all arm command => command in normalize_nested_wallet_command returns still-nested commands unchanged for any newly added group variant. The dispatch site then hits assert!(!is_nested_wallet_command(&normalized)), turning every invocation of a forgotten group into a runtime panic in the async handler (task abort / opaque failure) instead of a graceful "unknown command" error. Consider returning a WalletCliCommandError::InvalidInput("unsupported nested command") from the fallthrough for nested inputs, or restructuring so the exhaustiveness is enforced at compile time (e.g. only grouped variants are accepted as input).
There was a problem hiding this comment.
Fixed: a nested command that survives translation without a flat form now returns an InvalidInput error naming the command; the recursion assert was replaced.
| if timestamp { | ||
| Some(format_block_timestamp( | ||
| wallet.chainstate_info().await?.best_block_timestamp, | ||
| )) | ||
| } else { | ||
| None | ||
| }, |
There was a problem hiding this comment.
In the single-flag --timestamp path, the timestamp comes from chainstate_info(), which per the code's own comment fabricates genesis values on a cold wallet while the per-value RPCs fail. As a result node best-block --timestamp silently prints fabricated data on a cold wallet, whereas --id and --height correctly error — inconsistent behavior within the same command. Consider probing the node (like the multi-flag path does) even for the timestamp-only case.
Suggestion:
| if timestamp { | |
| Some(format_block_timestamp( | |
| wallet.chainstate_info().await?.best_block_timestamp, | |
| )) | |
| } else { | |
| None | |
| }, | |
| if timestamp { | |
| // Probe the node first so a cold wallet errors like --id/--height | |
| // instead of silently printing the fabricated genesis timestamp. | |
| wallet.node_best_block_id().await?; | |
| Some(format_block_timestamp( | |
| wallet.chainstate_info().await?.best_block_timestamp, | |
| )) | |
| } else { | |
| None | |
| }, |
There was a problem hiding this comment.
Fixed: the --timestamp arm probes the node first, so a cold wallet errors consistently with --id and --height instead of printing the genesis stub.
| pub struct DialoguerBackend; | ||
|
|
||
| impl PromptBackend for DialoguerBackend { | ||
| fn text(&mut self, prompt: &str, default: Option<&str>) -> Result<String, WizardError> { |
There was a problem hiding this comment.
DialoguerBackend reads from stdin/terminal unconditionally; when stdin is not a TTY (piped input / automation), dialoguer fails with a confusing raw I/O error instead of a clear "wizard requires an interactive terminal" message. Consider gating wizard invocation on terminal detection at the entry point, or checking std::io::stdin().is_terminal() in DialoguerBackend and returning a typed WizardError::InvalidInput with an actionable message.
There was a problem hiding this comment.
Fixed: wizard invocation is gated on a terminal on standard input with an actionable message, matching the password-prompt gate.
| if !root.get_arguments().any(|arg| arg.get_long() == Some("help")) { | ||
| debug_assert!(false, "a shared clap Command must already be built"); | ||
| // The assertion above is compiled out in release builds, so also | ||
| // leave a trace: the completer will produce no candidates. | ||
| logging::log::warn!( | ||
| "WalletCompletions received an unbuilt clap command tree; completions will be empty" | ||
| ); | ||
| } |
There was a problem hiding this comment.
When the completer receives an already-shared Arc that was not built, it silently yields no candidates; the "--help flag present" proxy for "tree was built" is fragile (e.g. a built tree with disable_help_flag(true) would still trip the warning, and an unbuilt tree might not be detected in release builds where debug_assert is compiled out). Prefer a typed constructor that requires a built tree (e.g. WalletCompletions::from_built(Arc)) or call build() eagerly in build_repl_command's contract, so a future caller cannot get silently empty completions.
Suggestion:
| if !root.get_arguments().any(|arg| arg.get_long() == Some("help")) { | |
| debug_assert!(false, "a shared clap Command must already be built"); | |
| // The assertion above is compiled out in release builds, so also | |
| // leave a trace: the completer will produce no candidates. | |
| logging::log::warn!( | |
| "WalletCompletions received an unbuilt clap command tree; completions will be empty" | |
| ); | |
| } | |
| pub fn from_built(root: Arc<Command>) -> Self { | |
| Self { root } | |
| } |
There was a problem hiding this comment.
Fixed: a from_built constructor expresses the built-tree contract for shared Arcs and the run loop uses it; the convenience constructor keeps its own guard for owned trees.
| .get("protocolVersion") | ||
| .and_then(Value::as_str) | ||
| .filter(|requested| SUPPORTED_PROTOCOL_VERSIONS.contains(requested)) | ||
| .unwrap_or(SUPPORTED_PROTOCOL_VERSIONS.last().copied().unwrap_or("2024-11-05")); |
There was a problem hiding this comment.
The fallback literal "2024-11-05" duplicates the sole entry of SUPPORTED_PROTOCOL_VERSIONS. When a newer protocol revision is added to the list, the fallback unwrap_or default silently stays at the old version, so clients requesting an unknown version get an outdated downgrade that is hard to diagnose. Derive the default from the list only, or reference a named constant.
Suggestion:
| .unwrap_or(SUPPORTED_PROTOCOL_VERSIONS.last().copied().unwrap_or("2024-11-05")); | |
| .unwrap_or_else(|| SUPPORTED_PROTOCOL_VERSIONS.last().copied().expect("SUPPORTED_PROTOCOL_VERSIONS is non-empty")); |
There was a problem hiding this comment.
Fixed: the fallback derives from SUPPORTED_PROTOCOL_VERSIONS.last() per the documented newest-last order; no duplicated literal remains.
53040b3 to
831e2ef
Compare
| let response = match serde_json::from_str::<Value>(trimmed) { | ||
| Ok(message) => handle_message(&message, &wallet).await, |
There was a problem hiding this comment.
The single-threaded stdio event loop awaits handle_message(...).await — and behind it the wallet RPC client calls — with no timeout. The wallet-rpc-client crate has no visible timeout handling, so a hung daemon connection stalls serve() indefinitely and the MCP client gets no response and no error. Consider wrapping each request in tokio::time::timeout and returning an isError tool result (or JSON-RPC error) on deadline, so the loop stays responsive.
Suggestion:
| let response = match serde_json::from_str::<Value>(trimmed) { | |
| Ok(message) => handle_message(&message, &wallet).await, | |
| let response = match serde_json::from_str::<Value>(trimmed) { | |
| Ok(message) => { | |
| // Bound each request so a hung wallet daemon cannot stall the loop. | |
| match tokio::time::timeout(REQUEST_TIMEOUT, handle_message(&message, &wallet)).await { | |
| Ok(response) => response, | |
| Err(_) => Some(serde_json::json!({ | |
| "jsonrpc": JSONRPC_VERSION, | |
| "id": message.get("id").cloned(), | |
| "error": { "code": -32000, "message": "wallet request timed out" } | |
| })), | |
| } | |
| } |
There was a problem hiding this comment.
Fixed: each request is bounded by a 60-second timeout that answers with a JSON-RPC -32000 error (echoing the request id, so string ids and null ids work), and the timeout is logged to stderr like every other daemon failure.
|
Addressed the latest review rounds (commits 831e2ef..69c88fc):
All open review threads now have replies; remaining diffs to the suggestions are marked declined with reasons in the threads. |
| let account_index = U31::from_u32(account_index) | ||
| .ok_or_else(|| format!("Invalid account index: {account_index}"))?; | ||
| let utxo_states = utxo_states.into_iter().map(utxo_state).collect(); |
There was a problem hiding this comment.
parse_account_index accepts the full u32 range, but U31::from_u32 rejects values >= 2^31 (MSB set). This maps the failure to a String error that call_tool classifies as ToolCallError::Execution (an isError tool result) rather than a -32602 protocol error, producing an inconsistent error surface for what is purely invalid input. Either reject the out-of-range indices during argument parsing in lib.rs (e.g. validate against U31's range there) or map this to a protocol error.
Suggestion:
| let account_index = U31::from_u32(account_index) | |
| .ok_or_else(|| format!("Invalid account index: {account_index}"))?; | |
| let utxo_states = utxo_states.into_iter().map(utxo_state).collect(); | |
| let account_index = U31::from_u32(account_index) | |
| .ok_or_else(|| format!("Invalid account index: {account_index}"))?; |
There was a problem hiding this comment.
Fixed: the protocol parser now rejects indices ≥ 2³¹ with a -32602, and the tool schema's maximum (2147483647) matches the enforced bound, so the U31 conversion can no longer produce a tool-execution error for invalid input.
| clap = { workspace = true, features = ["derive"] } | ||
| crossterm.workspace = true | ||
| derive_more.workspace = true | ||
| dialoguer.workspace = true |
There was a problem hiding this comment.
The newly added dialoguer, zeroize, and rpassword dependencies are not referenced anywhere in wallet/wallet-cli-commands/src (no use, full-path, or type-name matches). Unused dependencies bloat the build and misrepresent the crate's API surface; remove them here or move them to the crate that actually uses them.
There was a problem hiding this comment.
False positive: dialoguer is used by the wizard module (src/wizard/mod.rs), zeroize by the password path (Zeroizing in src/command_handler/mod.rs), and rpassword by the hidden prompt — all in this crate.
| let file = std::fs::File::open(&read_path)?; | ||
| let metadata = file.metadata()?; |
There was a problem hiding this comment.
In the file branch of read_password_from_source, there is no check that the opened file is a regular file (e.g. metadata.file_type().is_file() or FileType::is_file()). A FIFO or device passed via --password-file will block indefinitely inside the spawn_blocking read with no timeout, hanging the CLI; the permissions warning likewise only inspects mode bits of whatever was opened. Consider rejecting non-regular files, or at least documenting the blocking behavior.
Suggestion:
| let file = std::fs::File::open(&read_path)?; | |
| let metadata = file.metadata()?; | |
| let file = std::fs::File::open(&read_path)?; | |
| let metadata = file.metadata()?; | |
| if !metadata.file_type().is_file() { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::InvalidInput, | |
| "the password file is not a regular file", | |
| )); | |
| } |
There was a problem hiding this comment.
Fixed: a cheap pre-open metadata check now fast-fails non-regular files before open(2) (which itself blocks on a writer-less FIFO), the post-open handle check stays authoritative for the race, and a test covers the directory case.
| ColdWalletCommand::EncryptPrivateKeys { | ||
| password: password.to_string(), | ||
| }, |
There was a problem hiding this comment.
The nested wallet encrypt/unlock path reads the password through a Zeroizing buffer, but then deliberately copies it into a plain String via password.to_string() and passes it to ColdWalletCommand::EncryptPrivateKeys/UnlockPrivateKeys, whose grammar field is a non-wiping String. The password therefore lives in non-zeroized memory (the command enum, the wallet RPC call) and can persist in core dumps/swap, partially defeating the purpose of the secure input path. The legacy flat spellings also still accept the password as a positional CLI argument (visible in process listings), only mitigated by a runtime warning. Consider a zeroizing-aware grammar type or passing the password out-of-band to the RPC layer.
There was a problem hiding this comment.
Declined: the grammar field and the wallet RPC payload are plaintext by design (the daemon stores the password it receives), so zeroizing the CLI-side copy would not reduce the real exposure. The dominant interactive risk — argv visibility — is already gone because prompts are the default path. A zeroizing-aware grammar type remains a possible follow-up.
| if crate::command_handler::is_nested_wallet_command(command) { | ||
| let normalized = | ||
| crate::command_handler::normalize_nested_wallet_command(command.clone()); | ||
| wallet_confirmation(&normalized) |
There was a problem hiding this comment.
confirmation_prompt clones the entire WalletCommand (including potentially large hex payloads in SubmitTransaction, Sign, Compose) just to classify nested commands via normalize_nested_wallet_command. This deep copy runs on every command submission from the interactive REPL (repl/interactive/mod.rs) and non-interactive runner (repl/mod.rs). Consider a read-only classification function that matches on nested group variants without rebuilding, or destructuring only the group payload.
There was a problem hiding this comment.
Declined: the clone runs once per user-submitted nested command (flat spellings are classified by reference), bounded by command size. A read-only classifier would duplicate the translation logic and drift from it.
| Err(err) => { | ||
| println!("The confirmation prompt is unavailable ({err}); treating as declined."); | ||
| false | ||
| } |
There was a problem hiding this comment.
Treating an unavailable confirmation prompt as a silent decline means that in any interactive session driven over a pipe (e.g. echo ... | wallet-cli with no --commands-file, or via a pty wrapper), every state-changing command is skipped with only a stdout message and no error exit status. Callers cannot distinguish 'user said no' from 'prompt infrastructure failed'. Consider returning Err(WalletCliError::ConfirmationRequired(...)) when std::io::stdin() is not a terminal, so scripts get a non-zero exit instead of a silently skipped mutation.
Suggestion:
| Err(err) => { | |
| println!("The confirmation prompt is unavailable ({err}); treating as declined."); | |
| false | |
| } | |
| Err(err) => { | |
| if !std::io::IsTerminal::is_terminal(&std::io::stdin()) { | |
| return Err(WalletCliError::ConfirmationRequired(format!( | |
| "cannot show the confirmation prompt ({err}); pass --yes or run interactively" | |
| ))); | |
| } | |
| println!("The confirmation prompt is unavailable ({err}); treating as declined."); | |
| false | |
| } |
There was a problem hiding this comment.
Fixed: without a prompt-capable terminal (standard input or standard error), the command now fails with ConfirmationRequired instead of silently declining; a genuine failure on a real terminal still declines per-command so the session survives.
| if std::env::args_os().nth(1).as_deref() == Some(std::ffi::OsStr::new("completions")) { | ||
| generate_completions(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The completions verb is intercepted by a raw string check on argv[1] before clap parsing. This is invisible to --help and to clap's error messages, and it will silently hijack any future invocation whose first positional argument legitimately equals completions (or a subcommand of that name). Consider exposing it as a real clap subcommand (e.g. with #[clap(subcommand)] or hide = true) so it appears in completion generation, participates in grammar validation, and produces proper error output when misused. If the pre-parser interception is intentional, at least add a test covering a positional value of "completions" to lock in the expected behavior.
There was a problem hiding this comment.
Documented and pinned: the trade-off is spelled out on the handler doc, and a test asserts the top-level grammar has no completions subcommand, so the verb can never be claimed twice.
| let bytes: Vec<u8> = std::mem::take(buffer); | ||
| // A protocol boundary must reject, not repair: lossy decoding could | ||
| // silently turn a truncated request into a different valid one. | ||
| let line = String::from_utf8(bytes) | ||
| .map_err(|err| std::io::Error::new(std::io::ErrorKind::InvalidData, err))?; |
There was a problem hiding this comment.
The recovery contract in serve states that every InvalidData error path must leave the reader positioned past the offending line, and specifically calls out the from_utf8 branch. However, the tests only cover oversized-message resync scenarios; there is no test feeding a line containing invalid UTF-8 (e.g. \xff\xfe\n) to next_message to prove that the stream stays usable after the String::from_utf8 failure. If that branch ever mis-consumed bytes (or failed to), the serve loop would emit parse errors forever or desynchronize silently, and nothing would catch it.
Suggestion:
| let bytes: Vec<u8> = std::mem::take(buffer); | |
| // A protocol boundary must reject, not repair: lossy decoding could | |
| // silently turn a truncated request into a different valid one. | |
| let line = String::from_utf8(bytes) | |
| .map_err(|err| std::io::Error::new(std::io::ErrorKind::InvalidData, err))?; | |
| #[tokio::test] | |
| async fn invalid_utf8_line_is_skipped_and_stream_stays_usable() { | |
| let mut input = b"{\xff\xfe}".to_vec(); | |
| input.extend_from_slice(b"\n{\"method\":\"ping\"}\n"); | |
| let mut reader = tokio::io::BufReader::with_capacity(8192, &input[..]); | |
| let mut buffer = Vec::new(); | |
| let first = next_message(&mut reader, &mut buffer).await; | |
| assert_eq!(first.unwrap_err().kind(), std::io::ErrorKind::InvalidData); | |
| let second = next_message(&mut reader, &mut buffer).await; | |
| assert_eq!(second.unwrap().as_deref(), Some("{\"method\":\"ping\"}")); | |
| } |
There was a problem hiding this comment.
Fixed: a test feeds an invalid-UTF-8 line and asserts the stream stays usable (the recovered outcome is rendered by error kind, so the assertion no longer depends on message text).
| // Recovery contract: every InvalidData from next_message has already | ||
| // consumed or discarded the offending line (or hit EOF), so the next | ||
| // iteration never re-reads the same bytes. The one error that must | ||
| // NOT be recovered from — skip_rest_of_line's endless-line give-up — | ||
| // uses io::Error::other on purpose; reclassifying it to InvalidData, | ||
| // or adding an InvalidData path that does not resync, would spin | ||
| // this loop forever. |
There was a problem hiding this comment.
The resync contract is only enforced by a code comment: it depends on skip_rest_of_line returning io::Error::other (which terminates the session) while every "recoverable" path must use InvalidData and fully consume the offending line. Nothing in the code enforces this pairing — if a future InvalidData branch returns before consuming/discarding the line, the serve loop will spin forever emitting -32700 parse errors to the peer. Consider encoding the distinction in the type (e.g. a private enum with Recovered/Fatal variants from next_message) so the compiler, not a comment, guarantees the loop invariant.
Suggestion:
| // Recovery contract: every InvalidData from next_message has already | |
| // consumed or discarded the offending line (or hit EOF), so the next | |
| // iteration never re-reads the same bytes. The one error that must | |
| // NOT be recovered from — skip_rest_of_line's endless-line give-up — | |
| // uses io::Error::other on purpose; reclassifying it to InvalidData, | |
| // or adding an InvalidData path that does not resync, would spin | |
| // this loop forever. | |
| enum MessageError { | |
| /// The offending line was fully consumed or discarded; the stream is usable. | |
| Recovered(std::io::Error), | |
| /// The stream is in an unknown state; the session must end. | |
| Fatal(std::io::Error), | |
| } |
There was a problem hiding this comment.
Fixed: next_message now returns a MessageOutcome enum whose Recovered/Fatal split makes the contract compile-enforced; the recovered parse-error reply keeps its newline terminator, and transient interrupts are retried with a bounded retry count before the stream is declared unusable.
Summary
Refactors the wallet CLI for usability while keeping every legacy command spelling working. 32 commits.
Command grammar (old and new coexist)
wallet <sub>,account <sub>,address <sub>,staking <sub>,delegation <sub>,token <sub>,order <sub>,htlc <sub>,transaction <sub>,node <sub>,peer <sub>.address-send,staking-create-pool, ...) keep working unchanged; a LEGACY_ALIASES table plus coexistence tests pin old names, output shapes and behaviors (byte-identical output is asserted where shapes match).Output
CommandOutput,ConsoleCommand::Json/Warn).{:#?}debug dumps replaced with human-readable renderers for chainstate-info, connected-peers, reserved-peers, pending-transactions, find-timestamps-for-staking (hidden command).--exec "<command>" --output text|jsonone-shot mode with exit status 0/1; JSON streams keep startup noise on stderr.Safety
--yesskips, non-interactive runs abort without it. The gate works identically for nested and legacy spellings.--password-file(or-for stdin) for wallet encrypt/unlock; TTY prompt otherwise; insecure positional-password spellings warn. Secret env values are hidden from--helpeverywhere.--qr).Completions & MCP
wallet-cli completions --shell <shell>generates completion scripts; an interactive grammar-walking completer with hint ghost text in the REPL.wallet-mcp-serverbinary: a read-only MCP (JSON-RPC over stdio) server exposing version, wallet-info, best-block, balance and standalone-addresses tools against a running wallet RPC daemon (cookie/basic/no-auth). No write tools.Testing