Fix/store error wrapper

[d4m014]

this is a new error file with an enum that wraps api::Error for Store for better error
handling in the future

related to #306

[greptile-apps]

Greptile Summary

This PR introduces a new error.rs module in the storage crate containing an Error enum that wraps api::Error, and updates init_store to return Result<Self, Error> as a preparatory step for better error handling.

• init_store now returns Result<Self, Error>, but all internal storage operations still use .expect() rather than ?, so the Err variant is unreachable — callers immediately .expect() the result, leaving behavior unchanged from before.
• The error module is declared privately (mod error) and Error is not re-exported from lib.rs, so the type is inaccessible to consumers of the crate.

Confidence Score: 3/5

Runtime behavior is identical to before since all storage failures still panic, but the incomplete transition leaves init_store with a misleading Result return type that can never produce an Err.

The init_store function's body still panics on every storage failure instead of propagating errors through the new Result wrapper, meaning the error-handling infrastructure added here is a no-op in practice. Any future caller that trusts the Result type and handles Err properly will get a false sense of safety.

crates/storage/src/store.rs — the init_store body needs its internal .expect() calls replaced with ? for the Result return type to be meaningful.

Important Files Changed

Filename	Overview
crates/storage/src/error.rs	New error module defining a Storage variant wrapping api::Error; module is private and the error message has a minor formatting issue (missing space after colon).
crates/storage/src/store.rs	init_store return type changed to Result<Self, Error> but all internal storage calls still use .expect(), so the Err path is unreachable; callers immediately .expect() the result, making the wrapper a no-op.
crates/storage/src/lib.rs	Adds mod error declaration; module is private and Error is not re-exported, limiting its usability to within the crate only.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["from_anchor_state()"] -->|calls| C["init_store()"]
    B["get_forkchoice_store()"] -->|calls| C
    C --> D{"internal .expect()"}
    D -->|storage op fails| E["panic"]
    D -->|success| F["Ok(Self)"]
    F -->|".expect()"| A
    F -->|".expect() wrapped in Ok()"| B
    A --> G["Self"]
    B --> H["Result<Self, GetForkchoiceStoreError>"]
    style E fill:#f55,color:#fff

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["from_anchor_state()"] -->|calls| C["init_store()"]
    B["get_forkchoice_store()"] -->|calls| C
    C --> D{"internal .expect()"}
    D -->|storage op fails| E["panic"]
    D -->|success| F["Ok(Self)"]
    F -->|".expect()"| A
    F -->|".expect() wrapped in Ok()"| B
    A --> G["Self"]
    B --> H["Result<Self, GetForkchoiceStoreError>"]
    style E fill:#f55,color:#fff

Loading

Comments Outside Diff (1)

1. crates/storage/src/store.rs, line 579-671 (link)

   Result return type can never be Err — all errors still panic internally

   init_store now declares Result<Self, Error> as its return type, but every storage operation inside it still uses .expect(...) (e.g. backend.begin_write().expect("write batch"), batch.commit().expect("commit")). This means the function will always either panic on failure or return Ok(Self) — it can never actually produce an Err value. The callers (from_anchor_state, get_forkchoice_store) correctly mirror this by calling .expect(...) on the result, but that makes the Result wrapper a no-op for error propagation. If the goal is proper error handling, the internal .expect() calls should be replaced with ? operators so real storage failures bubble up through the Err path.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/storage/src/store.rs
   Line: 579-671
   
   Comment:
   **`Result` return type can never be `Err` — all errors still panic internally**
   
   `init_store` now declares `Result<Self, Error>` as its return type, but every storage operation inside it still uses `.expect(...)` (e.g. `backend.begin_write().expect("write batch")`, `batch.commit().expect("commit")`). This means the function will always either panic on failure or return `Ok(Self)` — it can never actually produce an `Err` value. The callers (`from_anchor_state`, `get_forkchoice_store`) correctly mirror this by calling `.expect(...)` on the result, but that makes the `Result` wrapper a no-op for error propagation. If the goal is proper error handling, the internal `.expect()` calls should be replaced with `?` operators so real storage failures bubble up through the `Err` path.
   
   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/store.rs:579-671
**`Result` return type can never be `Err` — all errors still panic internally**

`init_store` now declares `Result<Self, Error>` as its return type, but every storage operation inside it still uses `.expect(...)` (e.g. `backend.begin_write().expect("write batch")`, `batch.commit().expect("commit")`). This means the function will always either panic on failure or return `Ok(Self)` — it can never actually produce an `Err` value. The callers (`from_anchor_state`, `get_forkchoice_store`) correctly mirror this by calling `.expect(...)` on the result, but that makes the `Result` wrapper a no-op for error propagation. If the goal is proper error handling, the internal `.expect()` calls should be replaced with `?` operators so real storage failures bubble up through the `Err` path.

### Issue 2 of 3
crates/storage/src/lib.rs:3
**`error` module is private and `Error` type is not re-exported**

`mod error` is declared without `pub`, and `lib.rs` does not `pub use error::Error`. This means `crate::error::Error` is only accessible within the `storage` crate itself — callers depending on this crate cannot name or match on the error type. If this type is intended for future use across crate boundaries (as the PR description suggests), it should be exported via `pub mod error` and/or a `pub use error::Error` re-export in `lib.rs`.

### Issue 3 of 3
crates/storage/src/error.rs:3
Missing space after the colon in the error message format string.

```suggestion
    #[error("storage error: {0}")]
```

_{Reviews (1): Last reviewed commit: "new error file with enum that wraps api:..." | Re-trigger Greptile}

[d4m014]

@MegaRedHand, once we agree on this, the rest of Store functions are ready just as we did init_store in #448 and can be pushed