Skip to content

refactor(storage): extract store auxiliary types#377

Open
chahat-101 wants to merge 3 commits into
lambdaclass:mainfrom
chahat-101:308-refactor-store-aux-types
Open

refactor(storage): extract store auxiliary types#377
chahat-101 wants to merge 3 commits into
lambdaclass:mainfrom
chahat-101:308-refactor-store-aux-types

Conversation

@chahat-101
Copy link
Copy Markdown

@chahat-101 chahat-101 commented May 15, 2026

Closes #308

📄 Description / Motivation

Refactors crates/storage/src/store.rs by extracting auxiliary types, constants, and helper utilities into dedicated modules to improve readability and maintainability.

The original store.rs had grown fairly large, making navigation and future changes harder.

What Changed

  • Added:

    • config.rs
    • types.rs
    • utils.rs
  • Moved:

    • metadata/config constants → config.rs
    • payload/gossip buffer types and related logic → types.rs
    • helper utilities (write_signed_block, live chain key helpers) → utils.rs
  • Updated:

    • lib.rs exports
    • imports/re-exports across the storage crate
    • store.rs to focus on core Store logic
  • Preserved newer upstream logic during rebase, including:

    • GetForkchoiceStoreError
    • anchor_pair_is_consistent
    • bits_is_subset
    • get_block()

Correctness / Behavior

No intended behavior changes.

This PR is a structural refactor only:

  • storage layout remains unchanged
  • public APIs remain intact
  • pruning/buffer behavior is preserved

Tests Run

cargo fmt
cargo check -p ethlambda-storage
cargo test -p ethlambda-storage


Full workspace clippy/tests were not completed locally on Windows due to upstream native dependency/toolchain issues (jemalloc / kzg-rs) unrelated to this change.```

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 15, 2026

Greptile Summary

This PR refactors crates/storage/src/store.rs by splitting auxiliary types, constants, and helper utilities into three dedicated modules (config.rs, types.rs, utils.rs). The core Store logic in store.rs is preserved, and the storage layout and backend behaviour are unchanged.

  • config.rs: Extracts metadata keys, retention window constants, and buffer caps — all correctly scoped to the private module.
  • types.rs: Extracts ForkCheckpoints, PayloadBuffer, and GossipSignatureBuffer. ForkCheckpoints fields are widened from private to pub and GossipSignatureBuffer::len loses its #[cfg(test)] guard.
  • store.rs: Adds a net-new extract_latest_all_attestations public method not present in the base.

Confidence Score: 3/5

Safe to merge after addressing the ForkCheckpoints field visibility change, which expands the crate public API contrary to the stated goals.

The refactor correctly moves code without altering storage or fork-choice logic, but ForkCheckpoints fields are now unconditionally pub where they were private — external consumers can construct the struct directly without using the provided constructors. The dropped cfg(test) on GossipSignatureBuffer::len and the undocumented new extract_latest_all_attestations method add further concerns.

crates/storage/src/types.rs (ForkCheckpoints field visibility, missing cfg(test) on GossipSignatureBuffer::len) and crates/storage/src/store.rs (new extract_latest_all_attestations method).

Important Files Changed

Filename Overview
crates/storage/src/config.rs New file extracting metadata keys, retention constants, and buffer cap constants from store.rs. All items correctly scoped via the private module declaration.
crates/storage/src/types.rs ForkCheckpoints fields changed from private to pub (API expansion); GossipSignatureBuffer::len drops its original #[cfg(test)] gate.
crates/storage/src/utils.rs Extracts encode/decode_live_chain_key and write_signed_block. Functions are pub but the module is private, so they remain crate-internal.
crates/storage/src/lib.rs Adds module declarations and re-exports ForkCheckpoints and GossipSignatureSnapshot from types. Correct.
crates/storage/src/store.rs Reduced to core Store logic. Adds a new extract_latest_all_attestations method not present in the base; core logic preserved correctly.

Comments Outside Diff (1)

  1. crates/storage/src/store.rs, line 853-873 (link)

    P2 New method not mentioned in PR description

    extract_latest_all_attestations is a net-new pub method on Store — it does not appear in the base branch and is not listed in the PR description's "Preserved newer upstream logic" section. The PR is described as a structural refactor with no intended behavior changes, but this adds new public API surface. If this was cherry-picked from upstream, it should be tracked explicitly; if added unintentionally it should be removed or handled in a separate PR.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: crates/storage/src/store.rs
    Line: 853-873
    
    Comment:
    **New method not mentioned in PR description**
    
    `extract_latest_all_attestations` is a net-new `pub` method on `Store` — it does not appear in the base branch and is not listed in the PR description's "Preserved newer upstream logic" section. The PR is described as a structural refactor with no intended behavior changes, but this adds new public API surface. If this was cherry-picked from upstream, it should be tracked explicitly; if added unintentionally it should be removed or handled in a separate PR.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
crates/storage/src/types.rs:14-18
The `ForkCheckpoints` fields were private in the original `store.rs` (`head`, `justified`, `finalized`). They're now `pub`, which expands the crate's public API: external consumers can directly construct a struct literal or read/write the fields without going through the `head_only`/`new` constructors. The minimum visibility needed for access from `store.rs` is `pub(crate)`. Widening to `pub` contradicts the stated goal of keeping public APIs intact.

```suggestion
pub struct ForkCheckpoints {
    pub(crate) head: H256,
    pub(crate) justified: Option<Checkpoint>,
    pub(crate) finalized: Option<Checkpoint>,
}
```

### Issue 2 of 3
crates/storage/src/types.rs:337-346
The original `GossipSignatureBuffer::len` was gated with `#[cfg(test)]` because it is only called from test code. Dropping that attribute means the method now compiles in production builds where it is dead code, and the compiler will emit a `dead_code` warning for it.

```suggestion
    pub fn total_signatures(&self) -> usize {
        self.total_signatures
    }

    #[cfg(test)]
    pub fn len(&self) -> usize {
        self.data.len()
    }
}

#[cfg(test)]
```

### Issue 3 of 3
crates/storage/src/store.rs:853-873
**New method not mentioned in PR description**

`extract_latest_all_attestations` is a net-new `pub` method on `Store` — it does not appear in the base branch and is not listed in the PR description's "Preserved newer upstream logic" section. The PR is described as a structural refactor with no intended behavior changes, but this adds new public API surface. If this was cherry-picked from upstream, it should be tracked explicitly; if added unintentionally it should be removed or handled in a separate PR.

Reviews (1): Last reviewed commit: "refactor(storage): extract store auxilia..." | Re-trigger Greptile

Comment thread crates/storage/src/types.rs
Comment thread crates/storage/src/types.rs
chahat-101 and others added 2 commits May 16, 2026 01:22
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move auxiliary types outside crates/storage/src/store.rs

1 participant