Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change updates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
scarmuega
force-pushed
the
main
branch
2 times, most recently
from
295b558 to
09d0b81
Compare
|
@nelsonksh this is very reasonable, but I'm afraid that without support for field masks, the performance penalty of those who don't need the block can be prohibitive. Solving field mask should be a cross-cut concern, since it's needed in many endpoint, but it's particularly important in this one. Maybe we could implement in ad-hoc here while we wait for the long-term solution. Just a quick check: only if the field masks include the "block" field, we inject the data. |
![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)
src/serve/grpc/watch.rs (1)
154-173:⚠️ Potential issue | 🟠 MajorGate
AnyChainTx.blockby field mask; current code always injects it.Line 168 unconditionally sets
block: Some(...), which conflicts with the stated requirement to include block data only when the request field mask contains"block". Also, Line 171 remaps the same block CBOR per transaction.Proposed refactor
fn block_to_txs<C: LedgerContext>( block: &RawBlock, mapper: &interop::Mapper<C>, request: &u5c::watch::WatchTxRequest, ) -> Vec<u5c::watch::AnyChainTx> { - let body: &BlockBody = █ - let block = MultiEraBlock::decode(block).unwrap(); - let txs = block.txs(); + let raw_block: &BlockBody = block; + let decoded_block = MultiEraBlock::decode(raw_block).unwrap(); + let txs = decoded_block.txs(); + + let include_block = request + .field_mask + .as_ref() + .is_some_and(|m| m.paths.iter().any(|p| p == "block")); + + let mapped_block = include_block.then(|| u5c::watch::AnyChainBlock { + native_bytes: raw_block.to_vec().into(), + chain: Some(u5c::watch::any_chain_block::Chain::Cardano( + mapper.map_block_cbor(raw_block), + )), + }); txs.iter() .map(|x: &pallas::ledger::traverse::MultiEraTx<'_>| mapper.map_tx(x)) .filter(|tx| { request .predicate .as_ref() .is_none_or(|predicate| apply_predicate(predicate, tx)) }) .map(|x| u5c::watch::AnyChainTx { chain: Some(u5c::watch::any_chain_tx::Chain::Cardano(x)), - block: Some(u5c::watch::AnyChainBlock { - native_bytes: body.to_vec().into(), - chain: Some(u5c::watch::any_chain_block::Chain::Cardano( - mapper.map_block_cbor(body), - )), - }), + block: mapped_block.clone(), }) .collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serve/grpc/watch.rs` around lines 154 - 173, The code currently always sets u5c::watch::AnyChainTx.block per transaction; change this so you first detect whether the request's field mask includes "block" (e.g. inspect request.field_mask or helper that checks field names), compute the block CBOR and native_bytes once (call mapper.map_block_cbor(body) and body.to_vec()) before iterating txs, and then when building each AnyChainTx set block: Some(...) only if the mask indicates "block" is requested, otherwise set block: None; update the mapping expression that builds AnyChainTx.block to use the precomputed values instead of remapping per transaction (refer to AnyChainTx.block and mapper.map_block_cbor).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/serve/grpc/watch.rs`:
- Around line 154-173: The code currently always sets
u5c::watch::AnyChainTx.block per transaction; change this so you first detect
whether the request's field mask includes "block" (e.g. inspect
request.field_mask or helper that checks field names), compute the block CBOR
and native_bytes once (call mapper.map_block_cbor(body) and body.to_vec())
before iterating txs, and then when building each AnyChainTx set block:
Some(...) only if the mask indicates "block" is requested, otherwise set block:
None; update the mapping expression that builds AnyChainTx.block to use the
precomputed values instead of remapping per transaction (refer to
AnyChainTx.block and mapper.map_block_cbor).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/serve/grpc/watch.rs (1)
172-177: PrecomputeAnyChainBlockonce per block, not once per tx.Line 176 remaps identical block CBOR for every transaction. Hoisting this out of the tx loop reduces repeated CPU/allocation work when
include_blockis true.♻️ Proposed refactor
let body: &BlockBody = &raw_block; let block = MultiEraBlock::decode(raw_block).unwrap(); + let mapped_block = include_block.then(|| u5c::watch::AnyChainBlock { + native_bytes: body.to_vec().into(), + chain: Some(u5c::watch::any_chain_block::Chain::Cardano( + mapper.map_block_cbor(body), + )), + }); let txs = block.txs(); @@ .map(|x| u5c::watch::AnyChainTx { chain: Some(u5c::watch::any_chain_tx::Chain::Cardano(x)), - block: if include_block { - Some(u5c::watch::AnyChainBlock { - native_bytes: body.to_vec().into(), - chain: Some(u5c::watch::any_chain_block::Chain::Cardano( - mapper.map_block_cbor(body), - )), - }) - } else { - None - }, + block: mapped_block.clone(), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serve/grpc/watch.rs` around lines 172 - 177, The code remaps the same block CBOR for every transaction by constructing u5c::watch::AnyChainBlock inside the tx loop; instead, precompute the AnyChainBlock once when include_block is true (call mapper.map_block_cbor(body) and build the u5c::watch::AnyChainBlock value once) and reuse that value for each transaction’s block field (clone or reference as appropriate) so you avoid repeating the CPU/alloc work in the tx loop; update the logic that currently builds AnyChainBlock inline to use the precomputed variable (referencing AnyChainBlock, mapper.map_block_cbor, and include_block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/serve/grpc/watch.rs`:
- Around line 154-156: The current include_block calculation uses substring
matching (mask.paths.iter().any(|p| p.contains("block"))) which can incorrectly
match names like "blockchain"; update the check to follow FieldMask semantics by
matching full path "block" and also allowing nested fields via prefix "block." —
i.e., in the closure over mask.paths replace the contains("block") test with a
combined exact or prefix check (p == "block" || p.starts_with("block.")) so
include_block only becomes true for the actual "block" field or its subfields;
keep the surrounding logic using request.field_mask and mask.paths unchanged.
---
Nitpick comments:
In `@src/serve/grpc/watch.rs`:
- Around line 172-177: The code remaps the same block CBOR for every transaction
by constructing u5c::watch::AnyChainBlock inside the tx loop; instead,
precompute the AnyChainBlock once when include_block is true (call
mapper.map_block_cbor(body) and build the u5c::watch::AnyChainBlock value once)
and reuse that value for each transaction’s block field (clone or reference as
appropriate) so you avoid repeating the CPU/alloc work in the tx loop; update
the logic that currently builds AnyChainBlock inline to use the precomputed
variable (referencing AnyChainBlock, mapper.map_block_cbor, and include_block).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/serve/grpc/watch.rs`:
- Line 159: Replace the risky unwrap on MultiEraBlock::decode(raw_block) inside
the watch request handler: instead of let block =
MultiEraBlock::decode(raw_block).unwrap(); match or propagate the Result from
MultiEraBlock::decode(raw_block) (e.g., using match or .map_err()) and handle
the Err case by logging the decode error and returning a safe fallback response
(or an appropriate gRPC Status error) rather than panicking; update references
to the block variable accordingly so downstream code only runs when decode
succeeded and ensure the watch stream continues/returns a controlled error on
decode failures.
3 participants
Summary by CodeRabbit
New Features
Chore