fix(minibf): return affected addresses from /blocks/{hash_or_number}/addresses#1124
fix(minibf): return affected addresses from /blocks/{hash_or_number}/addresses#1124slowbackspace wants to merge 4 commits into
Conversation
The /blocks/{hash_or_number}/addresses endpoint returned a plain list of
tx hashes instead of Blockfrost's address entries. Rebuild it to return
addresses affected by the block's transactions, resolving spent inputs
through the archive to include input-side addresses.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe block addresses endpoint now returns deterministic address-to-transaction mappings, including addresses from consumed inputs resolved through dependent transactions. It loads dependencies before modeling results and adds coverage for ordering, pagination, and error responses. ChangesBlock address resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant by_hash_or_number_addresses
participant BlockModelBuilder
participant domain.get_tx_batch
Client->>by_hash_or_number_addresses: request block addresses
by_hash_or_number_addresses->>BlockModelBuilder: create from BlockBody
BlockModelBuilder->>by_hash_or_number_addresses: required_input_deps
by_hash_or_number_addresses->>domain.get_tx_batch: fetch dependency CBOR
by_hash_or_number_addresses->>BlockModelBuilder: load_dep for each dependency
by_hash_or_number_addresses->>Client: return paginated JSON
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minibf/src/routes/blocks.rs (1)
406-440: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing dependency transactions are silently dropped, producing an incomplete address list with no signal.
When
domain.get_tx_batch(deps)returnsNonefor a hash (e.g. the spent tx is outside the archive's retention window), the loop at Lines 423-427 simply skipsload_depfor that key. Downstream,BlockModelBuilder::into_model(crates/minibf/src/mapping.rs Lines 2324-2337) then can't resolve that input's address and just omits it fromtouched— no error, no log, nothing to tell the caller the response is incomplete. Since real Blockfrost typically has full history and would resolve every input address, this is a real compatibility gap for older/pruned blocks, and it fails silently rather than surfacing the limitation.At minimum, log when a required dependency is missing so this degradation is observable; consider whether returning an error is more appropriate than silently serving a partial result for an endpoint whose entire purpose is byte-for-byte parity with Blockfrost.
🩹 Minimal observability fix
for (key, cbor) in deps.iter() { if let Some(cbor) = cbor { builder.load_dep(*key, cbor)?; + } else { + tracing::warn!(tx_hash = %key, "missing dependency tx while resolving block addresses; result may be incomplete"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minibf/src/routes/blocks.rs` around lines 406 - 440, Add observable handling for missing required dependencies in by_hash_or_number_addresses: when get_tx_batch returns None for a dependency, log the missing transaction identifier before continuing. Preserve the existing successful load_dep behavior and response flow; do not silently skip the missing dependency without a diagnostic.
🧹 Nitpick comments (2)
crates/minibf/src/mapping.rs (1)
2001-2010: 📐 Maintainability & Code Quality | 🔵 TrivialConsider using
EraCboraccessors instead of tuple indices.
cbor.0/cbor.1bypass the.era()/.cbor()accessor methods already defined onEraCbor(crates/core/src/lib.rs). Purely cosmetic, but using the accessors keeps this call site consistent with the rest of the codebase and insulates it from future field reordering.♻️ Proposed tweak
- let era = try_into_or_500!(cbor.0); + let era = try_into_or_500!(cbor.era()); - let tx = MultiEraTx::decode_for_era(era, &cbor.1) + let tx = MultiEraTx::decode_for_era(era, cbor.cbor()) .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minibf/src/mapping.rs` around lines 2001 - 2010, Update load_dep to use EraCbor’s era() and cbor() accessors instead of direct cbor.0 and cbor.1 tuple indexing, while preserving the existing decoding and error-handling behavior.crates/minibf/src/routes/blocks.rs (1)
608-715: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for input-side address resolution and past-the-end pagination.
The new tests cover output-side addresses, alphabetical ordering, ignoring
order, one pagination case, and error paths, but none of them exercise the actual new capability this PR introduces — resolving addresses of consumed inputs viarequired_input_deps/load_dep. There's also no test for the stated "returns200 []for past-the-end pages" behavior mentioned in the PR objectives.Consider adding a test with a fixture block whose first tx spends an input from a known predecessor address, plus a pagination test using a
page/countcombination beyond the last entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minibf/src/routes/blocks.rs` around lines 608 - 715, Extend the address endpoint tests around blocks_by_hash_or_number_addresses_happy_path to use a fixture block whose transaction consumes an input from a known predecessor address, then assert that address and its transaction are returned through required_input_deps/load_dep resolution. Add a pagination test for blocks_by_hash_or_number_addresses using page/count beyond the final entry and assert the endpoint returns HTTP 200 with an empty list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/minibf/src/routes/blocks.rs`:
- Around line 406-440: Add observable handling for missing required dependencies
in by_hash_or_number_addresses: when get_tx_batch returns None for a dependency,
log the missing transaction identifier before continuing. Preserve the existing
successful load_dep behavior and response flow; do not silently skip the missing
dependency without a diagnostic.
---
Nitpick comments:
In `@crates/minibf/src/mapping.rs`:
- Around line 2001-2010: Update load_dep to use EraCbor’s era() and cbor()
accessors instead of direct cbor.0 and cbor.1 tuple indexing, while preserving
the existing decoding and error-handling behavior.
In `@crates/minibf/src/routes/blocks.rs`:
- Around line 608-715: Extend the address endpoint tests around
blocks_by_hash_or_number_addresses_happy_path to use a fixture block whose
transaction consumes an input from a known predecessor address, then assert that
address and its transaction are returned through required_input_deps/load_dep
resolution. Add a pagination test for blocks_by_hash_or_number_addresses using
page/count beyond the final entry and assert the endpoint returns HTTP 200 with
an empty list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d4f82c1-dcd8-4c40-9219-a208d6c92cb5
📒 Files selected for processing (2)
crates/minibf/src/mapping.rscrates/minibf/src/routes/blocks.rs
|
@coderabbitai your summary is wrong. Bug fixes section includes |
|
🐇 ✅ Action performedSummary regeneration triggered. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai On the silently-skipped missing deps: this is deliberate, and consistent with the crate's existing behavior — Making one endpoint strict (or noisy) while its siblings degrade silently would trade a consistency problem for a visibility one. The right fix is crate-wide — e.g. a single policy at the 🤖 Generated with Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
A crate-wide policy at the 🐇💭 ✏️ Learnings added
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
output_at indexes regular outputs only, so an input spending a failed tx's collateral return (produced at index |outputs|) would not resolve and its address would be omitted. produces_at is validity-aware and matches the existing lookup paths in the addresses/assets routes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c4052ad to
0a5320b
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the dolos-minibf implementation of Blockfrost’s /blocks/{hash_or_number}/addresses endpoint so it returns one entry per affected address (with the transactions that touched it) rather than a flat list of transaction hashes, aligning the API shape with Blockfrost’s contract.
Changes:
- Updated
/blocks/{hash_or_number}/addressesto returnVec<BlockContentAddressesInner>and paginate address entries (ignoringorder, matching Blockfrost). - Added dependency collection/loading to resolve input-side addresses by fetching referenced transactions’ CBOR (with graceful skipping when deps are missing).
- Implemented address grouping + per-address tx deduplication and added a focused test suite for ordering, pagination, and error cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/minibf/src/routes/blocks.rs | Switches the addresses endpoint response type/semantics, loads input dependencies via get_tx_batch, and adds endpoint-level tests. |
| crates/minibf/src/mapping.rs | Extends BlockModelBuilder with dependency storage + input-dep discovery, and maps blocks to grouped affected-address entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #1068
Problem
/blocks/{hash_or_number}/addressesreturned a plain list of tx hashes — the same shape as/blocks/{hash_or_number}/txs— instead of Blockfrost's contract: one entry per affected address with the transactions that affected it. The existingIntoModel<Vec<BlockContentAddressesInner>>impl was dead code, and it only looked at outputs, emitted one entry per output (no grouping), andunwrap()ed address decoding.Implementation
An address is affected by a tx if it received a regular output or had a UTxO spent by a regular input. Since inputs are only
(tx_hash, index)pointers, input-side addresses require resolving the referenced transactions — done with the same dep-resolution patternTxModelBuilderuses for/txs/{hash}/utxos:BlockModelBuilder::required_input_deps()collects the deduped source-tx hashes across the block, the handler fetches them viaget_tx_batch, andload_depfeeds them back for resolution during mapping.Response semantics (matching Blockfrost):
orderparam is ignored, as in Blockfrostcount/pagepaginate whole address entries; past-the-end pages return200 []Deliberate divergences from Blockfrost
tx_in/tx_out(valid txs only) — treated as a Blockfrost bug rather than behavior to reproduce. Documented in a code comment.200 []for/blocks/0/addresses/ the genesis hash; dolos returns 404 (the genesis block isn't in the archive). Kept simple deliberately — no practical traffic on that path.Testing
Verified locally against the official blockfrost-tests suite targeting a dolos preview node bootstrapped from a full snapshot and synced to tip:
Result: 4/4 passed —
blocks/d452532f…/addresses,blocks/1846333/addresses, and both?count=3&page=1variants, matching the pinned fixture responses (which include input-side addresses on a deep-history block).orderparam ignored, pagination, bad request, not found, fault-injected 500.cargo +nightly fmt --check,cargo clippy --workspace --all-targets --all-features(zero warnings),cargo test -p dolos-minibf(227 passing).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
orderparameter.Tests
count/pagepagination (including empty past-the-end pages), and 400/404/500 error handling.