refactor(storage): extract store auxiliary types#377
Conversation
Greptile SummaryThis PR refactors
Confidence Score: 3/5Safe 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).
|
| 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)
-
crates/storage/src/store.rs, line 853-873 (link)New method not mentioned in PR description
extract_latest_all_attestationsis a net-newpubmethod onStore— 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
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>
Closes #308
📄 Description / Motivation
Refactors
crates/storage/src/store.rsby extracting auxiliary types, constants, and helper utilities into dedicated modules to improve readability and maintainability.The original
store.rshad grown fairly large, making navigation and future changes harder.What Changed
Added:
config.rstypes.rsutils.rsMoved:
config.rstypes.rswrite_signed_block, live chain key helpers) →utils.rsUpdated:
lib.rsexportsstore.rsto focus on coreStorelogicPreserved newer upstream logic during rebase, including:
GetForkchoiceStoreErroranchor_pair_is_consistentbits_is_subsetget_block()Correctness / Behavior
No intended behavior changes.
This PR is a structural refactor only:
Tests Run