diff --git a/CHANGELOG.md b/CHANGELOG.md index 837f61df9e..432b001b79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ All notable changes to this project will be documented in this file. - Centralize top-level error rendering through `doublezero_cli_core::error::render_eyre`. Replaces three ad-hoc `eprintln!("Error: {e}")` sites in `client/doublezero/src/main.rs` (env-parse failure, env-config resolution failure, top-level command failure) with a single helper that prints `Error: ` followed by the full chain of causes on stderr. - Rename the `smartcontract/cli/` crate from `doublezero_cli` to `doublezero-serviceability-cli` to satisfy RFC-20's module-crate naming contract (`doublezero--cli` in kebab-case). The crate stays at `smartcontract/cli/`; only the `[package].name` and `[lib].name` change (lib name is `doublezero_serviceability_cli` because Rust requires underscores in import paths). All in-tree consumers are updated: `client/doublezero`, `client/doublezero-geolocation-cli`, `controlplane/doublezero-admin`, and the workspace `Cargo.toml`. External operators who depend on the workspace crate by its old name (`doublezero_cli`) must update their `Cargo.toml` and `use` statements. No user-facing command, flag, or output change. - Migrate `location get` to the RFC-20 conforming verb pattern as the project's reference. `GetLocationCliCommand::execute` is now `async fn`, takes `&CliContext` as its first non-self argument, and emits a `tracing::debug!` event so `-v` surfaces what the verb is doing. The verb's user-facing args, flags, table layout, and JSON schema are unchanged. The unit test consumes the shared `doublezero_cli_core::testing::cli_context_default_for_tests()` helper and continues to use the existing `MockCliCommand` (auto-generated by `#[automock]`) as the backend. Binary dispatch arms in `client/doublezero` and `controlplane/doublezero-admin` are updated to `.await` the new method; other location verbs (Create, Update, List, Delete) keep their current sync signatures and migrate opportunistically. + - Add `docs/cli-standard.md`, the contributor-facing summary of RFC-20 with the `location get` worked example and pointers to the shared validators, formatters, logging facade, and test helpers in `doublezero-cli-core`. + - Update `CLAUDE.md` with a CLI-standard section pointing at RFC-20, the contributor doc, and the reference verb so future contributors land on the standard quickly. ## [v0.24.0](https://github.com/malbeclabs/doublezero/compare/client/v0.23.0...client/v0.24.0) - 2026-05-22 diff --git a/CLAUDE.md b/CLAUDE.md index 8e70d35363..752b1cfc71 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,6 +100,13 @@ make generate-fixtures # Regenerate .bin/.json fixtures from Rust - When asked if a doc is up to date, evaluate it against its intended purpose and scope — not against whatever was most recently worked on. Implementation bug fixes and edge-case handling are not design decisions. Don't inflate docs with implementation details just because they're fresh in context. +## CLI Standard (RFC-20) + +- New CLI verbs and module crates follow RFC-20 (`rfcs/rfc20-cli-standardization.md`). A contributor-facing summary lives at `docs/cli-standard.md`, with `smartcontract/cli/src/location/get.rs` as the reference verb. +- Shared CLI utilities (`CliContext`, validators, formatters, `RequirementCheck`, `init_logging`) live in `crates/doublezero-cli-core/`. Verbs MUST consume the shared validators and route diagnostic output through `tracing`. The `doublezero` binary owns global flags (`--env`, `--url`, `--ws`, `--solana-url`, `--keypair`, `--program-id`, `--geo-program-id`, `--sock-file`, `--log-verbose`, `--version`); modules MUST NOT redeclare them. +- The serviceability module crate is named `doublezero-serviceability-cli` (crate path `smartcontract/cli/`, import path `doublezero_serviceability_cli`). +- Migration is opportunistic. Existing verbs are grandfathered; new verbs conform from day one. + ## Style & Terminology - Use "onchain" (one word, no hyphen), never "on-chain" diff --git a/crates/doublezero-cli-core/src/validators.rs b/crates/doublezero-cli-core/src/validators.rs index 6a9018906b..86d19bf039 100644 --- a/crates/doublezero-cli-core/src/validators.rs +++ b/crates/doublezero-cli-core/src/validators.rs @@ -1,11 +1,19 @@ //! Shared `clap` value-parser validators. //! -//! Per RFC-20 (§Argument conventions): "Identifiers accept both pubkey and +//! Per RFC-20 (§Argument conventions): identifiers accept both pubkey and //! code. Any flag that references an onchain entity MUST accept either a //! Solana pubkey or the entity's human-readable code via the shared -//! validator. The magic value `\"me\"` resolves to the current payer's pubkey -//! at execution time." Module crates re-export and consume these helpers -//! rather than re-implementing them. +//! validator. Module crates re-export and consume these helpers rather than +//! re-implementing them. +//! +//! Resolution of the literal `"me"` to the current payer's pubkey is a +//! verb-level responsibility, performed in the verb's `execute` path using +//! the payer pubkey from `CliContext`. The validators here only enforce +//! grammar; they do not perform runtime resolution. `validate_pubkey` +//! short-circuits `"me"` because pubkey-only fields have no code fallback; +//! `validate_pubkey_or_code` admits `"me"` as a syntactically valid code, +//! and verbs that opt in to payer resolution check for the literal +//! themselves. use doublezero_program_common::{types::parse_utils::bandwidth_parse, validate_account_code}; use solana_sdk::pubkey::Pubkey; diff --git a/docs/cli-standard.md b/docs/cli-standard.md new file mode 100644 index 0000000000..6dd1ed2906 --- /dev/null +++ b/docs/cli-standard.md @@ -0,0 +1,248 @@ +# DoubleZero CLI Standard (RFC-20) + +This document is a contributor-facing summary of the CLI standard defined by +[RFC-20: CLI Standardization and Library Composition](../rfcs/rfc20-cli-standardization.md). +The RFC is the normative source; this page is the day-to-day reference for +writing a new verb or migrating an existing one. + +## The shape + +DoubleZero ships a single `doublezero` binary. The binary is thin: it parses +global flags, builds a `CliContext`, then dispatches to verbs that live in +**module crates**. Each module crate is a library named +`doublezero--cli` and conforms to a fixed contract. + +``` +doublezero (binary) client/doublezero/ + └─ CliContext + dispatch +doublezero-cli-core (shared library) crates/doublezero-cli-core/ + ├─ CliContext, CliContextBuilder, OutputFormat + ├─ RequirementCheck (preflight bitflags) + ├─ shared validators (pubkey, code, bandwidth, latency, ...) + ├─ display formatters + ├─ init_logging (tracing facade) + └─ testing helpers +doublezero-serviceability-cli (module) smartcontract/cli/ +doublezero--cli ... +``` + +The core crate stays small on purpose: it depends on `clap`, the logging +facade, `doublezero-config`, and `doublezero-program-common`. The Solana +SDK, daemon HTTP stack, and remote-service transports live with the module +crates that use them. + +## The module contract + +A CLI module crate **MUST**: + +1. Be a library-only crate named `doublezero--cli`. No `[[bin]]`. +2. Export at least one top-level subcommand type that derives clap's + `Subcommand`. Verbs are variants. +3. Provide an `async fn execute` on each subcommand type. The runtime lives + in the binary; modules MUST NOT call `block_on` or hide async work behind + a sync facade. +4. Define per-verb args and display types **colocated** with the verb. +5. Consume `CliContext` for environment-derived inputs. Modules MUST NOT + read environment variables, configuration files, or `argv` directly. +6. Use the shared validators (`validate_pubkey`, `validate_pubkey_or_code`, + `validate_code`, `validate_parse_bandwidth`, ...) from + `doublezero_cli_core::validators` wherever those types appear. +7. Send all output through the writer. `println!`, `eprintln!`, and + `print!` MUST NOT appear in execute paths. Diagnostic logging goes + through `tracing` (stderr). + +A module **SHOULD** keep each verb in a single file, expose its backend +client(s) behind a mockable trait, and provide per-verb unit tests against a +mocked client. + +## Argument conventions + +- Named flags only. No positional arguments. +- Long names in kebab-case. +- Short aliases on booleans only. +- Identifiers that reference an onchain entity use `validate_pubkey_or_code` + and accept either a pubkey or the entity's code. Where a flag denotes a + signer or payer-scoped entity (for example `--administrator`, + `--user-payer`, `--contributor`), the verb MAY also accept the literal + `"me"` and resolve it to the current payer's pubkey at execution time. + `"me"` resolution is a verb-level responsibility, not a validator + behavior; verbs that do not opt in will treat `"me"` as a literal code. +- Repeatable inputs use one flag per value (`--add a --add b`), not comma + lists. +- No env-var reads at the verb level. Anything an operator might set in + their environment is parsed at the binary's global-flag layer and + surfaced through `CliContext`. + +## Output conventions + +- Default output is a table. +- Every `get`, `list`, and read command MUST expose `--json`. The display + type MUST be `Serialize`. Pubkey fields use the shared stable + serializer. +- Commands MAY additionally expose `--json-compact` for single-line JSON. + The flag name is fixed. +- Mutating commands print the transaction signature and post-confirmation + status. +- All user-facing output flows through the writer passed to `execute`. + +## Global flags + +The binary owns these globals; modules MUST NOT redeclare them: + +| Flag | Purpose | +| ---- | ------- | +| `--env` | Primary config knob; selects deployment and resolves URLs, program IDs, and default service endpoints. | +| `--url` | DZ ledger RPC URL override (does NOT affect Solana L1). | +| `--ws` | DZ ledger WebSocket URL override. | +| `--solana-url` | Solana L1 RPC URL override (does NOT affect DZ ledger). | +| `--keypair` | Path to signer keypair file. | +| `--program-id` | Serviceability program ID override. | +| `--geo-program-id` | Geolocation program ID override. | +| `--sock-file` | Daemon Unix socket path override. | +| `--no-version-warning` | Suppress version-check banner. | +| `--log-verbose` | Diagnostic logging. Repeat for higher levels: once raises to `debug`, twice raises to `trace`. No short alias because `connect`/`disconnect` still own `-v`/`--verbose` for their own flags. | +| `--version`, `-V` | Print version and exit. | + +`--env` resolves through `doublezero-config`. Recognized values are +`mainnet-beta`/`m`, `testnet`/`t`, `devnet`/`d`, `local`/`l`. + +## Diagnostic logging + +Diagnostic output goes to **stderr** via `tracing`. Modules use the +standard log macros (`debug!`, `info!`, `warn!`, `error!`, `trace!`) for +anything that explains what a verb is doing internally: backend requests, +retries, pubkey-or-code resolution, polling progress. + +```rust +tracing::debug!(env = %ctx.env, code = %self.code, "location get"); +``` + +Modules MUST NOT call `init_subscriber` themselves; the binary calls +`doublezero_cli_core::init_logging(verbosity)` once at startup. The +`RUST_LOG` env var overrides verbosity for per-module filtering. + +JSON output on stdout stays parseable at every verbosity level because logs +go to stderr. + +## Reference verb: `location get` + +`smartcontract/cli/src/location/get.rs` is the worked example. It demonstrates +the conforming pattern end to end: + +```rust +use clap::Args; +use doublezero_cli_core::CliContext; +use doublezero_sdk::commands::location::get::GetLocationCommand; +use serde::Serialize; +use std::io::Write; +use tabled::Tabled; + +use crate::{doublezerocommand::CliCommand, validators::validate_pubkey_or_code}; + +#[derive(Args, Debug)] +pub struct GetLocationCliCommand { + /// Location Pubkey or code to get details for + #[arg(long, value_parser = validate_pubkey_or_code)] + pub code: String, + /// Output as JSON + #[arg(long)] + pub json: bool, +} + +#[derive(Tabled, Serialize)] +struct LocationDisplay { /* ... */ } + +impl GetLocationCliCommand { + pub async fn execute( + self, + ctx: &CliContext, + client: &C, + out: &mut W, + ) -> eyre::Result<()> { + tracing::debug!(env = %ctx.env, code = %self.code, "location get"); + + let (pubkey, location) = client.get_location(GetLocationCommand { + pubkey_or_code: self.code, + })?; + + let display = LocationDisplay { /* ... */ }; + if self.json { + writeln!(out, "{}", serde_json::to_string_pretty(&display)?)?; + } else { + // render table via Tabled + } + Ok(()) + } +} +``` + +Unit test (excerpt): + +```rust +use doublezero_cli_core::testing::cli_context_default_for_tests; + +let ctx = cli_context_default_for_tests(); +let mut output = Vec::new(); +let res = block_on( + GetLocationCliCommand { code: "test".into(), json: true } + .execute(&ctx, &client, &mut output), +); +assert!(res.is_ok()); +``` + +The test uses `MockCliCommand` (auto-generated by `#[automock]` on the +`CliCommand` trait) as the backend, and the shared +`cli_context_default_for_tests()` helper from +`doublezero_cli_core::testing` to build a `CliContext` with sensible +defaults. + +## Preflight checks + +Verbs MAY call `RequirementCheck` to gate on common preconditions: + +```rust +use doublezero_cli_core::RequirementCheck; + +let checks = RequirementCheck::KEYPAIR | RequirementCheck::BALANCE; +``` + +The bitflags align with the legacy `CHECK_ID_JSON | CHECK_BALANCE | +CHECK_FOUNDATION_ALLOWLIST` `u8` constants in +`smartcontract/cli/src/requirements.rs`: + +| Flag | Bit | +| ---- | --- | +| `RequirementCheck::KEYPAIR` | `0b001` | +| `RequirementCheck::BALANCE` | `0b010` | +| `RequirementCheck::FOUNDATION_ALLOWLIST` | `0b100` | + +The actual `check_requirements` function lives with the module that owns +the typed backend client (today, `smartcontract/cli/src/requirements.rs`). +The bitflag type is shared so future modules consume the same canonical +set. + +## Authorization + +Authorization is **onchain**. The CLI is a thin client. The program +rejects unauthorized signers; the CLI surfaces the error. Modules MUST NOT +gate verbs by inspecting the caller's identity. + +## Migration is opportunistic + +RFC-20 explicitly grandfathers existing CLI surfaces. Existing verbs keep +their current shape until they are touched for unrelated work. New verbs +MUST conform from day one. When you touch a legacy verb, prefer to +migrate it to the conforming pattern; if migration would balloon the +change, leave it for a follow-up and note it in the PR description. + +## Open follow-ups + +Tracked in RFC-20 §Open Questions and in this work's plan: + +- Serviceability `Command` enum lives in the binary today; future PR moves + it into `smartcontract/cli` with `#[command(flatten)]` mounting. +- Geolocation module crate (defer per current scope). +- Daemon-control verbs (Connect, Status, Enable, Disable, Latency, Routes) + become their own module crate. +- JSON schema versioning once `--json` is a stable contract. +- Shell-completion install location. diff --git a/rfcs/rfc20-cli-standardization.md b/rfcs/rfc20-cli-standardization.md index 0d7006f1a8..d48e03bd79 100644 --- a/rfcs/rfc20-cli-standardization.md +++ b/rfcs/rfc20-cli-standardization.md @@ -110,7 +110,7 @@ Modules MUST NOT mutate `CliContext` and MUST NOT re-resolve any value from `--e - **Short aliases on booleans only.** Boolean toggles MAY declare a single-letter short alias. Non-boolean flags MUST NOT use short aliases. -- **Identifiers accept both pubkey and code.** Any flag that references an onchain entity MUST accept either a Solana pubkey or the entity's human-readable code via the shared validator. The magic value `"me"` resolves to the current payer's pubkey at execution time. +- **Identifiers accept both pubkey and code.** Any flag that references an onchain entity MUST accept either a Solana pubkey or the entity's human-readable code via the shared validator. Where a flag denotes a signer or payer-scoped entity (for example `--administrator`, `--user-payer`, `--contributor`), the verb MAY also accept the literal `"me"` and resolve it to the current payer's pubkey at execution time. `"me"` resolution is a verb-level responsibility, performed in the verb's `execute` path using the payer pubkey from `CliContext`; the shared validators only enforce grammar. Verbs that do not opt in will treat `"me"` as a literal code. - **Repeatable inputs use one flag per value.** A list of permissions is `--add perm1 --add perm2`, not `--add perm1,perm2`. Exception: values that are naturally lists (such as CIDR prefix lists) MAY use a typed list parser. @@ -136,7 +136,7 @@ Modules MUST NOT mutate `CliContext` and MUST NOT re-resolve any value from `--e Diagnostic output is separate from user-facing output and goes to standard error through the shared logging facade in the CLI core crate. Modules use the standard log macros (`debug!`, `info!`, `warn!`, `error!`, and `trace!` when finer granularity is justified) for anything that explains what a verb is doing internally: backend requests issued, retries, resolution of pubkey-or-code arguments, polling progress, and similar. -The binary configures the global log level from `--verbose`: warnings and errors only by default, `debug` when `--verbose` is set, and `trace` when `-vv` is set. Modules MUST NOT set or override the log level themselves and MUST NOT use `println!` or `eprintln!` for diagnostics. JSON output remains parseable regardless of `--verbose` because diagnostic logs go to stderr and the user-facing writer goes to stdout. +The binary configures the global log level from `--log-verbose`: warnings and errors only by default, `debug` when `--log-verbose` is set once, and `trace` when set twice (`--log-verbose --log-verbose`). The flag is spelled `--log-verbose` rather than `--verbose, -v` because `connect` and `disconnect` still own their own per-subcommand `--verbose` (`-v`) flags from earlier releases; a future RFC may deprecate those and reclaim the shorter spelling. Modules MUST NOT set or override the log level themselves and MUST NOT use `println!` or `eprintln!` for diagnostics. JSON output remains parseable regardless of `--log-verbose` because diagnostic logs go to stderr and the user-facing writer goes to stdout. ### Environments and configuration resolution @@ -168,7 +168,7 @@ The unified binary owns the following global flags, propagated to every subcomma | `--geo-program-id` | Geolocation program ID override. | | `--sock-file` | Daemon Unix socket path override. | | `--no-version-warning` | Suppress the version-check banner. | -| `--verbose`, `-v` | Enable diagnostic logging at `debug` level. Repeating (`-vv`) MAY raise the level to `trace`. | +| `--log-verbose` | Enable diagnostic logging. Repeating (`--log-verbose --log-verbose`) raises the level from `debug` to `trace`. No short alias yet because `connect`/`disconnect` still own `-v`/`--verbose` for legacy per-subcommand flags. | | `--version`, `-V` | Print the binary version and exit. | The DZ-ledger and Solana-L1 transports use separate override flags by design: confusing the two leads to verbs that quietly run against the wrong network. When `--env` is set, all transports resolve consistently; when an override is needed for one transport, the others continue to follow `--env`.