refactor(cli): move CliOptions into a dedicated cli module by MegaRedHand · Pull Request #477 · lambdaclass/ethlambda

MegaRedHand · 2026-06-29T14:02:26Z

Why

bin/ethlambda/src/main.rs mixed the CLI flag surface (the ~80-line CliOptions clap struct) with node orchestration. Splitting them keeps each file single-purpose: the flag definitions live in one place, and main.rs reads as orchestration only.

What

New bin/ethlambda/src/cli.rs holding pub(crate) struct CliOptions (moved verbatim, docs and #[arg]/#[command] attributes intact). Fields are pub(crate) since main reads nearly all of them.
main.rs: registers mod cli;, imports CliOptions, drops the inline definition. No behavior change.

Verification

cargo build -p ethlambda ✅
cargo clippy -p ethlambda --all-targets -- -D warnings ✅
cargo fmt ✅

Pure code move; the parsed CLI surface is unchanged.

main.rs had grown to mix CLI parsing with node orchestration. Extract the CliOptions clap struct into its own bin/ethlambda/src/cli.rs so the flag surface lives in one place and main.rs reads as orchestration only.

github-actions · 2026-06-29T14:03:19Z

🤖 Kimi Code Review

This is a clean refactoring with no issues introduced.

Positive observations:

Module separation: Extracting CLI definitions into cli.rs improves maintainability and follows Rust conventions for binary organization.
Visibility correctness: Changing struct and fields to pub(crate) in bin/ethlambda/src/cli.rs (lines 12, 15, 18, etc.) is the correct approach to allow access from main.rs while keeping them internal to the crate.
Documentation preserved: The doc comments on struct fields are maintained, ensuring clap help text remains intact.

Verification:

All clap attributes (#[command], #[arg], etc.) are preserved correctly
Import in main.rs (use cli::CliOptions;) properly references the moved struct
Module declaration (mod cli;) placed appropriately with other module declarations

The refactoring is safe to merge.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

github-actions · 2026-06-29T14:03:31Z

🤖 Codex Code Review

No findings.

This diff is a pure refactor: CliOptions moved from main.rs into cli.rs, with the same clap annotations, defaults, and field types preserved. The added pub(crate) visibility on the struct and fields is appropriate because the parent module now reads those fields, so I do not see a correctness, security, performance, or consensus-behavior change here.

Residual risk: I could not complete a local compile in this environment. cargo check -p ethlambda was blocked first by rustup trying to write under a read-only /home/runner/.rustup, and then by the inability to fetch the git dependency lean-multisig with network disabled. So this review is based on source inspection rather than a successful build.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

greptile-apps · 2026-06-29T14:03:36Z

Greptile Summary

This PR moves the ethlambda CLI flags into a dedicated module. The main changes are:

Added bin/ethlambda/src/cli.rs for CliOptions.
Registered the new cli module in main.rs.
Kept the existing clap fields, defaults, and parser use unchanged.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.
The moved parser struct keeps the same clap attributes and field definitions.
The new module imports the symbols it needs, and main.rs can still call CliOptions::parse() and read the parsed fields.

Important Files Changed

Filename	Overview
bin/ethlambda/src/cli.rs	Adds the moved `CliOptions` clap parser struct with the same flags, defaults, and documentation.
bin/ethlambda/src/main.rs	Imports the new CLI module and removes the inline `CliOptions` definition.

_{Reviews (1): Last reviewed commit: "refactor(cli): move CliOptions into a de..." | Re-trigger Greptile}

github-actions · 2026-06-29T14:05:25Z

🤖 Claude Code Review

Review: `refactor(cli): move CliOptions into a dedicated cli module`

This PR is a pure structural refactor — it extracts the 80-line CliOptions clap struct from main.rs into a new cli.rs sibling module, adjusting visibility to pub(crate) as required by the module boundary. No CLI flags, defaults, constraints, or runtime behavior change.

The diff is correct and complete: all 15 fields are moved verbatim with identical types, clap attributes, value_parser constraints, value_delimiter settings, and cross-flag requires dependencies. The crate::version reference in cli.rs resolves correctly since version is declared as mod version in the crate root. The pub(crate) visibility is the minimum needed and carries no external risk (binary crate, single construction site via CliOptions::parse()).

One minor conventions finding:

bin/ethlambda/src/cli.rs:1 — Module-level doc comment explains what the module is, which the name cli already communicates.

//! Command-line interface for the ethlambda binary.

CLAUDE.md states: "Default to writing no comments. Only add one when the WHY is non-obvious." This line explains what the module is, not why it exists or any non-obvious constraint. The field-level doc comments that do survive this test (checkpoint-sync URL backward-compat stripping, hot-standby aggregator subnet-subscription freeze) contain genuine WHY content and are appropriate. This opening line should be removed.

Everything else — logic, safety, idiomatic Rust, cross-file wiring — is correct. The refactor achieves its stated goal cleanly.

Automated review by Claude (Anthropic) · sonnet · custom prompt

…#479) > Stacked on #477 (`refactor/cli-options-module`). Base will retarget to `main` once #477 merges. ## What Adds `--enable-proposer-aggregation` (default **off**) controlling how `build_block` collapses attestations that share the same `AttestationData`. A block may carry **at most one entry per `AttestationData`** — `on_block` rejects duplicates (`StoreError::DuplicateAttestationData`) before signature verification, so the proposer must collapse same-data proofs to one **either way**. This flag only changes *how*: | | strategy | coverage | cost | |---|---|---|---| | **enabled** | merge same-data proofs via recursive Type-1 aggregation into one union-coverage proof (leanSpec #510) | max (union of all proofs) | one leanVM aggregation per duplicated data entry | | **disabled** (default) | keep the single best-coverage proof per data, drop the rest | bounded by the best individual proof | no leanVM aggregation | Today the proposer always aggregates; the dominant cost of building blocks that carry attestations is exactly this per-data leanVM aggregation. The flag makes it opt-in. ## Threading ``` CliOptions.enable_proposer_aggregation (bin/ethlambda/src/cli.rs, default false) -> BlockChain::spawn(..) (crates/blockchain/src/lib.rs) -> BlockChainServer.enable_proposer_aggregation -> produce_block_with_signatures(.., flag) (store.rs) -> build_block(.., flag) (block_builder.rs) ├─ enabled -> compact_attestations (unchanged) └─ disabled -> keep_best_proof_per_data (new) ``` `keep_best_proof_per_data` groups selected entries by `AttestationData`, keeps the entry with the most participants (ties → first occurrence), and preserves first-occurrence order — mirroring `compact_attestations`' grouping but with no crypto. The one-entry-per-data invariant and the 1:1 attestation↔proof correspondence hold in both paths. ## Behavior change The binary default flips: proposers stop merging same-data proofs unless `--enable-proposer-aggregation` is passed. Blocks stay valid and interoperable; attestation entries just carry less voter coverage (the best single proof rather than the union). ## Scope note This gates only the in-`build_block` collapse (per the request). The separate Type-1→Type-2 merge in `propose_block` is untouched. With aggregation off there are at most `MAX_ATTESTATIONS_DATA` single-proof components feeding that merge, same as before (one per data) — so no blow-up there. Net build-time impact is worth a devnet A/B. ## Tests - New `build_block_without_proposer_aggregation_keeps_single_best_proof_per_data`: one `AttestationData` with two proofs ({0} and {1,2}); with the flag off, asserts exactly **one** entry survives, 1:1 with its signature, and the kept proof is the higher-coverage `{1,2}` (no leanVM crypto invoked). - Existing `build_block` tests pass `true` to preserve their current (aggregating) behavior. - `cargo clippy -p ethlambda-blockchain -p ethlambda --all-targets -- -D warnings` ✅ - `cargo test -p ethlambda-blockchain --lib` ✅ (39 passed) ## History First commit took the naive "leave duplicates unmerged" approach; the follow-up fix (`keep_best_proof_per_data`) corrects it after confirming `on_block` rejects duplicate `AttestationData`.

refactor(cli): move CliOptions into a dedicated cli module

0f542bb

main.rs had grown to mix CLI parsing with node orchestration. Extract the CliOptions clap struct into its own bin/ethlambda/src/cli.rs so the flag surface lives in one place and main.rs reads as orchestration only.

This was referenced Jun 29, 2026

refactor(cli): group aggregator flags into AggregatorOpts #478

Closed

feat(blockchain): gate proposer attestation aggregation behind a flag #479

Merged

pablodeymo approved these changes Jun 29, 2026

View reviewed changes

pablodeymo merged commit 22780a3 into main Jun 29, 2026
7 checks passed

pablodeymo deleted the refactor/cli-options-module branch June 29, 2026 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(cli): move CliOptions into a dedicated cli module#477

refactor(cli): move CliOptions into a dedicated cli module#477
pablodeymo merged 1 commit into
mainfrom
refactor/cli-options-module

MegaRedHand commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

greptile-apps Bot commented Jun 29, 2026

Important Files Changed

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

MegaRedHand commented Jun 29, 2026

Why

What

Verification

Uh oh!

github-actions Bot commented Jun 29, 2026

🤖 Kimi Code Review

Uh oh!

github-actions Bot commented Jun 29, 2026

🤖 Codex Code Review

Uh oh!

greptile-apps Bot commented Jun 29, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

github-actions Bot commented Jun 29, 2026

🤖 Claude Code Review

Review: refactor(cli): move CliOptions into a dedicated cli module

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Review: `refactor(cli): move CliOptions into a dedicated cli module`