fix: rework cli for consistency and UX#636
Conversation
WalkthroughRefactors multiple CLI commands to centralize CLI value-name/env constants, reshape Args structs (adding ledger_dir, chain_dir, network, force, etc.), replace file-based nonce/snapshot imports with embedded/defaults, update bootstrap signature and logging, and thread explicit directories (e.g., headers_dir) through fetch/import flows. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (fetch-chain-headers)
participant Peer as ChainSyncPeer
participant FS as Filesystem (headers_dir)
CLI->>FS: ensure headers_dir exists
CLI->>Peer: connect and request headers starting from points
Peer-->>CLI: send header chunk
CLI->>FS: write header file into headers_dir
CLI->>Peer: request next block/point
Peer-->>CLI: send header chunk
CLI->>FS: write header file (repeat until done)
CLI->>FS: finalize/rotate files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 LanguageToolengineering-decision-records/019-guidelines-for-writing-cli.md[style] ~14-~14: To elevate your writing, try using a synonym here. (HARD_TO) [style] ~26-~26: To elevate your writing, try using a synonym here. (HARD_TO) [style] ~52-~52: To elevate your writing, try using a synonym here. (HARD_TO) 🪛 markdownlint-cli2 (0.18.1)engineering-decision-records/019-guidelines-for-writing-cli.md109-109: Emphasis style (MD049, emphasis-style) 109-109: Emphasis style (MD049, emphasis-style) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs (2)
383-392: Critical: Test won't compile -target_diris nowPathBuf, notOption<PathBuf>.Oi, mate! The pipeline's throwing a wobbly here. The
Argsstruct got refactored to usetarget_dir: PathBuf(with a default of"."), but this test is still wrapping it inSome(...). It's like trying to fit a square peg in a round hole - the compiler's not having it!🐛 Proposed fix
for snapshot in snapshots { let args = super::Args { + network, snapshot, - target_dir: Some(tempdir.path().to_path_buf()), - network, + target_dir: tempdir.path().to_path_buf(), }; run(args)
434-444: Same compilation error here - needs the same fix.Déjà vu! Same story, different test. The
target_dirfield needs to drop theSome(...)wrapper.🐛 Proposed fix
for snapshot in snapshots { let args = super::Args { + network, snapshot, - target_dir: Some(target_dir.clone()), - network, + target_dir: target_dir.clone(), }; run(args)
🤖 Fix all issues with AI agents
In `@crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs`:
- Around line 227-236: The call to std::fs::create_dir_all is passing a needless
borrow of headers_dir (headers_dir is already a &Path); remove the ampersand so
call std::fs::create_dir_all(headers_dir) instead, keeping the same
inspect_err/error handling and map_err( |_| WorkerError::Panic ) surrounding it
to preserve behavior.
In `@crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs`:
- Around line 65-74: The doc for the snapshot_dir field is inaccurate: it claims
a default per-network directory when in fact run() returns IncorrectUsage if
both snapshot and snapshot_dir are omitted; update the doc comment on
snapshot_dir to state that omitting both snapshot and snapshot_dir will cause an
IncorrectUsage error (refer to the snapshot_dir struct field and the run()
function which performs the check/returns IncorrectUsage), or alternatively
implement the intended default behavior inside run() by deriving the per-network
snapshots directory when snapshot_dir is None before validating and proceeding.
In `@crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs`:
- Around line 31-37: The doc comment above the ledger_dir field mistakenly says
"chain database"; update that comment to accurately reference the ledger
database (e.g., "The path to the ledger database to reset") so the documentation
matches the field name ledger_dir in reset_to_epoch.rs and avoids the
chain/ledger terminology mix-up.
In `@crates/amaru/src/bin/amaru/cmd/run.rs`:
- Around line 35-40: The chain_dir CLI arg is bound to the wrong environment
variable constant; update the arg definition for chain_dir (the struct field
chain_dir) to use amaru::env_vars::CHAIN_DIR instead of amaru::env_vars::NETWORK
so the AMARU_CHAIN_DIR env var is honored; locate the arg attributes on
chain_dir in the run.rs file and change the env = amaru::env_vars::NETWORK to
env = amaru::env_vars::CHAIN_DIR.
In `@crates/amaru/src/lib.rs`:
- Around line 95-102: The module-level doc comment for value_names is truncated
("aligned with:"); update the comment for pub mod value_names to complete the
thought by either specifying the intended reference (e.g., "aligned with clap's
naming conventions" or your project's CLI style guide) or by removing the
dangling fragment and finishing the sentence so the doc reads as a complete
convention description.
In `@engineering-decision-records/019-guidelines-for-writing-cli.md`:
- Line 85: Replace the misspelled word "objets" with "objects" in the ADR text
(the occurrence currently reads "informative as possible, and identical for
options that refer to objets of the"); update that token to "objects" so the
sentence reads correctly.
- Around line 175-176: Update the markdown link for the PR so GitHub resolves
correctly: change the URL in the text "Various command-line changes introduced
via [amaru#636](https://github.com/pragma-org/amaru/pulls/636) to comply with
these guidelines." to use /pull/ instead of /pulls/ (i.e.,
https://github.com/pragma-org/amaru/pull/636), keeping the link text "amaru#636"
unchanged.
🧹 Nitpick comments (3)
crates/amaru/src/bin/amaru/main.rs (1)
141-152: Minor consistency nit: match arms aren't alphabetically ordered.The PR mentions ordering commands alphabetically for consistency - the enum variants above nail it, but these match arms are vibin' to their own beat. It's like having your vinyl collection sorted on the shelf but playing tracks in shuffle mode!
Not a deal-breaker by any means, just a wee polish opportunity if you're keen.
💅 Proposed alphabetical ordering
let result = match args.command { - Command::Run(args) => cmd::run::run(args, metrics).await, - Command::ImportLedgerState(args) => cmd::import_ledger_state::run(args).await, - Command::ImportHeaders(args) => cmd::import_headers::run(args).await, - Command::ImportNonces(args) => cmd::import_nonces::run(args).await, Command::Bootstrap(args) => cmd::bootstrap::run(args).await, - Command::FetchChainHeaders(args) => cmd::fetch_chain_headers::run(args).await, Command::ConvertLedgerState(args) => cmd::convert_ledger_state::run(args).await, Command::DumpChainDB(args) => cmd::dump_chain_db::run(args).await, + Command::FetchChainHeaders(args) => cmd::fetch_chain_headers::run(args).await, + Command::ImportHeaders(args) => cmd::import_headers::run(args).await, + Command::ImportLedgerState(args) => cmd::import_ledger_state::run(args).await, + Command::ImportNonces(args) => cmd::import_nonces::run(args).await, Command::MigrateChainDB(args) => cmd::migrate_chain_db::run(args).await, Command::ResetToEpoch(args) => cmd::reset_to_epoch::run(args).await, + Command::Run(args) => cmd::run::run(args, metrics).await, };crates/amaru/src/bin/amaru/cmd/import_nonces.rs (1)
66-70: Optional: Consider async file read for consistency.The logic here is spot on, mate! However, you're using
std::fs::read(synchronous) in an async function, while other commands likeconvert_ledger_state.rsusetokio::fs. It's a small file so it won't block the runtime for long, but if you're keen on consistency, async would be the way to go.Not a biggie for a wee JSON file - totally your call!
🔧 Async alternative
let initial_nonces = if let Some(manually_specified_file) = args.nonces_file { - serde_json::from_slice(std::fs::read(manually_specified_file)?.as_slice())? + serde_json::from_slice(tokio::fs::read(manually_specified_file).await?.as_slice())? } else { default_initial_nonces(network)? };crates/amaru/src/bin/amaru/cmd/import_headers.rs (1)
83-90: Consider adding file context to read errors.When
fs::readfails, the error won't tell you which file caused the drama – bit like getting a "something went wrong" without knowing what. Not a huge deal since there's usually only a few files, but it'd be nice for debugging.♻️ Optional improvement
let headers = if args.header_file.is_empty() { get_bootstrap_headers(network)?.collect::<Vec<_>>() } else { args.header_file .iter() - .map(fs::read) + .map(|path| { + fs::read(path).map_err(|e| { + format!("failed to read header file '{}': {}", path.display(), e) + }) + }) .collect::<Result<_, _>>()? };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
crates/amaru/src/bin/amaru/cmd/bootstrap.rscrates/amaru/src/bin/amaru/cmd/convert_ledger_state.rscrates/amaru/src/bin/amaru/cmd/dump_chain_db.rscrates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rscrates/amaru/src/bin/amaru/cmd/import_headers.rscrates/amaru/src/bin/amaru/cmd/import_ledger_state.rscrates/amaru/src/bin/amaru/cmd/import_nonces.rscrates/amaru/src/bin/amaru/cmd/migrate_chain_db.rscrates/amaru/src/bin/amaru/cmd/reset_to_epoch.rscrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bootstrap.rscrates/amaru/src/lib.rsengineering-decision-records/019-guidelines-for-writing-cli.md
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru/src/bin/amaru/cmd/dump_chain_db.rscrates/amaru/src/bin/amaru/cmd/reset_to_epoch.rscrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/amaru/cmd/bootstrap.rscrates/amaru/src/bootstrap.rscrates/amaru/src/bin/amaru/cmd/convert_ledger_state.rscrates/amaru/src/bin/amaru/cmd/migrate_chain_db.rscrates/amaru/src/bin/amaru/cmd/import_headers.rscrates/amaru/src/bin/amaru/cmd/import_ledger_state.rscrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/lib.rscrates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rscrates/amaru/src/bin/amaru/cmd/import_nonces.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rscrates/amaru/src/bin/amaru/cmd/run.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/amaru/cmd/run.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/bootstrap.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2026-01-11T20:05:19.348Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/chainsync/responder.rs:191-213
Timestamp: 2026-01-11T20:05:19.348Z
Learning: In crates/amaru-protocols/src/chainsync/responder.rs, the chainsync responder intentionally does not support serving headers when the tip is Origin. Amaru is not designed to cold-start a new Cardano blockchain, so the intersect() function correctly fails when tip is Origin without needing special handling. This is a conscious design decision.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2025-12-28T19:26:35.354Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173
Timestamp: 2025-12-28T19:26:35.354Z
Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants.
<!--
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
🧬 Code graph analysis (10)
crates/amaru/src/bin/amaru/cmd/dump_chain_db.rs (3)
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs (1)
run(84-108)crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (1)
run(77-101)crates/amaru/src/lib.rs (1)
default_chain_dir(47-49)
crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs (1)
crates/amaru/src/lib.rs (1)
default_ledger_dir(43-45)
crates/amaru/src/bin/amaru/main.rs (9)
crates/amaru/src/observability.rs (1)
setup_observability(320-349)crates/amaru/src/bin/amaru/cmd/bootstrap.rs (1)
run(58-95)crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs (1)
run(68-80)crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs (1)
run(84-108)crates/amaru/src/bin/amaru/cmd/import_headers.rs (1)
run(64-93)crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (1)
run(77-101)crates/amaru/src/bin/amaru/cmd/import_nonces.rs (1)
run(52-77)crates/amaru/src/bin/amaru/cmd/migrate_chain_db.rs (1)
run(46-80)crates/amaru/src/bin/amaru/cmd/run.rs (1)
run(129-155)
crates/amaru/src/bin/amaru/cmd/bootstrap.rs (2)
crates/amaru/src/bootstrap.rs (1)
bootstrap(153-172)crates/amaru/src/lib.rs (2)
default_chain_dir(47-49)default_ledger_dir(43-45)
crates/amaru/src/bootstrap.rs (1)
crates/amaru/src/lib.rs (3)
default_initial_nonces(67-75)default_snapshots_dir(55-57)get_bootstrap_file(77-83)
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs (1)
crates/amaru/src/bootstrap.rs (1)
bootstrap(153-172)
crates/amaru/src/bin/amaru/cmd/import_headers.rs (1)
crates/amaru/src/lib.rs (1)
default_chain_dir(47-49)
crates/amaru/src/bin/amaru/cmd/run.rs (2)
crates/amaru/src/metrics.rs (1)
track_system_metrics(26-63)crates/amaru/src/lib.rs (2)
default_ledger_dir(43-45)default_chain_dir(47-49)
crates/amaru/src/lib.rs (1)
crates/amaru/src/bootstrap.rs (1)
bootstrap(153-172)
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs (2)
crates/amaru/src/bootstrap.rs (1)
bootstrap(153-172)crates/amaru/src/lib.rs (1)
get_bootstrap_file(77-83)
🪛 GitHub Actions: Coding Practices
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs
[error] 385-385: mismatched types: expected PathBuf, found Option for target_dir: Some(tempdir.path().to_path_buf())
[error] 437-437: mismatched types: expected PathBuf, found Option for target_dir: Some(target_dir.clone())
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
[error] 228-228: clippy: needless borrow for generic args. The call to std::fs::create_dir_all(&headers_dir) may be better as std::fs::create_dir_all(headers_dir).
🪛 LanguageTool
engineering-decision-records/019-guidelines-for-writing-cli.md
[style] ~14-~14: To elevate your writing, try using a synonym here.
Context: ... key to build good interface; and it is hard to get consistency unless guidelines an...
(HARD_TO)
[style] ~26-~26: To elevate your writing, try using a synonym here.
Context: ...ts. However, environment variables are hard to document. Command-line options, on t...
(HARD_TO)
[style] ~52-~52: To elevate your writing, try using a synonym here.
Context: ... command. It can be hard to figure out what exactly gets used as...
(HARD_TO)
[grammar] ~85-~85: Ensure spelling is correct
Context: ...and identical for options that refer to objets of the same nature.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
engineering-decision-records/019-guidelines-for-writing-cli.md
109-109: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
109-109: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 60, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 60, 10.5.3)
- GitHub Check: Benches
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/macos
- GitHub Check: Test coverage
- GitHub Check: Test the CLI
- GitHub Check: Analyze (rust)
🔇 Additional comments (31)
engineering-decision-records/019-guidelines-for-writing-cli.md (1)
1-180: Ripper of a doc, love the structure!This EDR is a solid piece of work – like having a proper game manual instead of figuring things out as you go. The guidelines are clear, well-organized with the collapsible sections, and the code examples really drive the points home. Following these conventions will make the CLI experience way more consistent for users. Good on ya!
crates/amaru/src/bin/amaru/cmd/dump_chain_db.rs (2)
28-46: Args struct looking fresh!The new
chain_diroption with propervalue_nameandenvattributes, plus the centralized constants fornetwork– this is exactly what the EDR ordered. Consistent as a metronome, mate!
48-58: Structured logging done rightLove the new logging format with
_command,chain_dir, andnetworkfields – follows the EDR guidelines to a T. Theunwrap_or_elsepattern for deriving defaults from network is clean and consistent with the other commands. Like a well-choreographed scene in a Tarantino flick!crates/amaru/src/bin/amaru/cmd/migrate_chain_db.rs (2)
26-44: Args struct nicely aligned with the new conventionsSwitched over to the centralized
value_namesandenv_varsconstants – this is the way, as the Mandalorian would say. The added doc comment fornetworkis a nice touch too.
53-58: Logging format is spot onThe structured log with
_command = "migrate-chain-db"and the relevant fields before the "running" message – chef's kiss! Matches the EDR conventions perfectly.crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs (3)
24-29: Env var on a positional argument – intentional?The
epochis a positional argument (nolongattribute), but it has anenvbinding. Per the EDR guideline 7, positional args are typically for mandatory values that users must always provide explicitly. Having an env var fallback here might let users accidentally run this destructive reset operation without realizing they've gotAMARU_EPOCHset in their environment.Given this command deletes epoch data, you might want to reconsider whether the env var is appropriate here, or if it's intentional to allow this workflow.
49-60: Structured logging and default derivation – noice!The
ledger_dirderivation from network and the structured logging format are both consistent with the other commands. Clean implementation, mate!
120-122: Redundant.clone()on the stringMinor thing –
title.to_lowercase()already returns a newString, so the.clone()is unnecessary. Like carrying a spare key when you've already got one in your hand!- let mut lower = title.to_lowercase().clone(); + let mut lower = title.to_lowercase();Wait, this is in
dump_chain_db.rsnot this file – ignore me, wrong file context! This line doesn't exist inreset_to_epoch.rs.crates/amaru/src/bootstrap.rs (3)
152-172: Bootstrap API simplification – sweet as!Removing
snapshots_dirfrom the public API and computing it internally viadefault_snapshots_dir(network)is a solid move. Less for users to worry about, more "it just works" vibes – like when a game auto-configures graphics settings correctly!
200-230:import_noncesvisibility bump topub– makes senseChanging from
pub(crate)topubenables the fine-grained command usage mentioned in the PR objectives. Users can now runimport_noncesindependently with custom networks. The signature takingInitialNoncesdirectly instead of a file path is cleaner – no more filesystem coupling for the core logic.
164-164: The conversion looks solid—no worries hereThe
network.into()call at line 164 is properly convertingNetworkNameto&EraHistoryvia a well-implementedFromtrait. Each network (Mainnet, Preprod, Preview) gets its own static EraHistory constant, and custom testnets all use a sharedTESTNET_ERA_HISTORY—which is the right design choice. No dodgy lifetime shenanigans here; it's all safe as houses since you're grabbing references to constants, not temporaries. Like nailing a perfect combo in a fighting game, the conversion is crispy and correct.crates/amaru/src/bin/amaru/main.rs (1)
39-107: Lookin' good, mate! The Command enum is properly structured.The expanded command surface with
FetchChainHeaders,ImportHeaders,ImportLedgerState,ImportNonces, andRunaligns nicely with the PR's goal of fine-grained control. Like unlocking all the side quests in an RPG instead of just having the main storyline!The variants are alphabetically ordered (Bootstrap → ConvertLedgerState → DumpChainDB → ... → Run), which is spot on for the consistency goals.
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs (2)
25-58: Args struct refactor looks solid.The streamlined
Argswith centralized constants fromamaru::value_namesandamaru::env_varsis exactly what the PR was aiming for. Clean and consistent, like a well-organized inventory system in Skyrim!The
snapshotbeing required (no default) makes perfect sense - ya can't convert thin air.
68-80: run() function is lookin' crisp.The structured logging with
_command = "convert-ledger-state"is consistent with other commands in the PR. No moreunwrap_or_elsedance sincetarget_dirhas a sensible default now. Chef's kiss! 🤌crates/amaru/src/bin/amaru/cmd/import_nonces.rs (2)
21-50: Args struct is well-structured with the newnonces_fileoption.Nice addition of the optional
nonces_fileparameter! This gives users fine-grained control to supply their own nonces JSON when needed - exactly what the PR set out to achieve. Like unlocking a secret character in a fighting game!The centralized
value_namesandenv_varsconstants keep everything consistent across the CLI.
59-64: Structured logging is consistent with other commands.The
_command = "import-nonces"pattern matches the other subcommands nicely. Consistency is key, like following the same art style across a game trilogy!crates/amaru/src/bin/amaru/cmd/bootstrap.rs (3)
21-56: Args struct is clean with the new--forceflag.The
--force/-fflag is a great UX addition! Prevents accidental data loss while still giving power users the control they need. Like having a "Are you sure?" prompt before deleting a save file, but with an option to skip it.Args fields are alphabetically ordered (chain_dir → force → ledger_dir → network) which aligns with PR goals. Noice!
78-92: Force logic is solid - nice safeguard!This is a proper "Confirm delete?" pattern. The warning when
ledger_direxists (without--force) is helpful for users who might accidentally try to re-bootstrap. And when they do mean it, the--forcepath handles cleanup gracefully.One tiny thing:
remove_dir_allis sync but you're in an async context. Not a drama for a bootstrap operation, just noting it for consistency's sake.
69-76: Structured logging is chef's kiss!Alphabetically ordered log fields (chain_dir → force → ledger_dir → network), consistent
_commandprefix, and the"running"message. It's all coming together like the final piece of a puzzle!crates/amaru/src/lib.rs (3)
67-75:default_initial_nonces()is a tidy helper!Centralizing the nonces loading logic here is smart - DRY principle in action! Other modules can just call this without duplicating the file-reading and JSON parsing logic. It's like having a shared loot chest in a co-op game.
The error handling with
BootstrapError::MissingConfigFileis consistent with how other bootstrap files are handled.
103-124: value_names constants are well-organized.Clear, descriptive constants that tell users exactly what format to expect.
NETWORK = "mainnet|preprod|preview|testnet_<U32>"is particularly helpful - shows all valid options at a glance. Like a good character select screen!
126-178: env_vars module is beautifully organized!Alphabetically ordered, consistent
AMARU_prefix, and each constant has a doc comment showing its corresponding CLI flag. This is exactly what the PR aimed for - every option now has a matching env var. It's like having a proper settings menu instead of scattered config files!This centralizes what was probably scattered across multiple files before. Solid refactor, mate!
crates/amaru/src/bin/amaru/cmd/run.rs (3)
42-127: Args struct looking sharp, mate!The CLI argument definitions are nicely consistent – using centralized
value_namesandenv_varsconstants throughout, proper defaults, and the multi-value peer_address handling withArgAction::Appendis spot on. Like a well-oiled machine, this is.
129-155: Meter provider integration – smooth as a flat white!The optional metrics tracking setup is clean – conditionally spawning the tracker and properly aborting it on exit. This is proper resource management, no leaks here. Good stuff!
168-180: Structured logging looking grand!The structured log fields are comprehensive and the
_commandtag for consistency across commands is a nice touch – makes log aggregation way easier. One tiny thing: thepid_filefield will show an empty string whenNone, which is fine but could potentially be"<none>"or similar for clarity. Not a blocker though, just a thought!crates/amaru/src/bin/amaru/cmd/import_headers.rs (1)
24-62: Args struct is solid – like finding a rare loot drop!The multi-file
header_filesupport withArgAction::Appendandvalue_delimiteris well-designed. Users can pass multiple--header-fileflags or comma-separate them – flexible as a yoga instructor. The defaults and env var bindings are consistent with the PR's goals.crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (2)
25-75: Args struct is well-designed – like a good RPG skill tree!The mutually exclusive
snapshotvssnapshot_diroptions usingconflicts_withis proper CLI hygiene. The multi-snapshot support withnum_args(0..)and the centralized env/value naming conventions are consistent. Clean separation of concerns here.
77-101: Run function logic is clean and clear!The three-way branching is easy to follow – snapshot files take priority, then snapshot directory, then error out if neither is provided. The structured logging with all the relevant fields upfront is proper observability practice. Good job!
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs (3)
35-82: Args struct is looking good – proper adventurer's loadout!The
headers_dirwith a sensible default, multi-valueparentsupport, and consistent env/value naming conventions. The docstrings are helpful too – explaining why you need the parent point to fetch headers is like getting a proper quest briefing before heading out.
84-108: Run function is well-structured!The fallback to bootstrap
headers.jsonwhen no parent is provided is smart – keeps the command usable out of the box while allowing power users to specify custom starting points. The structured logging is comprehensive. This is like having both auto-aim and manual controls in a shooter – best of both worlds!
139-175: Header fetching pipeline is solid!The
headers_diris properly threaded through all the helper functions – nice clean refactor. The progress bar integration and the fetch loop with proper Continue/Stop signaling is like a well-designed game loop. Tidy work!
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Mainly: - Align env vars across all command options, and make sure that every option consistently comes with one. - Align metavar / 'value_names' across all command options - Allow users to run fine-grained commands with their own options. This is the case in particular for import_headers, fetach_chain_headers and import_nonces. Some people may run on fully custom networks, therefore requiring fine-grained control over the holistic "bootstrap" method. - Order commands and options alphabetically. Less messy. - Fix, complete, and make more consistent the initial log event of all command. Signed-off-by: KtorZ <matthias.benkort@gmail.com>
KtorZ
force-pushed
the
command-line-consistency
branch
from
e4e26db to
5c171af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engineering-decision-records/019-guidelines-for-writing-cli.md`:
- Line 35: Fix the typo in the heading text that reads "Use a common prefix for
envionment variables (e.g. AMARU_)" by changing "envionment" to "environment" so
the heading becomes "Use a common prefix for environment variables (e.g.
AMARU_)"; update the exact string in the markdown (the heading line containing
"Use a common prefix for envionment variables") and keep the example prefix
"AMARU_" unchanged.
♻️ Duplicate comments (2)
engineering-decision-records/019-guidelines-for-writing-cli.md (1)
181-181: Still got that URL glitch, mate!This one slipped through the net – the link's still using
/pulls/instead of/pull/. GitHub won't be able to resolve it properly, like trying to load a save file from the wrong directory!🔗 Proposed fix
-- [amaru#636](https://github.com/pragma-org/amaru/pulls/636) +- [amaru#636](https://github.com/pragma-org/amaru/pull/636)crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (1)
65-75: Doc comment is a bit misleading, mate.The doc says "When omitted, defaults to a per-network snapshots directory based on the network name" but checking the
run()function (lines 94-100), if bothsnapshotandsnapshot_dirare omitted, it returnsIncorrectUsagerather than using any default. It's like the quest description promising treasure but the chest is actually empty!Either update the doc to reflect reality, or implement the default behavior.
📝 Suggested doc fix if no default is intended
/// Path to a directory containing multiple CBOR snapshots to import. - /// - /// When omitted, defaults to a per-network snapshots directory based on the network name. + /// + /// Either this or --snapshot must be provided. #[arg(
🧹 Nitpick comments (6)
engineering-decision-records/019-guidelines-for-writing-cli.md (1)
14-14: Optional style polish for the completionists out thereStatic analysis flagged a few minor style suggestions if you're keen on that extra level of polish:
- Lines 14, 26, 52: Could swap "hard to" for fancier synonyms like "difficult to" or "challenging to" (though honestly, "hard to" reads perfectly fine for tech docs)
- Line 109: Markdown emphasis uses asterisks when the linter prefers underscores for consistency
These are purely optional nitpicks – the doc's already solid as-is, mate!
Also applies to: 26-26, 52-52, 109-109
crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs (1)
120-121: Minor style nit: Consider using&Pathinstead of&PathBufin function signatures.Hey mate, just a wee suggestion – functions like
get_ledger_db_snapshotsandcopy_dir_recursivetake&PathBufbut idiomatically Rust prefers&Pathfor borrowed paths. It's like the difference between borrowing someone's whole backpack vs just the map inside – you only need what you need!This isn't blocking at all, just a "nice to have" for future reference.
♻️ Optional refactor
fn get_ledger_db_snapshots( - ledger_dir: &PathBuf, + ledger_dir: &Path, ) -> Result<Vec<Folder>, Box<dyn std::error::Error>> {-fn copy_dir_recursive(src: &PathBuf, dst: &PathBuf) -> io::Result<()> { +fn copy_dir_recursive(src: &Path, dst: &Path) -> io::Result<()> {crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (1)
82-92: Structured logging is grand, but there's a wee gotcha!The logging looks good with all the fields, but using
.unwrap_or_default()onsnapshot_dirwill log an empty string when it'sNone. Might be clearer to log"<not set>"or similar, or use theOptiondebug format. It's like a loading screen that just shows blank – technically works but could be more informative!Not a blocker, just something to consider for debugging clarity.
♻️ Optional: More explicit None handling
info!( _command = "import-ledger-state", ledger_dir = %ledger_dir.to_string_lossy(), network = %args.network, snapshot = ?args.snapshot, - snapshot_dir = %args.snapshot_dir - .clone() - .unwrap_or_default() - .to_string_lossy(), + snapshot_dir = ?args.snapshot_dir, "running", );crates/amaru/src/bootstrap.rs (1)
283-283: Minor style nit:&Vec<PathBuf>could be&[PathBuf].Hey mate, small suggestion – taking
&Vec<PathBuf>is fine, but idiomatic Rust prefers&[PathBuf](a slice) for borrowed collections. It's more flexible and doesn't tie you toVecspecifically. Like accepting any controller input rather than just one specific brand!Not blocking at all, just a wee polish opportunity.
♻️ Optional refactor
pub async fn import_snapshots( network: NetworkName, - snapshots: &Vec<PathBuf>, + snapshots: &[PathBuf], ledger_dir: &PathBuf, ) -> Result<(), Box<dyn std::error::Error>> {crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs (1)
383-392: Tests updated correctly for the new Args shape.The test now uses a concrete
target_dir: tempdir.path().to_path_buf()instead of an optional value. The field ordering in the test struct construction (snapshot,target_dir,network) doesn't match the struct definition order (network,snapshot,target_dir), but Rust handles named fields in any order, so this is purely stylistic.🔧 Optional: Match struct field order for consistency
for snapshot in snapshots { let args = super::Args { + network, snapshot, target_dir: tempdir.path().to_path_buf(), - network, };crates/amaru/src/bin/amaru/cmd/import_headers.rs (1)
64-70: Minor inconsistency:args.networkused after bindingnetworkvariable.On line 65, you bind
let network = args.network;, but on line 69 you useargs.networkin theunwrap_or_elseclosure. Not a bug since both refer to the same value (andNetworkNameisCopy), but usingnetworkwould be more consistent with the pattern used in other commands likeimport_nonces.rsandbootstrap.rs.🔧 Optional: Use the local `network` binding consistently
let chain_dir = args .chain_dir - .unwrap_or_else(|| default_chain_dir(args.network).into()); + .unwrap_or_else(|| default_chain_dir(network).into());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
crates/amaru/src/bin/amaru/cmd/bootstrap.rscrates/amaru/src/bin/amaru/cmd/convert_ledger_state.rscrates/amaru/src/bin/amaru/cmd/dump_chain_db.rscrates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rscrates/amaru/src/bin/amaru/cmd/import_headers.rscrates/amaru/src/bin/amaru/cmd/import_ledger_state.rscrates/amaru/src/bin/amaru/cmd/import_nonces.rscrates/amaru/src/bin/amaru/cmd/migrate_chain_db.rscrates/amaru/src/bin/amaru/cmd/reset_to_epoch.rscrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bootstrap.rscrates/amaru/src/lib.rsengineering-decision-records/019-guidelines-for-writing-cli.md
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rscrates/amaru/src/bin/amaru/cmd/import_nonces.rscrates/amaru/src/bin/amaru/cmd/dump_chain_db.rscrates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rscrates/amaru/src/bin/amaru/cmd/reset_to_epoch.rscrates/amaru/src/bin/amaru/cmd/import_ledger_state.rscrates/amaru/src/bin/amaru/cmd/migrate_chain_db.rscrates/amaru/src/lib.rscrates/amaru/src/bin/amaru/cmd/import_headers.rscrates/amaru/src/bin/amaru/cmd/bootstrap.rscrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/bootstrap.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru/src/bin/amaru/main.rscrates/amaru/src/bin/amaru/cmd/run.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In the amaru consensus pipeline, headers are now stored before validation (store_header stage runs before validate_header stage). The validity state of headers should be tracked in the database to avoid revalidating already-valid headers and to prevent flood attacks with duplicate invalid headers.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2026-01-11T20:05:19.348Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/chainsync/responder.rs:191-213
Timestamp: 2026-01-11T20:05:19.348Z
Learning: In crates/amaru-protocols/src/chainsync/responder.rs, the chainsync responder intentionally does not support serving headers when the tip is Origin. Amaru is not designed to cold-start a new Cardano blockchain, so the intersect() function correctly fails when tip is Origin without needing special handling. This is a conscious design decision.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 392
File: crates/ouroboros-traits/src/is_header/fake.rs:52-88
Timestamp: 2025-08-23T15:44:44.318Z
Learning: In the Amaru project, FakeHeader in crates/ouroboros-traits/src/is_header/fake.rs is specifically designed for testing purposes, and panic behavior (like copy_from_slice() panicking on malformed hex) is the preferred approach rather than graceful error handling, as it helps identify test data issues quickly during development.
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2025-12-28T19:26:35.354Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 612
File: crates/amaru-protocols/src/blockfetch/mod.rs:151-173
Timestamp: 2025-12-28T19:26:35.354Z
Learning: In crates/amaru-protocols/src/blockfetch/mod.rs, the blockfetch initiator uses .expect() when popping from the request queue on NoBlocks and Done results. These are intentional fail-fast assertions: the protocol state machine guarantees the queue is non-empty when these messages arrive, so an empty queue indicates a protocol violation. A misbehaving peer triggers an erroneous protocol transition that will close the connection (supervision to be implemented in a future PR). This follows the project's fail-fast philosophy for protocol invariants.
<!--
Applied to files:
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rscrates/amaru/src/bin/amaru/cmd/run.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/bootstrap.rs
🧬 Code graph analysis (11)
crates/amaru/src/bin/amaru/cmd/import_nonces.rs (2)
crates/amaru/src/bootstrap.rs (2)
bootstrap(153-172)import_nonces(200-230)crates/amaru/src/lib.rs (2)
default_chain_dir(47-49)default_initial_nonces(67-75)
crates/amaru/src/bin/amaru/cmd/dump_chain_db.rs (1)
crates/amaru/src/lib.rs (1)
default_chain_dir(47-49)
crates/amaru/src/bin/amaru/main.rs (11)
crates/amaru/src/bootstrap.rs (1)
import_nonces(200-230)crates/amaru/src/bin/amaru/cmd/bootstrap.rs (1)
run(58-95)crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs (1)
run(68-80)crates/amaru/src/bin/amaru/cmd/dump_chain_db.rs (1)
run(48-88)crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs (1)
run(84-108)crates/amaru/src/bin/amaru/cmd/import_headers.rs (1)
run(64-93)crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (1)
run(77-101)crates/amaru/src/bin/amaru/cmd/import_nonces.rs (1)
run(52-77)crates/amaru/src/bin/amaru/cmd/migrate_chain_db.rs (1)
run(46-80)crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs (1)
run(49-97)crates/amaru/src/bin/amaru/cmd/run.rs (1)
run(129-155)
crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs (2)
crates/amaru/src/bootstrap.rs (1)
bootstrap(153-172)crates/amaru/src/lib.rs (1)
get_bootstrap_file(77-83)
crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs (1)
crates/amaru/src/lib.rs (1)
default_ledger_dir(43-45)
crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (3)
crates/amaru/src/bootstrap.rs (1)
std(260-263)crates/amaru/src/lib.rs (1)
default_ledger_dir(43-45)crates/amaru/tests/summary.rs (1)
default_ledger_dir(36-38)
crates/amaru/src/lib.rs (1)
crates/amaru/src/bootstrap.rs (1)
bootstrap(153-172)
crates/amaru/src/bin/amaru/cmd/import_headers.rs (1)
crates/amaru/src/lib.rs (1)
default_chain_dir(47-49)
crates/amaru/src/bin/amaru/cmd/bootstrap.rs (2)
crates/amaru/src/bootstrap.rs (1)
bootstrap(153-172)crates/amaru/src/lib.rs (2)
default_chain_dir(47-49)default_ledger_dir(43-45)
crates/amaru/src/bin/amaru/cmd/run.rs (1)
crates/amaru/src/lib.rs (2)
default_ledger_dir(43-45)default_chain_dir(47-49)
crates/amaru/src/bootstrap.rs (1)
crates/amaru/src/lib.rs (3)
default_initial_nonces(67-75)default_snapshots_dir(55-57)get_bootstrap_file(77-83)
🪛 LanguageTool
engineering-decision-records/019-guidelines-for-writing-cli.md
[style] ~14-~14: To elevate your writing, try using a synonym here.
Context: ... key to build good interface; and it is hard to get consistency unless guidelines an...
(HARD_TO)
[style] ~26-~26: To elevate your writing, try using a synonym here.
Context: ...ts. However, environment variables are hard to document. Command-line options, on t...
(HARD_TO)
[style] ~52-~52: To elevate your writing, try using a synonym here.
Context: ... command. It can be hard to figure out what exactly gets used as...
(HARD_TO)
🪛 markdownlint-cli2 (0.18.1)
engineering-decision-records/019-guidelines-for-writing-cli.md
109-109: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
109-109: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 60, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 60, 10.5.3)
- GitHub Check: clippy
- GitHub Check: Build aarch64/macos
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Benches
- GitHub Check: Test the CLI
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (37)
crates/amaru/src/bin/amaru/cmd/dump_chain_db.rs (3)
17-25: LGTM on the import reorganization!The imports are nicely consolidated and grouped. Clean as a whistle, mate – like organizing your inventory before a boss fight.
28-46: Solid CLI argument standardization!G'day! The new
chain_diroption with centralizedvalue_names::DIRECTORYandenv_vars::CHAIN_DIRis spot on. The pattern of making it optional with a network-derived default is consistent with the other commands in this PR – like unlocking fast travel once you've discovered all the locations.
53-58: Structured logging looks grand!The new structured log format with
_command,chain_dir, andnetworkfields is mint. Consistent with the other command modules and makes log aggregation way easier – like having proper quest markers instead of just "go find it somewhere."crates/amaru/src/bin/amaru/cmd/reset_to_epoch.rs (2)
22-47: CLI arguments are looking proper now!The Args struct is nicely aligned with the centralized constants. The
epochpositional arg, optionalledger_dir, andnetworkwith default – all using the standardizedvalue_namesandenv_vars. It's like finally getting all your gear sets to match in an RPG!
49-62: Run function setup is clean!The pattern of computing
ledger_dirfrom args or falling back todefault_ledger_dir(args.network)is consistent with the other commands. The structured logging with_command = "reset-to-epoch"fits the bill perfectly. Fair dinkum work, mate!crates/amaru/src/bin/amaru/cmd/import_ledger_state.rs (3)
27-42: CLI standardization is looking ace!The
ledger_dirandnetworkarguments with centralized constants are consistent with the other commands. Nice one!
44-63: Multi-snapshot support is a beaut!Changing
snapshottoVec<PathBuf>withnum_args(0..)gives users flexibility to import multiple snapshots in one go. Theverbatim_doc_commentpreserves that nicely formatted example – like having a proper tutorial level before the main game.
94-100: Control flow is crystal clear!The branching logic is straightforward – check
snapshotfirst, thensnapshot_dir, else error. Easy to follow, like a dialogue tree with only three options.crates/amaru/src/bin/amaru/cmd/migrate_chain_db.rs (2)
26-44: Args struct is mint!The centralized
value_names::DIRECTORY,env_vars::CHAIN_DIR,value_names::NETWORK, andenv_vars::NETWORKconstants are all in place. The new doc comment fornetworkadds clarity. Consistent as a speed-runner's route!
53-58: Structured logging follows the pattern nicely!The
_command = "migrate-chain-db"withchain_dirandnetworkfields matches the other commands. Good stuff – like having all your HUD elements in the same spot across different games in a series.crates/amaru/src/bootstrap.rs (3)
15-15: New internal defaults imported!Using
default_initial_noncesanddefault_snapshots_dirfrom crate root keeps the bootstrap logic clean and centralized. Like having your loadout presets saved!
152-172: Bootstrap function streamlined nicely!The function now internally derives
snapshots_dirviadefault_snapshots_dir(network)and sources nonces fromdefault_initial_nonces(network)instead of reading from disk. This simplifies the public API while maintaining flexibility through the standalone commands. It's like the game offering both "Quick Play" and "Custom Match" options!One thing to verify – since
default_initial_noncescan return anErr, and you're using?on line 164, any error will bubble up. That's probably fine, but worth noting for callers.
200-230:import_noncesis now public – spot on!Bumping the visibility from
pub(crate)topubis the right move here. The standaloneimport_noncescommand over incrates/amaru/src/bin/amaru/cmd/import_nonces.rsneeds direct access to this function, and it was always gonna be a bit dodgy trying to work around that. The function itself is clean as a whistle – opens the chain DB, computes the epoch from the era history, builds theNoncesstruct, and persists it. No drama there, mate!The usage pattern checks out: you've got the internal call during bootstrap (line 164) and the external command hook (line 76), so this is legit intentional. Good call making it part of the public API surface.
crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs (2)
25-58: Nice one, mate! The Args struct is looking clean and consistent.The centralized
value_namesandenv_varsusage is spot on — like finding all the loot in one chest instead of scattered across the map. Default fortarget_dirto"."is a sensible UX choice too.One small observation: per the PR objectives about alphabetical ordering, the fields here are
network,snapshot,target_dirwhich is alphabetically ordered. Good stuff!
69-75: Structured logging is looking sharp!The
_commandfield pattern for identifying which command is running is consistent with the other commands in the PR. Like a well-organized inventory system in an RPG — everything's where you'd expect it to be.crates/amaru/src/bin/amaru/main.rs (2)
39-107: Command enum is lookin' deadly, mate!The alphabetical ordering of variants is on point — Bootstrap through Run, like a perfectly sorted game menu. The
verbatim_doc_commenton Bootstrap is a nice touch for preserving that formatted documentation. The aliases (import-chain-dbfor ImportHeaders,importfor ImportLedgerState,daemonfor Run) are solid for backward compatibility and user convenience.Like the saying goes: "It's dangerous to go alone!" — but with these clear command docs, users won't be lost.
141-152: All command dispatch paths are wired up correctly.Every variant in the
Commandenum has a corresponding arm in the match statement. The flow is clean and each command module'srunfunction is invoked properly.crates/amaru/src/bin/amaru/cmd/bootstrap.rs (2)
21-56: Args struct is well organized with alphabetical field ordering.The centralized
value_namesandenv_varsconstants are consistently applied. Theforceflag with short-fis a nice UX touch for power users — like having a quick-save shortcut in a game.
78-92: The--forceflag logic is clear, but the silent success on existing dir might surprise some users.When
ledger_direxists and--forceisn't set, the command logs a warning and returnsOk(()). This is a valid idempotent design choice (like how some installers say "already installed, skipping"), but it might be worth considering whether this should be more explicit in the help text or return a non-zero exit in strict modes.That said, for scripting and automation, this "no-op on existing" behavior is actually quite handy. Like respawning at a checkpoint instead of starting the whole level over.
crates/amaru/src/bin/amaru/cmd/import_nonces.rs (3)
21-50: Args struct looks good with the newnonces_fileoption.Giving users the ability to specify a custom nonces file is a solid addition for custom network support. The doc comment explaining the fallback behavior is helpful — users will know what to expect when they don't provide the flag.
66-70: The nonces loading logic is straightforward.The branching is clean: manual file takes precedence, otherwise fall back to network defaults. One small note —
std::fs::readis synchronous I/O within an async function. For small config files like nonces JSON, this is generally fine and won't block the event loop noticeably. If this ever needed to handle larger files, you'd want to reach fortokio::fs::read, but for now, she'll be right.
72-76: Heads up: The FIXME comment documents a known design limitation with testnet EraHistory handling.The conversion
args.network.into()is type-safe and works exactly as intended—it leverages aFrom<NetworkName> for &EraHistoryimplementation that directly returns a reference to the appropriate static constant (&MAINNET_ERA_HISTORY,&PREPROD_ERA_HISTORY,&PREVIEW_ERA_HISTORY, or&TESTNET_ERA_HISTORY). This matches theimport_noncesfunction signature perfectly, so there's no borrowed/owned mismatch here—like a well-oiled speedboat engine, the types align beautifully.The FIXME itself is solid documentation of a separate architectural concern: relying on a statically-defined testnet EraHistory can be problematic depending on your use case. That's worth keeping an eye on, but the code as written here is doing its job without issue.
crates/amaru/src/bin/amaru/cmd/import_headers.rs (2)
40-52: Theheader_fileargument configuration is flexible and user-friendly.Using
ArgAction::Appendwithvalue_delimiter = ','gives users multiple ways to specify files:
--header-file a.cbor --header-file b.cbor--header-file a.cbor,b.cborThat's like having both keyboard and controller support — everyone's happy! The
num_args(0..)allows zero or more values, which works well with the fallback to bootstrap headers.
83-92: Header loading logic is clean and follows the established pattern.The branching mirrors the approach in
import_nonces.rs: if no files specified, fall back to bootstrap defaults; otherwise, load from provided files. Thecollect::<Result<_, _>>()?pattern for propagating I/O errors from multiple file reads is idiomatic Rust.Like finding all the collectibles in one go — efficient and satisfying!
crates/amaru/src/lib.rs (4)
15-15: LGTM!Import cleanly pulls in the types needed for the new
default_initial_noncesfunction. No worries here, mate!
67-75: LGTM!Nice one! This function follows the same patterns as other bootstrap file helpers in the crate. The error handling is clean and the
Box<dyn Error>return type keeps things flexible for callers – like choosing your loadout before a boss fight, you've got options!
95-123: LGTM!This is cleaner than a freshly waxed surfboard! The value_names module provides a single source of truth for CLI metavars – no more hunting through files like trying to find that one save point in Dark Souls. The conventions are clearly documented and consistently applied.
125-177: LGTM!Top shelf work here! Centralizing env vars like this is the CLI equivalent of a well-organized inventory system – you always know where to find what you need. The
AMARU_prefix keeps everything namespaced nicely, and those doc comments linking back to CLI options are chef's kiss. It's giving "I actually care about maintainability" energy.crates/amaru/src/bin/amaru/cmd/fetch_chain_headers.rs (6)
34-82: LGTM!The
Argsstruct is looking proper tidy! Using the centralizedvalue_namesandenv_varsconstants keeps things DRY – like having a shared stash box in a co-op game. Theparentfield configuration withArgAction::Appendand comma delimiter gives users flexibility to specify multiple points, which is a nice UX touch.
84-108: LGTM!Sweet implementation! The fallback logic for
parentis like having an auto-save – users get sensible defaults but can override when they need control. The structured logging at the start makes debugging a breeze. It's giving "I've played enough games to know good UX when I see it" vibes.
110-137: LGTM!Parameter threading done right –
headers_dirflows through cleanly. Using&Pathas the parameter type is the idiomatic choice here.
139-175: LGTM!The
headers_dirparameter flows through to the inner functions nicely. No drama here!
183-210: LGTM!Both helper functions properly thread the
headers_dirparameter through tohandle_response. Clean pass-through with no surprises – like a well-executed relay race.
212-281: LGTM!The
handle_responsefunction is looking solid! The error handling withinspect_errfor logging before mapping toWorkerError::Panicis a nice pattern – you get visibility into failures without losing the error flow. I see the past review's needless borrow note was sorted out too. Good stuff!The
unwrap()on line 222 for CBOR deserialization has the appropriate#[allow(clippy::unwrap_used)]attribute – per project conventions, fail-fast on malformed protocol data is the preferred approach here.crates/amaru/src/bin/amaru/cmd/run.rs (3)
30-127: LGTM!The
Argsstruct is now properly kitted out! All the env vars are pointing to the right constants (I see thatchain_dir→CHAIN_DIRfix went through – no more accidental crouch-jumping!). The alphabetical ordering makes this read like a well-organized game settings menu. UsingArgAction::Appendwith comma delimiter forpeer_addressgives users the flexibility to connect to multiple upstream peers in whatever way suits them.
129-155: LGTM!The
runfunction is structured like a proper game loop – setup, main execution, and cleanup on exit. The metrics handle abort on shutdown is a nice touch for resource cleanup.
157-194: LGTM!The
parse_argsfunction nails the defaults pattern – when users don't specifyledger_dirorchain_dir, they get sensible network-based defaults like./ledger.preprod.db. The structured logging gives operators full visibility into the config at startup, which is clutch for debugging – like having a "show all stats" screen before starting a new run.The
Configconstruction cleanly maps all the args fields, and using..Config::default()for any remaining fields keeps things future-proof.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jeluard
force-pushed
the
command-line-consistency
branch
from
11b170e to
e5a8010
Compare
2 participants
Mainly:
Align env vars across all command options, and make sure that every option consistently comes with one.
Align metavar / 'value_names' across all command options
Allow users to run fine-grained commands with their own options. This is the case in particular for import_headers, fetach_chain_headers and import_nonces. Some people may run on fully custom networks, therefore requiring fine-grained control over the holistic "bootstrap" method.
Order commands and options alphabetically. Less messy.
Fix, complete, and make more consistent the initial log event of all command.
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.