feat(cli): implement dkg

[iamquang95]

Should address: #314

This covers only the entry point of pluto dkg, not the full implementation of dkg.

[iamquang95]

@claude

[claude]

Claude finished @iamquang95's task in 3m 31s —— View job

---

Code Review — feat(cli): implement dkg

• Gather context — reading changed files
• Compare with Go (Charon) implementation
• Review CLI wiring and command structure
• Review DKG entry point implementation
• Review eth1wrap changes
• Post final review

---

Summary

This PR wires up the pluto dkg CLI entry point — argument parsing, config construction, and a preflight stub that calls load_definition, check_clear_data_dir, and check_writes before hitting an unimplemented!() boundary. The scope is appropriately limited and clearly stated. The factoring of shared relay/DKG helpers into commands/common.rs is clean. Most parity gaps below are expected given the stub nature; a few low-risk items warrant attention.

---

Findings

• [Medium] Operation order in dkg::run deviates from Go
  Impact: Go calls loadDefinition → validateKeymanagerFlags; Rust reverses this. When the definition fails to load, Go users see a definition error, not a keymanager error. Failing fast on keymanager flags is arguably better UX, but it's an undocumented divergence.
  Evidence: crates/dkg/src/dkg.rs:171-181
  Go reference: charon/dkg/dkg.go:154-169
  Recommendation: Add a comment explaining the intentional reordering (early-exit on misconfiguration), or track it as a known parity deviation.

• [Medium] has_test_config() is incomplete relative to Go
  Impact: Go's HasTestConfig checks 4 fields (StoreKeysFunc, SyncCallback, Def, P2PNodeCallback). Rust only checks test_config.def.is_some(). When future backend callbacks are added to TestConfig, the guard against running preflight in tests (check_clear_data_dir skip) may not fire correctly.
  Evidence: crates/dkg/src/dkg.rs:139-142
  Go reference: charon/dkg/dkg.go:84-86
  Recommendation: Leave a // TODO comment in has_test_config referencing the other fields that will need adding as the backend is ported.

• [Medium] DEFAULT_RELAYS duplicated across two crates
  Impact: crates/cli/src/commands/common.rs:15 and crates/dkg/src/dkg.rs:9 define the same three relay URLs. If one is updated without the other, CLI defaults and programmatic defaults silently diverge.
  Evidence: crates/cli/src/commands/common.rs:15, crates/dkg/src/dkg.rs:9
  Recommendation: Re-export or reference one from the other, e.g. have dkg::DEFAULT_RELAYS re-use pluto_cli's constant, or move the canonical constant to a shared crate (core or p2p::config).

• [Medium] --log-format accepted but silently ignored
  Impact: DkgLogArgs.format is parsed from CLI and stored, but into_config() only passes level and color to build_console_tracing_config. A user passing --log-format=json gets console output with no warning.
  Evidence: crates/cli/src/commands/dkg.rs:127, crates/cli/src/commands/common.rs:34-51
  Recommendation: Either wire format into TracingConfig (if supported), or add a // TODO and a warn! if a non-console format is requested.

• [Low] warn_for_insecure_relays uses string prefix instead of parsed multiaddr protocol
  Impact: The relay command checks for HTTP via Protocol::Http on the parsed multiaddr (correct). The DKG command does a raw string prefix check on the original relay string. A relay expressed as an HTTP multiaddr not written as a URL (e.g. /ip4/1.2.3.4/tcp/80/http) would not trigger the DKG warning.
  Evidence: crates/cli/src/commands/dkg.rs:294-300, crates/cli/src/commands/relay.rs:43-52
  Recommendation: Mirror the relay approach: parse first via parse_relay_addr, then check the multiaddr for Protocol::Http. Fix this →

• [Low] validate_keymanager_flags trims whitespace; Go does not
  Impact: Rust treats " " (whitespace-only) as empty for both address and auth_token. Go would treat them as non-empty and proceed to URL parsing, yielding a different error. Minor behavioral divergence on edge-case input.
  Evidence: crates/dkg/src/dkg.rs:191-192
  Go reference: charon/dkg/dkg.go:1165-1184
  Recommendation: Document intentional difference, or align with Go by removing .trim().

• [Low] run_executes_preflight_before_reaching_backend_boundary asserts is_panic()
  Impact: The test spawns a task and asserts is_panic() to verify the unimplemented!() boundary is hit. This will need to be replaced when the backend is implemented; it's a brittle proxy for "preflight passed".
  Evidence: crates/dkg/src/dkg.rs:284-296
  Recommendation: Track this as a TODO comment: // TODO: replace with a real assertion once the backend is implemented.

---

Parity Matrix

Component	Go	Rust	Match	Notes
--data-dir flag	✓ CHARON_DATA_DIR	✓	✓
--definition-file flag	✓ CHARON_DEFINITION_FILE	✓	✓
--no-verify flag	✓ CHARON_NO_VERIFY	✓	✓
--timeout flag (default 1m)	✓	✓	✓
--publish-timeout (default 30s)	✓	✓	✓
--shutdown-delay (default 1s)	✓	✓	✓
--keymanager-address	✓	✓	✓
--keymanager-auth-token	✓	✓	✓
--p2p-relays	✓	✓	✓
--zipped flag	✓	✓	✓
--publish flag	✓	✓	✓
--execution-client-rpc-endpoint	✓	✓	✓
Keymanager validation error strings	✓	✓	✓	exact match
validateKeymanager: trim() behavior	strips nothing	strips whitespace	✗	minor edge case
Operation order: validate → load	load → validate	validate → load	✗	intentional early-exit
HasTestConfig guard	4 fields	1 field (def)	partial	other fields not yet ported
checkClearDataDir logic	✓	✓	✓
checkWrites logic	✓	✓	✓
Private key lock file	✓	✗	✗	expected stub gap
Definition version check	✓	✗	✗	expected stub gap
EthClient no-op on empty addr	✓ (via noop runner)	✓	✓

Tests

Tests observed in the diff — not executed locally (no cargo test run in this review):

• dkg_is_registered_as_top_level_subcommand — sanity-checks CLI wiring ✓
• dkg_defaults_match_go — validates all default values against Go ✓
• invalid_duration_fails_during_parse — clap parse-time validation ✓
• dkg_args_expose_expected_env_bindings — env var contract check ✓
• config_mapping_preserves_fields — round-trip CLI → Config ✓
• run_passes_config_and_token_to_runner — runner receives correct config ✓
• run_rejects_mismatched_keymanager_flags / run_executes_preflight_* / run_surfaces_data_dir_* — coverage of dkg::run preflight ✓
• empty_address_returns_noop_client — eth1wrap no-op path ✓

Test coverage is strong for the stub scope.

Open Questions / Assumptions

1. The log-format argument (console / logfmt / json) — is there a planned issue to wire this into TracingConfig? Both relay and DKG have the same TODO.
2. AppendConfig (Go's merge-two-DKG-ceremonies feature) — expected to remain absent until that feature is ported?
3. When adding the backend, will TestConfig be extended with callbacks (StoreKeysFunc, SyncCallback, etc.) at that point, or earlier?