Repository navigation
feat(dash-spv)!: make the storage private and add header queries to DashSpvClient - #1176
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue [#1173] requires DashSpvClient::tip_header() to return Option<HashedBlockHeader>. The current method returns Option<BlockHeaderTip>. BlockHeaderTip stores separate height, BlockHeader,… |
Change tip_header() to return Option<HashedBlockHeader>. Preserve None for an empty header store. Add or update automated client tests for the required tip and missing-header behavior. |
| Docstring Coverage | |
Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 22 files. (2 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: making storage private and adding header queries to DashSpvClient. Out of Scope Changes check The reviewed changes support the storage encapsulation and public header-query migration in [ #1173]. They update client construction, call sites, tests, examples, benchmarks, documentation, and test-o…Full details: Out of Scope Changes check
-
Explanation
The reviewed changes support the storage encapsulation and public header-query migration in [
#1173]. They update client construction, call sites, tests, examples, benchmarks, documentation, and test-only storage exports. No demonstrated unrelated product change is present.
-
Full details: Linked Issues check
-
Explanation
Issue [
#1173] requiresDashSpvClient::tip_header()to returnOption<HashedBlockHeader>. The current method returnsOption<BlockHeaderTip>.BlockHeaderTipstores separate height,BlockHeader, and hash fields, so it is not the requiredHashedBlockHeadertype.header_atandheader_heighthave the requiredResult<Option<_>>shapes and the storage module is private.Resolution
Change
tip_header()to returnOption<HashedBlockHeader>. PreserveNonefor an empty header store. Add or update automated client tests for the required tip and missing-header behavior.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 22 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
a6de8d8 to
15bdb45
Compare
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Bots are done — your move: address xdustinface requested changes, then post |
github-actions
Bot
added
waiting-self-review
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1176 +/- ##
=======================================
Coverage 78.86% 78.87%
=======================================
Files 304 304
Lines 81410 81381 -29
=======================================
- Hits 64202 64187 -15
+ Misses 17208 17194 -14
|
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Direction looks right. A few things before merge:
1. Platform's offline storage wipe
Platform's offline clear_storage (spv/runtime.rs) opens its own DiskStorageManager, which no longer compiles. It only exists because Platform drops the client on stop() and builds a new one per start. Keeping one client across stop/start and clearing through it would remove it. Is that the plan for the Platform side? If dropping on stop is deliberate (e.g. memory while backgrounded), a client-free DashSpvClient::clear_storage_at(&ClientConfig) would be the alternative.
update_config should also reject a changed storage_path now that the client opens its storage from it.
2. Stale DashSpvClient docs
The doc comment in client/core.rs still describes the S: StorageManager parameter, swappable storage backends, and DiskStorageManager as a third type argument in the examples.
3. No tests for the new queries
anchored_tip / birth_height_anchors_chain_to_nearest_checkpoint in client/mod.rs could assert tip_header(), header_at and header_height against the 200 000 checkpoint, including the Ok(None) cases.
Non-blocking
tip_headerreturnsOptionandget_tipdrops read errors, unlike the other two.get_tip_height+get_header(tip)would giveResult<Option<_>>.pub mod syncstill exports managers generic over storage traits that are no longer nameable outside the crate. Follow-up to make thempub(crate)?
…ashSpvClient The storage module is no longer exported, and `DashSpvClient` is no longer generic over the storage: `new` opens a `DiskStorageManager` at `config.storage_path` itself. `storage()` is replaced by `tip_header`, `header_at` and `header_height`. Tests and benches reach the storage types through `test_utils`. Closes #1173 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
1 - I talked with them, we already have a method in the SPV CLient to clear the storage, there is no point on exposing the entire storage. About the update config method, I think is not even tested and a lot of stuff cant be really changed runtime, I would drop that feature entirely and force everyone to create a new client if they want to change the configuration, avoiding possible issues. No body uses the method here or in platform 2 - Done 3 - I dont really see the point on having a test for a facade without logic, can you explain why you think they are important to have here?? About pub mod sync, I will take care of them in a different PR, no point on having them exposed I think but it is out of the scope of this one, just in case I have to adjust something different else |
15bdb45 to
8965f47
Compare
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
|
Bots are done — your move: address xdustinface requested changes, then post |
github-actions
Bot
added
waiting-self-review
Platform reads headers through the storage internals (
client.storage()→block_headers()). This makes the storage internal and gives the client the queries it needs.dash_spv::storageis no longer exported, andDashSpvClient<W, N>is no longer generic over the storage.DashSpvClient::newno longer takes a storage: it opens aDiskStorageManageratconfig.storage_path.storage()is replaced bytip_header(),header_at(height)andheader_height(&BlockHash). A missing header isOk(None), distinct from a read error.test_utils(test-utilsfeature).Breaking:
dash_spv::storageis private,DashSpvClientloses its storage type parameter, andDashSpvClient::newandstorage()change as above.Closes #1173
🤖 Generated with Claude Code
Summary by CodeRabbit
PR Hygiene ·
8965f47/self-revieweddash-spv-bench/src/main.rs,dash-spv-ffi/src/client.rs) — QuantumExplorer or xdustinfacedash-spv(dash-spv/CLAUDE.md,dash-spv/README.md,dash-spv/benches/storage.rsand 19 more) — QuantumExplorer or xdustinfaceWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.