refactor: extract get_module_wrapper helper in InitiaModuleStorage

[beer-1]

Summary

• Extracts repeated preamble (ModuleId construction + checksum fetch + cache lookup) from all 6 ModuleStorage trait methods into a single get_module_wrapper private helper
• Reduces boilerplate by ~30 lines with no behavioral changes
• All 11 storage crate tests pass

Details

Every ModuleStorage trait method in InitiaModuleStorage repeated:

let id = ModuleId::new(*address, module_name.to_owned());
let checksum = match self.base_storage.fetch_checksum(address, module_name)? {
    Some(checksum) => checksum,
    None => return Ok(None),
};
self.module_cache.get_module_or_build_with(&id, &checksum, self)?

The new get_module_wrapper returns Option<(ModuleId, Checksum, ModuleWrapper)> — the tuple includes all three because fetch_verified_module needs id and checksum for dependency verification. Other methods destructure with (_, _, module).

Test plan

• cargo check -p initia-move-storage passes
• cargo test -p initia-move-storage — all 11 tests pass
• CI verification

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Centralized module lookup and validation into a single shared helper to remove duplicated logic across storage operations.
  • Streamlined existence checks, module fetching, size/metadata access, verification, and dependency resolution to reuse the common flow.
  • No user-facing behavioral changes; internal implementation simplified and easier to maintain.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9d0d09a4-12db-41aa-8f3a-6e18aee7ad50

📥 Commits

Reviewing files that changed from the base of the PR and between 8441220 and 4371741.

📒 Files selected for processing (1)

• crates/storage/src/module_storage.rs

---

Walkthrough

Added an inherent helper get_module_wrapper on InitiaModuleStorage that builds a ModuleId, fetches its checksum from base_storage, and calls module_cache.get_module_or_build_with, returning Ok(None) when checksum is missing or Ok(Some((id, checksum, wrapper))). Multiple ModuleStorage methods were refactored to use this helper.

Changes

Cohort / File(s)

Summary

Module storage refactor crates/storage/src/module_storage.rs

Added InitiaModuleStorage::get_module_wrapper(...) to centralize ModuleId construction, checksum fetch, and cache lookup. Updated ModuleStorage method implementations (check_module_exists, fetch_module_bytes, fetch_module_size_in_bytes, fetch_module_metadata, fetch_deserialized_module, fetch_verified_module) and visit_dependencies_and_verify to use the helper and remove duplicated resolution logic.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Caller
    participant IMS as InitiaModuleStorage
    participant BS as BaseStorage
    participant MC as ModuleCache

    Caller->>IMS: request module (address, name)
    IMS->>IMS: construct ModuleId
    IMS->>BS: fetch checksum(module_id)
    alt checksum found
        BS-->>IMS: checksum
        IMS->>MC: get_module_or_build_with(module_id, checksum, context)
        MC-->>IMS: ModuleWrapper
        IMS-->>Caller: Ok(Some(module_id, checksum, wrapper))
    else checksum missing
        BS-->>IMS: None
        IMS-->>Caller: Ok(None)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hop to fetch a checksum bright,
I stitch the id and cache it tight,
One helper now joins every quest,
Fewer hops, and fewer tests —
A little hop, a tidy night.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor: extract get_module_wrapper helper in InitiaModuleStorage' directly and accurately describes the main change: extraction of a new helper method. It is concise, specific, and clearly summarizes the primary refactoring effort.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/module-storage-helper

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/storage/src/module_storage.rs (1)

194-208: Reuse get_module_wrapper in dependency traversal too.

This removes the duplication from the trait methods, but Lines 331-343 still inline the same ModuleId + checksum + cache lookup sequence. Pulling that path onto this helper would keep module resolution behavior aligned in one place.

♻️ Possible follow-up

let (dependency_id, dependency_checksum, dependency) = module_cache_with_context
    .get_module_wrapper(addr, name)?
    .ok_or_else(|| module_linker_error!(addr, name))?;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/storage/src/module_storage.rs` around lines 194 - 208, The dependency
traversal duplicates the ModuleId + checksum + cache lookup logic; refactor to
call the existing get_module_wrapper(&self, address, module_name) helper from
the traversal code (replace the inline ModuleId::new(...), fetch_checksum(...),
and module_cache.get_module_or_build_with(...) sequence). Unwrap the Option with
ok_or_else(|| module_linker_error!(addr, name)) to return the linker error when
missing, e.g. let (dependency_id, dependency_checksum, dependency) =
self.get_module_wrapper(addr, name)? .ok_or_else(|| module_linker_error!(addr,
name))?; ensuring you use the same checksum and ModuleWrapper types as before so
resolution behavior stays centralized in get_module_wrapper.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/storage/src/module_storage.rs`:
- Around line 194-208: The dependency traversal duplicates the ModuleId +
checksum + cache lookup logic; refactor to call the existing
get_module_wrapper(&self, address, module_name) helper from the traversal code
(replace the inline ModuleId::new(...), fetch_checksum(...), and
module_cache.get_module_or_build_with(...) sequence). Unwrap the Option with
ok_or_else(|| module_linker_error!(addr, name)) to return the linker error when
missing, e.g. let (dependency_id, dependency_checksum, dependency) =
self.get_module_wrapper(addr, name)? .ok_or_else(|| module_linker_error!(addr,
name))?; ensuring you use the same checksum and ModuleWrapper types as before so
resolution behavior stays centralized in get_module_wrapper.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dec26092-17c6-4d65-a46b-82bec7dfd258

📥 Commits

Reviewing files that changed from the base of the PR and between c39ce30 and a823766.

📒 Files selected for processing (1)

• crates/storage/src/module_storage.rs

[coderabbitai]

🧹 Nitpick comments (1)

crates/storage/src/module_storage.rs (1)

191-193: Clarify the helper’s None vs Err contract.

The wording here blurs two different outcomes: missing module data returns Ok(None), while deserialization/checksum failures still bubble up as Err. Tightening the comment would make the helper’s behavior clearer for future callers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/storage/src/module_storage.rs` around lines 191 - 193, Update the
documentation for the helper that "Resolves a module by address and name" to
explicitly state its Result contract: document that when the module checksum or
stored data is missing the function returns Ok(None), when the module is
successfully found and built it returns Ok(Some(module)), and when
deserialization or checksum validation fails the function returns Err(...)
propagating the underlying error; mention the exact behavior for checksum
lookup, cache build, and deserialization so callers of this helper (the
resolve-by-address-and-name helper in module_storage.rs) can unambiguously
handle Ok(None) vs Err.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/storage/src/module_storage.rs`:
- Around line 191-193: Update the documentation for the helper that "Resolves a
module by address and name" to explicitly state its Result contract: document
that when the module checksum or stored data is missing the function returns
Ok(None), when the module is successfully found and built it returns
Ok(Some(module)), and when deserialization or checksum validation fails the
function returns Err(...) propagating the underlying error; mention the exact
behavior for checksum lookup, cache build, and deserialization so callers of
this helper (the resolve-by-address-and-name helper in module_storage.rs) can
unambiguously handle Ok(None) vs Err.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 93ee28b5-3cc0-4ec4-81b8-eebf654076e0

📥 Commits

Reviewing files that changed from the base of the PR and between a823766 and 8441220.

📒 Files selected for processing (1)

• crates/storage/src/module_storage.rs

[traviolus]

LGTM