Deduplicate AggregationBits type alias and remove redundant getters

[pablodeymo]

Motivation

Two cleanup items from the code simplification audit:

1. Duplicate AggregationBits: The type alias pub type AggregationBits = ssz_types::BitList<ValidatorRegistryLimit> was defined identically in both attestation.rs and block.rs, with the same doc comment. This creates confusion about which is the canonical definition.

2. Redundant getters on AggregatedSignatureProof: The participants() and proof_data() methods return references to fields that are already pub. No code calls these getters — all access goes through the fields directly.

Description

AggregationBits dedup

• Keep the single definition in crates/common/types/src/attestation.rs (where it semantically belongs)
• Remove the duplicate from crates/common/types/src/block.rs, import from attestation instead
• Update crates/blockchain/src/store.rs to import AggregationBits from attestation instead of block
• Update crates/blockchain/tests/signature_types.rs to drop the redundant EthAggregationBitsSig alias (was identical to EthAggregationBits from attestation)

Redundant getters removal

• Remove AggregatedSignatureProof::participants() and AggregatedSignatureProof::proof_data() — both fields are pub and all code accesses them directly

Files changed

File	Change
crates/common/types/src/block.rs	Remove duplicate type alias + remove redundant getters
crates/blockchain/src/store.rs	Move AggregationBits import from block to attestation
crates/blockchain/tests/signature_types.rs	Remove duplicate alias, use EthAggregationBits

How to test

make fmt
make lint
make test

[github-actions]

🤖 Kimi Code Review

Review Summary

The PR consolidates the AggregationBits type definition by removing the duplicate definition in block.rs and using the one from attestation.rs. This is a good cleanup that reduces redundancy.

Issues Found

1. Breaking API Change (High Priority)

File: crates/common/types/src/block.rs, line 109

• The PR removes AggregationBits from block.rs but this is a public type re-export. Any external crates importing ethlambda_types::block::AggregationBits will break.
• Fix: Add a type alias in block.rs:

  pub use crate::attestation::AggregationBits;

2. Incomplete Import Fix (Medium Priority)

File: crates/blockchain/src/store.rs, line 13

• The import path for AggregationBits should be updated to use the attestation module consistently:

  use ethlambda_types::{
      ShortRoot,
      attestation::{AggregatedAttestation, Attestation, AttestationData, SignedAttestation, AggregationBits},
      block::{AggregatedAttestations, AggregatedSignatureProof, Block, BlockBody, SignedBlockWithAttestation},
      primitives::{H256, ssz::TreeHash},
  };

3. Missing Method Migration (Low Priority)

File: crates/common/types/src/block.rs, lines 105-117

• The removed methods participants() and proof_data() from AggregatedSignatureProof might be used elsewhere. While the fields are public (pub), the convenience methods provided a cleaner API.
• Check: Verify no external code depends on these methods before removal.

Positive Changes

• ✅ Eliminates duplicate type definition
• ✅ Improves code organization by centralizing attestation-related types
• ✅ Maintains backward compatibility for field access (fields remain pub)

Recommendation

Before merging, ensure the public API compatibility issue (Point 1) is addressed with a proper re-export to prevent breaking downstream consumers.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Summary
This PR is a small refactor (type relocation + import cleanup). I don’t see consensus-logic, state-transition, or verification changes. No security or performance regressions apparent.

Findings

1. Potential API break: removed accessors on AggregatedSignatureProof
   The getters participants() and proof_data() were removed. If any downstream crates relied on those methods (instead of public fields), this is a breaking change. Consider keeping them or adding them back as thin wrappers for compatibility.
   File: crates/common/types/src/block.rs:78-104

2. Type move may cause downstream churn
   AggregationBits now lives in attestation.rs. If external users were importing it from block, they will break. Consider re-exporting it from the old module (e.g., pub use crate::attestation::AggregationBits;) to keep backward compatibility.
   File: crates/common/types/src/attestation.rs:59-63

Consensus-layer checklist

• Fork choice / justification / finalization / state transition: not touched.
• Attestation processing, XMSS verification, SSZ encode/decode: not touched.

If the API churn is intentional, this looks good to me.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

The diff is small and I have enough context now. Let me write up the review.

---

PR #146 Review — Deduplicate AggregationBits type alias and remove redundant getters

Changes are correct and well-scoped. This is a mechanical cleanup with no logic changes. Here are my observations:

AggregationBits deduplication (block.rs → attestation.rs)

The duplicate was byte-for-byte identical, including the doc comment. The canonical location in attestation.rs is semantically appropriate since the type originates with attestation participation tracking and is borrowed by block types. The import chain is now clear: block.rs imports AggregationBits from attestation alongside AggregatedAttestation, which reflects the actual dependency.

The ValidatorRegistryLimit import in block.rs is still needed for AttestationSignatures and AggregatedAttestations, so no dead imports were introduced.

Getter removal (participants() and proof_data())

Verified against the codebase: all access sites use direct field access (p.participants, proof.participants.clone(), aggregated_proof.proof_data). The getters had zero callers. Removing them reduces API surface without breaking anything.

The fields remain pub, so external crates (if any were ever added) would still have access without indirection. The removal is safe.

signature_types.rs alias cleanup

EthAggregationBitsSig was AggregationBits from block, which was ssz_types::BitList<ValidatorRegistryLimit> — identical to EthAggregationBits from attestation. Replacing the former with the latter at line 231 is a no-op semantically. The test file already imported EthAggregationBits, so no new import is needed.

No concerns

Nothing to flag. The PR description accurately describes what was changed and why. The "how to test" section (make fmt && make lint && make test) covers the necessary surface for this kind of refactor.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Summary

This PR eliminates code duplication by consolidating the AggregationBits type alias to a single canonical location in the attestation module and removes unused getter methods from AggregatedSignatureProof.

Key changes:

• Removed duplicate AggregationBits type definition from block.rs (kept single definition in attestation.rs where it semantically belongs)
• Removed participants() and proof_data() getter methods from AggregatedSignatureProof since both fields are already pub and accessed directly throughout the codebase
• Updated imports in store.rs and signature_types.rs to reference AggregationBits from the attestation module
• Removed redundant EthAggregationBitsSig type alias in test file

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• This is a pure refactoring that removes code duplication without changing any logic. The removed getter methods are confirmed unused (no matches in codebase), and the type alias consolidation maintains identical definitions with proper import updates.
• No files require special attention

Important Files Changed

Filename	Overview
crates/common/types/src/block.rs	Removed duplicate AggregationBits type alias and redundant getter methods. Import now comes from attestation module.
crates/blockchain/src/store.rs	Updated import to use AggregationBits from attestation module instead of block module.
crates/blockchain/tests/signature_types.rs	Removed redundant EthAggregationBitsSig alias. Now uses EthAggregationBits from attestation module directly.

_{Last reviewed commit: 4259856}