Capnp encoding for queries #75
Conversation
WalkthroughAdds optional Cap'n Proto serialization to the client, migrates field selections from string literals to strongly-typed enums, and redesigns net-types around a Cap'n Proto schema with builder/reader plumbing, query/response types, tests, and related build adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Config
participant GetArrow
participant JSON as "JSON Path"
participant Capnp as "Cap'n Proto Path"
participant Server
Client->>Config: read serialization_format
Config-->>Client: SerializationFormat
Client->>GetArrow: request data (get_arrow)
alt format == Json
GetArrow->>JSON: get_arrow_impl_json()
JSON->>Server: send JSON query / accept JSON response
Server-->>JSON: JSON payload
JSON-->>GetArrow: parsed Arrow data
else format == CapnProto
GetArrow->>Capnp: get_arrow_impl_capnp()
Capnp->>Server: send Cap'n Proto query / accept capnp response
Server-->>Capnp: Cap'n Proto payload
Capnp-->>GetArrow: parsed Arrow data
end
GetArrow-->>Client: Arrow result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🪛 checkmake (0.2.2)hypersync-net-types/Makefile[warning] 1-1: Missing required phony target "all" (minphony) [warning] 1-1: Missing required phony target "clean" (minphony) [warning] 1-1: Missing required phony target "test" (minphony) 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 |
JonoPrest
force-pushed
the
jp/hack-efficient-queries
branch
from
fa82206 to
91d2fc4
Compare
commit f91c16a Author: Jono Prest <[email protected]> Date: Fri Oct 24 14:25:31 2025 +0200 Allow defining "include" and "exclude" filters on selections in query (#80) * Add exclude selections to query * Bump version * Use empty vecs instead of optionals for exclude selections * Change structure to allow and or semantics * Add fmt and clippy * Bump minor versions since the public api breaks * Add backwards compatability tests * Add serialization test for new queries * Add pretty assertions * Fix serialization compatability test and improve serialization efficiency * Use checksum addresses for passing tests * Add rc flag for release commit 14920fe Author: Jono Prest <[email protected]> Date: Thu Oct 16 17:34:00 2025 +0200 Handle null effectiveGas for zeta receipts (#81) commit a1ec5d3 Author: Özgür Akkurt <[email protected]> Date: Wed Sep 10 20:57:05 2025 +0600 format: make quantity compatible with bincode (#77) * format: make quantity compatible with bincode * improve code path * fix serde expect string * version bump commit 13b5362 Merge: fd505a4 0f67fb5 Author: Dmitry Zakharov <[email protected]> Date: Thu Sep 4 15:55:11 2025 +0400 Merge pull request #76 from enviodev/dz/improve-get-events-join-logic Improve Get Event join logic commit 0f67fb5 Author: Dmitry Zakharov <[email protected]> Date: Thu Sep 4 15:49:57 2025 +0400 Update EventResponse type commit 5f6d2a3 Author: Dmitry Zakharov <[email protected]> Date: Thu Sep 4 15:44:00 2025 +0400 Fixes after review commit 8208307 Author: Dmitry Zakharov <[email protected]> Date: Wed Sep 3 20:19:58 2025 +0400 Fix clippy commit 001c2ef Author: Dmitry Zakharov <[email protected]> Date: Wed Sep 3 20:05:20 2025 +0400 Improve Get Event join logic commit fd505a4 Author: Jason Smythe <[email protected]> Date: Thu Sep 4 13:17:47 2025 +0200 BlockNumber as integer rather than Hex for Sonic RPC (#72) * BlockNumber as integer rather than Hex for Sonic RPC Enhance Quantity deserialization to accept numeric values Updated the QuantityVisitor to handle both hex strings and numeric values (u64 and i64) for deserialization. Added tests to verify the correct handling of numeric JSON values. * bump hypersync format version * Improve deserializer for quantity * Bump versions again for release --------- Co-authored-by: Jono Prest <[email protected]> Merge branch 'main' into jp/hack-efficient-queries
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
hypersync-net-types/hypersync_net_types.capnp (3)
212-224: Reorder field declarations to match field numbers.The
joinMode @0field is declared last (Line 223), while fields@1through@10are declared first. While Cap'n Proto allows any declaration order, having the@0field at the end is confusing and hurts readability.Consider reordering to match the field numbers:
struct QueryBody { + joinMode @0 :JoinMode; logs @1 :List(Selection(LogFilter)); transactions @2 :List(Selection(TransactionFilter)); traces @3 :List(Selection(TraceFilter)); blocks @4 :List(Selection(BlockFilter)); includeAllBlocks @5 :Bool; fieldSelection @6 :FieldSelection; maxNumBlocks @7 :OptUInt64; maxNumTransactions @8 :OptUInt64; maxNumLogs @9 :OptUInt64; maxNumTraces @10 :OptUInt64; - joinMode @0 :JoinMode; }
236-242: Consider documenting the optional value pattern.The
OptUInt64andOptUInt8wrapper structs implement optional values in Cap'n Proto. While this pattern is valid, it's worth documenting whether the optionality is determined by:
- Checking if the struct is present (using Cap'n Proto's
has*()methods)- Checking if the
valuefield itself is set- Some other convention
This clarification would help downstream implementations correctly handle these optional values.
1-242: Add schema-level documentation.The schema would benefit from comments explaining:
- The purpose and usage of the schema
- The semantics of key patterns (Selection, Filter suffix fields, optional types)
- Examples of common query patterns
- Version compatibility notes
Consider adding file-level comments at the top and inline comments for complex structures.
hypersync-net-types/Makefile (1)
1-2: Mark the target as .PHONY.The
generate_capnp_typestarget doesn't produce a file with that name, so it should be declared as a phony target to ensure it always runs when invoked.Apply this diff:
+.PHONY: generate_capnp_types generate_capnp_types: capnp compile ./hypersync_net_types.capnp -o rust:./src/__generated__hypersync-client/src/lib.rs (2)
389-423: Prefer explicit Accept header and offload parsing to spawn_blocking
- Add Accept: application/x-capnp to make content negotiation explicit.
- Use tokio::task::spawn_blocking instead of block_in_place to avoid blocking a runtime worker thread under load.
async fn get_arrow_impl_json(&self, query: &Query) -> Result<(ArrowResponse, u64)> { @@ - if let Some(bearer_token) = &self.bearer_token { + if let Some(bearer_token) = &self.bearer_token { req = req.bearer_auth(bearer_token); } + // Response is a packed capnp message containing Arrow data + req = req.header("accept", "application/x-capnp"); @@ - let res = tokio::task::block_in_place(|| { - parse_query_response(&bytes).context("parse query response") - })?; + let bytes_cloned = bytes.clone(); + let res = tokio::task::spawn_blocking(move || parse_query_response(&bytes_cloned)) + .await + .map_err(|e| anyhow!("spawn_blocking join error: {e}"))??; @@ - Ok((res, bytes.len().try_into().unwrap())) + Ok((res, u64::try_from(bytes.len()).unwrap_or(u64::MAX))) }
424-464: Cap’n Proto path: add Accept header and avoid fallible usize→u64 unwrap
- Mirror Accept header.
- Use u64::try_from to avoid unwrap on exotic targets.
async fn get_arrow_impl_capnp(&self, query: &Query) -> Result<(ArrowResponse, u64)> { @@ - let res = req - .header("content-type", "application/x-capnp") + let res = req + .header("content-type", "application/x-capnp") + .header("accept", "application/x-capnp") .body(query_bytes) .send() .await .context("execute http req")?; @@ - let res = tokio::task::block_in_place(|| { - parse_query_response(&bytes).context("parse query response") - })?; + let bytes_cloned = bytes.clone(); + let res = tokio::task::spawn_blocking(move || parse_query_response(&bytes_cloned)) + .await + .map_err(|e| anyhow!("spawn_blocking join error: {e}"))??; @@ - Ok((res, bytes.len().try_into().unwrap())) + Ok((res, u64::try_from(bytes.len()).unwrap_or(u64::MAX))) }hypersync-net-types/src/trace.rs (2)
32-114: Avoid unchecked usize→u32 casts when initializing capnp listsUse u32::try_from(..) and propagate a capnp::Error on overflow. It’s defensive and consistent across builders.
- let mut from_list = builder.reborrow().init_from(self.from.len() as u32); + let mut from_list = builder + .reborrow() + .init_from(u32::try_from(self.from.len()).map_err(|_| { + capnp::Error::failed("trace.from length exceeds u32".into()) + })?); @@ - let mut to_list = builder.reborrow().init_to(self.to.len() as u32); + let mut to_list = builder + .reborrow() + .init_to(u32::try_from(self.to.len()).map_err(|_| { + capnp::Error::failed("trace.to length exceeds u32".into()) + })?); @@ - let mut addr_list = builder.reborrow().init_address(self.address.len() as u32); + let mut addr_list = builder + .reborrow() + .init_address(u32::try_from(self.address.len()).map_err(|_| { + capnp::Error::failed("trace.address length exceeds u32".into()) + })?); @@ - .init_call_type(self.call_type.len() as u32); + .init_call_type(u32::try_from(self.call_type.len()).map_err(|_| { + capnp::Error::failed("trace.call_type length exceeds u32".into()) + })?); @@ - .init_reward_type(self.reward_type.len() as u32); + .init_reward_type(u32::try_from(self.reward_type.len()).map_err(|_| { + capnp::Error::failed("trace.reward_type length exceeds u32".into()) + })?); @@ - let mut type_list = builder.reborrow().init_type(self.type_.len() as u32); + let mut type_list = builder + .reborrow() + .init_type(u32::try_from(self.type_.len()).map_err(|_| { + capnp::Error::failed("trace.type length exceeds u32".into()) + })?); @@ - let mut sighash_list = builder.reborrow().init_sighash(self.sighash.len() as u32); + let mut sighash_list = builder + .reborrow() + .init_sighash(u32::try_from(self.sighash.len()).map_err(|_| { + capnp::Error::failed("trace.sighash length exceeds u32".into()) + })?);
116-256: De-duplicate list parsing with small helpersfrom_reader repeats near-identical 20-byte address and 4-byte sighash loops. Extract helpers (e.g., read_addr_list(reader.get_from()?), read_sighash_list(...)) to cut repetition and reduce future bugs.
hypersync-net-types/src/response.rs (1)
4-27: DTOs align with client usage; optional: derive Deserialize for RollbackGuardArchiveHeight/ChainId are correct for JSON endpoints. If RollbackGuard may ever be JSON-returned, consider adding Deserialize now for symmetry. Otherwise, good as-is.
hypersync-client/src/simple_types.rs (1)
56-65: Document invariants with debug_asserts to guard unwraps in join pathOnlyLogJoinField assumes corresponding log join fields are present and original single-field selections are removed. Add debug_asserts after mutation to make this contract explicit and catch regressions in tests.
pub(crate) fn add_join_fields_to_selection(&self, field_selection: &mut FieldSelection) { match self.block { InternalJoinStrategy::NotSelected => (), InternalJoinStrategy::OnlyLogJoinField => { field_selection.log.insert(LOG_JOIN_FIELD_WITH_BLOCK); field_selection.block.remove(&BLOCK_JOIN_FIELD); + debug_assert!( + field_selection.log.contains(&LOG_JOIN_FIELD_WITH_BLOCK), + "LOG_JOIN_FIELD_WITH_BLOCK must be present for OnlyLogJoinField (block)" + ); } InternalJoinStrategy::FullJoin => { field_selection.log.insert(LOG_JOIN_FIELD_WITH_BLOCK); field_selection.block.insert(BLOCK_JOIN_FIELD); } } @@ InternalJoinStrategy::NotSelected => (), InternalJoinStrategy::OnlyLogJoinField => { field_selection.log.insert(LOG_JOIN_FIELD_WITH_TX); field_selection.transaction.remove(&TX_JOIN_FIELD); + debug_assert!( + field_selection.log.contains(&LOG_JOIN_FIELD_WITH_TX), + "LOG_JOIN_FIELD_WITH_TX must be present for OnlyLogJoinField (tx)" + ); } InternalJoinStrategy::FullJoin => { field_selection.log.insert(LOG_JOIN_FIELD_WITH_TX); field_selection.transaction.insert(TX_JOIN_FIELD); } }Also applies to: 80-86, 92-98
hypersync-net-types/src/log.rs (3)
22-54: Use u32::try_from for capnp list sizesMirror the defensive casting suggestion from trace.rs to avoid unchecked usize→u32 casts.
- let mut addr_list = builder.reborrow().init_address(self.address.len() as u32); + let mut addr_list = builder + .reborrow() + .init_address(u32::try_from(self.address.len()).map_err(|_| { + capnp::Error::failed("log.address length exceeds u32".into()) + })?); @@ - let mut topics_list = builder.reborrow().init_topics(self.topics.len() as u32); + let mut topics_list = builder + .reborrow() + .init_topics(u32::try_from(self.topics.len()).map_err(|_| { + capnp::Error::failed("log.topics length exceeds u32".into()) + })?); @@ - .init(i as u32, topic_vec.len() as u32); + .init( + i as u32, + u32::try_from(topic_vec.len()).map_err(|_| { + capnp::Error::failed("log.topic length exceeds u32".into()) + })?, + );
80-87: Remove stale commented codeOld TODO/commented lines around address_filter deserialization can be dropped to reduce noise; the real implementation is present below.
91-109: Avoid silent drops with ArrayVec; prefer try_push and log on overflowIf topics exceed capacity (schema change or malformed input), try_push will signal it instead of silently discarding. Optionally log::warn on overflow.
- if i < 4 && !topic_vec.is_empty() { - topics.push(topic_vec); - } + if !topic_vec.is_empty() { + if topics.try_push(topic_vec).is_err() { + // Consider logging to surface unexpected over-capacity conditions. + // log::warn!("topics capacity exceeded; extra topics dropped"); + } + }hypersync-net-types/src/block.rs (1)
44-80: Consider documenting the silent-drop behavior and adding observability.The silent dropping of invalid entries (hashes ≠32 bytes, miners ≠20 bytes) is confirmed to be an intentional, consistent pattern across all filter implementations (block.rs, log.rs, trace.rs, transaction.rs). However, the codebase provides no documentation or observability for this behavior.
While this defensive parsing approach provides resilience, consider:
- Adding a doc comment to explain why invalid entries are dropped (e.g., for malformed Cap'n Proto messages)
- Including debug logging when entries are skipped (useful for troubleshooting serialization issues)
- Tracking a metric/counter for monitoring data loss
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
hypersync-net-types/src/__generated__/hypersync_net_types_capnp.rsis excluded by!**/__generated__/**hypersync-net-types/src/__generated__/mod.rsis excluded by!**/__generated__/**
📒 Files selected for processing (19)
hypersync-client/Cargo.toml(2 hunks)hypersync-client/src/config.rs(1 hunks)hypersync-client/src/lib.rs(5 hunks)hypersync-client/src/preset_query.rs(7 hunks)hypersync-client/src/simple_types.rs(4 hunks)hypersync-client/tests/api_test.rs(4 hunks)hypersync-format/src/types/bloom_filter_wrapper.rs(2 hunks)hypersync-net-types/Cargo.toml(2 hunks)hypersync-net-types/Makefile(1 hunks)hypersync-net-types/build.rs(0 hunks)hypersync-net-types/hypersync_net_types.capnp(1 hunks)hypersync-net-types/src/block.rs(1 hunks)hypersync-net-types/src/lib.rs(2 hunks)hypersync-net-types/src/log.rs(1 hunks)hypersync-net-types/src/query.rs(1 hunks)hypersync-net-types/src/response.rs(1 hunks)hypersync-net-types/src/trace.rs(1 hunks)hypersync-net-types/src/transaction.rs(1 hunks)hypersync-net-types/src/types.rs(1 hunks)
💤 Files with no reviewable changes (1)
- hypersync-net-types/build.rs
🧰 Additional context used
🧬 Code graph analysis (11)
hypersync-net-types/src/types.rs (1)
hypersync-format/src/types/fixed_size_data.rs (1)
FixedSizeData(105-105)
hypersync-net-types/src/trace.rs (4)
hypersync-net-types/src/log.rs (8)
populate_builder(23-54)from_reader(57-116)cmp(154-156)partial_cmp(160-162)all(166-169)to_capnp(172-191)from_capnp(194-213)schema(226-230)hypersync-net-types/src/transaction.rs (10)
populate_builder(63-84)populate_builder(118-207)from_reader(87-114)from_reader(210-365)cmp(437-439)partial_cmp(443-445)all(449-452)to_capnp(455-566)from_capnp(569-680)schema(693-697)hypersync-net-types/src/lib.rs (5)
populate_builder(52-52)populate_builder(64-79)from_reader(54-56)from_reader(81-91)from(43-48)hypersync-schema/src/lib.rs (1)
trace(131-160)
hypersync-client/src/preset_query.rs (3)
hypersync-net-types/src/block.rs (2)
BlockField(264-267)all(145-148)hypersync-net-types/src/log.rs (2)
LogField(231-234)all(166-169)hypersync-net-types/src/transaction.rs (2)
TransactionField(698-701)all(449-452)
hypersync-net-types/src/log.rs (6)
hypersync-net-types/src/block.rs (6)
populate_builder(21-42)from_reader(45-79)all(145-148)to_capnp(151-196)from_capnp(199-244)schema(259-263)hypersync-net-types/src/trace.rs (6)
populate_builder(33-113)from_reader(116-256)all(323-326)to_capnp(329-365)from_capnp(368-404)schema(417-421)hypersync-net-types/src/lib.rs (5)
populate_builder(52-52)populate_builder(64-79)from_reader(54-56)from_reader(81-91)from(43-48)hypersync-format/src/types/bloom_filter_wrapper.rs (3)
new(27-29)from(103-105)from_bytes(58-62)hypersync-net-types/src/query.rs (1)
test_query_serde(457-496)hypersync-schema/src/lib.rs (1)
log(113-129)
hypersync-net-types/src/transaction.rs (6)
hypersync-net-types/src/block.rs (6)
populate_builder(21-42)from_reader(45-79)all(145-148)to_capnp(151-196)from_capnp(199-244)schema(259-263)hypersync-net-types/src/trace.rs (6)
populate_builder(33-113)from_reader(116-256)all(323-326)to_capnp(329-365)from_capnp(368-404)schema(417-421)hypersync-net-types/src/lib.rs (5)
populate_builder(52-52)populate_builder(64-79)from_reader(54-56)from_reader(81-91)from(43-48)hypersync-net-types/src/query.rs (1)
test_query_serde(457-496)hypersync-format/src/types/bloom_filter_wrapper.rs (3)
from(103-105)new(27-29)from_bytes(58-62)hypersync-schema/src/lib.rs (1)
transaction(61-111)
hypersync-client/src/simple_types.rs (3)
hypersync-net-types/src/block.rs (1)
BlockField(264-267)hypersync-net-types/src/log.rs (1)
LogField(231-234)hypersync-net-types/src/transaction.rs (1)
TransactionField(698-701)
hypersync-client/tests/api_test.rs (5)
hypersync-net-types/src/block.rs (2)
BlockField(264-267)all(145-148)hypersync-net-types/src/log.rs (2)
LogField(231-234)all(166-169)hypersync-net-types/src/transaction.rs (2)
TransactionField(698-701)all(449-452)hypersync-client/src/lib.rs (1)
new(70-93)hypersync-client/src/config.rs (2)
default(39-42)default(92-94)
hypersync-client/src/lib.rs (1)
hypersync-client/src/parse_response.rs (1)
parse_query_response(29-96)
hypersync-net-types/src/query.rs (6)
hypersync-net-types/src/block.rs (3)
BlockField(264-267)from_capnp(199-244)from_reader(45-79)hypersync-net-types/src/log.rs (3)
LogField(231-234)from_capnp(194-213)from_reader(57-116)hypersync-net-types/src/trace.rs (3)
TraceField(422-425)from_capnp(368-404)from_reader(116-256)hypersync-net-types/src/transaction.rs (4)
TransactionField(698-701)from_capnp(569-680)from_reader(87-114)from_reader(210-365)hypersync-client/src/preset_query.rs (2)
logs(66-82)transactions(120-133)hypersync-net-types/src/lib.rs (2)
from_reader(54-56)from_reader(81-91)
hypersync-net-types/src/block.rs (6)
hypersync-net-types/src/log.rs (6)
populate_builder(23-54)from_reader(57-116)all(166-169)to_capnp(172-191)from_capnp(194-213)schema(226-230)hypersync-net-types/src/trace.rs (6)
populate_builder(33-113)from_reader(116-256)all(323-326)to_capnp(329-365)from_capnp(368-404)schema(417-421)hypersync-net-types/src/transaction.rs (8)
populate_builder(63-84)populate_builder(118-207)from_reader(87-114)from_reader(210-365)all(449-452)to_capnp(455-566)from_capnp(569-680)schema(693-697)hypersync-net-types/src/lib.rs (5)
populate_builder(52-52)populate_builder(64-79)from_reader(54-56)from_reader(81-91)from(43-48)hypersync-net-types/src/query.rs (1)
test_query_serde(457-496)hypersync-schema/src/lib.rs (1)
block_header(27-59)
hypersync-net-types/src/lib.rs (4)
hypersync-net-types/src/block.rs (2)
populate_builder(21-42)from_reader(45-79)hypersync-net-types/src/log.rs (2)
populate_builder(23-54)from_reader(57-116)hypersync-net-types/src/trace.rs (2)
populate_builder(33-113)from_reader(116-256)hypersync-net-types/src/transaction.rs (4)
populate_builder(63-84)populate_builder(118-207)from_reader(87-114)from_reader(210-365)
🪛 checkmake (0.2.2)
hypersync-net-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 GitHub Actions: CI
hypersync-client/Cargo.toml
[error] 1-1: Process completed with exit code 101 during cargo test --release.
hypersync-net-types/Cargo.toml
[error] 1-1: Process completed with exit code 101 during cargo test --release.
hypersync-client/tests/api_test.rs
[error] 563-563: test_api_capnp_client panicked: called Result::unwrap() on an Err value: start inner stream -> Caused by: error sending request for url (http://localhost:1131/height) -> Connection refused (os error 111). The integration test failed due to the local server not starting or being unavailable.
🔇 Additional comments (50)
hypersync-net-types/hypersync_net_types.capnp (5)
26-29: LGTM: Generic Selection pattern.The generic
Selection(T)struct with include/exclude semantics is a clean pattern for filtering.
32-74: Clarify the "Filter" suffix field pattern.Several filter structs have dual patterns: list fields (e.g.,
address @0 :List(Data)) and singular "Filter" suffix fields (e.g.,addressFilter @1 :Data). The semantic difference between these patterns is not documented in the schema.For example,
LogFilterhas bothaddress(list) andaddressFilter(singular data), whileBlockFilteronly has list fields. This inconsistency and lack of documentation could lead to incorrect usage.Consider adding comments to explain:
- What the "Filter" suffix fields represent (bloom filters? bitmap filters?)
- When to use list fields vs. filter fields
- Why
BlockFilterdoesn't follow this pattern
76-81: LGTM: Field selection pattern.The
FieldSelectionstruct with lists of field enums enables efficient field-level filtering to reduce payload size.
83-87: LGTM: JoinMode enum.Clear enumeration of join modes for query behavior.
89-210: LGTM: Comprehensive field enums.The field enums (
BlockField,TransactionField,LogField,TraceField) provide extensive coverage of blockchain data fields, including L1/L2 optimizations and EIP-4844 blob fields. Sequential numbering is correct.hypersync-format/src/types/bloom_filter_wrapper.rs (2)
18-24: LGTM: Equality implementation is correct.The
PartialEqandEqimplementations correctly compare bloom filters by their byte representations, which is the appropriate way to check equality for this wrapper type.
58-62: LGTM: Deserialization method correctly implemented.The
from_bytesmethod provides a clean constructor for deserializing bloom filters from raw bytes, properly handling errors via theError::BloomFilterFromBytesvariant.hypersync-client/Cargo.toml (2)
3-3: LGTM: Version bump aligns with release candidate progression.
50-50: LGTM: Dependency version aligned with net-types changes.hypersync-net-types/src/types.rs (1)
1-3: LGTM: Type alias improves clarity and type safety.The
Sighashtype alias correctly represents 4-byte Ethereum function signatures usingFixedSizeData<4>, providing better semantic meaning than using the raw type directly.hypersync-net-types/Cargo.toml (3)
3-3: LGTM: Version bump aligns with release candidate progression.
14-16: LGTM: Dependencies support enum-based field selections.The addition of
schemars,strum, andstrum_macrosappropriately supports the migration to strongly-typed field enums (BlockField, TransactionField, LogField) introduced in this PR.
20-20: LGTM: Dev dependency added for testing.hypersync-client/src/config.rs (2)
24-27: LGTM: New configuration field enables serialization format selection.The
serialization_formatfield properly uses#[serde(default)]to ensure backward compatibility with existing configurations that don't specify this field.
29-43: LGTM: Well-designed enum with backward-compatible default.The
SerializationFormatenum provides a clear choice between JSON and Cap'n Proto serialization. The default implementation maintains backward compatibility while the comment explains the transitional nature, which is helpful for future maintainers.hypersync-client/src/preset_query.rs (6)
6-8: LGTM: Imports support type-safe field selections.
42-46: LGTM: Consistent use of typed field enums.
67-67: LGTM: Using all() method from LogField enum.
98-98: LGTM: Consistent enum usage for log fields.
121-121: LGTM: Transaction field enum usage.
144-144: LGTM: Consistent pattern throughout the file.hypersync-client/tests/api_test.rs (3)
5-12: LGTM: Updated imports support type-safe field selections and Cap'n Proto testing.
21-23: LGTM: Migration to type-safe BlockField enum.
453-455: LGTM: Consistent use of TransactionField enum.hypersync-client/src/lib.rs (3)
38-38: Public re-export looks goodRe-exporting SerializationFormat at the crate root is a helpful API addition for callers. No issues.
64-66: Config wiring for serialization_format is correctField added and initialized from ClientConfig.serialization_format; no behavior risk detected.
Also applies to: 91-91
466-472: Dispatching by SerializationFormat is clearMatch on SerializationFormat with dedicated impls keeps concerns separated. LGTM.
hypersync-net-types/src/trace.rs (3)
7-30: TraceFilter shape and serde look correctFields and serde(rename = "type") align with hypersync-schema::trace(). No issues.
259-405: TraceField enum and capnp mappings are comprehensiveVariant set matches schema, with stable ordering and to_capnp/from_capnp symmetry. LGTM.
407-482: Tests meaningfully guard schema/mapping/serdeThe schema parity check and round-trip tests are valuable here. Nice coverage.
hypersync-client/src/simple_types.rs (1)
9-11: Move to typed field enums eliminates string driftImporting BlockField/TransactionField/LogField and using typed consts is a solid improvement.
Also applies to: 31-34
hypersync-net-types/src/log.rs (2)
119-214: LogField enum and capnp mappings look correctFull coverage with stable ordering and symmetric conversions. LGTM.
216-295: Tests cover schema parity and serde round-tripsGood unit coverage and realistic full-value case.
hypersync-net-types/src/block.rs (4)
6-18: LGTM!The BlockFilter structure is well-documented and follows consistent patterns with the other filter types (LogFilter, TransactionFilter) in the codebase. The use of empty vectors to represent "match all" semantics is clearly documented.
21-42: LGTM!The
populate_builderimplementation correctly follows the Cap'n Proto builder pattern and is consistent with similar implementations in log.rs and trace.rs. The use ofreborrow()prevents borrow checker issues when populating multiple fields.
82-245: LGTM!The BlockField enum and its Cap'n Proto conversion methods are well-implemented:
- Exhaustive matches in
to_capnp()andfrom_capnp()ensure compile-time safety if the schema changes- The
Ordimplementation based on string representation provides consistent ordering- Using strum's
IntoEnumIteratorfor theall()method is idiomatic and maintainable
247-308: LGTM!The test suite provides good coverage:
- Schema alignment verification ensures BlockField variants match the Arrow schema
- Serde/strum consistency check prevents serialization mismatches
- Round-trip test with actual data validates the Cap'n Proto implementation
hypersync-net-types/src/query.rs (4)
108-128: LGTM!The Cap'n Proto serialization methods follow standard patterns and use packed serialization for efficiency.
130-255: LGTM!The
populate_capnp_querymethod correctly populates all fields and uses proper reborrow patterns. The comment "Hehe" at line 142 is a bit informal but doesn't affect functionality.
344-446: LGTM!The parsing logic for max limits, join mode, and selections correctly propagates errors and follows consistent patterns.
457-627: LGTM!The test suite provides comprehensive coverage including edge cases (default values, explicit defaults, large payloads). The benchmarking output is useful for performance analysis, though printing in tests is unconventional.
hypersync-net-types/src/lib.rs (3)
1-49: LGTM!The module organization is clean and the re-exports maintain backward compatibility. The
Selection<T>struct with include/exclude semantics is well-documented.
51-57: LGTM!The
BuilderReadertrait provides a clean abstraction for Cap'n Proto serialization/deserialization and is appropriately scoped aspub(crate).
59-92: LGTM!The
BuilderReaderimplementation forSelection<T>correctly handles the include/exclude pattern with proper borrow management using scoped blocks andreborrow().hypersync-net-types/src/transaction.rs (6)
1-58: LGTM!The
AuthorizationSelectionandTransactionFilterstructures are well-documented and follow consistent patterns with other filter types. The use ofFilterWrapperfor probabilistic filtering is clearly documented.
60-115: LGTM with caveat.The
BuilderReaderimplementation forAuthorizationSelectionfollows the same pattern as other filter types. Note that it shares the same behavior of silently dropping invalid address entries (line 105), as discussed in the block.rs review.
117-207: LGTM!The
populate_builderimplementation forTransactionFiltercorrectly handles all fields including nestedAuthorizationSelectionentries.
209-366: LGTM with caveat.The
from_readerimplementation forTransactionFiltercorrectly handles all fields including nested authorization lists. It shares the same silent-drop behavior for invalid entries as noted in earlier reviews. The error handling forFilterWrapperdeserialization is appropriate.
368-681: LGTM!The
TransactionFieldenum and conversion methods are well-implemented with exhaustive matches ensuring compile-time safety against schema changes.
683-824: LGTM!The test suite provides comprehensive coverage including nested
AuthorizationSelectionand various edge cases. The schema alignment test ensures consistency with the Arrow schema.
| let all_block_fields = BlockField::all(); | ||
| let all_tx_fields = TransactionField::all(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Excellent refactor: Type-safe field enums prevent typos.
The migration from string-based field names to strongly-typed BlockField and TransactionField enums improves code maintainability and catches field name errors at compile time rather than runtime.
🤖 Prompt for AI Agents
In hypersync-client/src/preset_query.rs around lines 18 to 19, the migration to
use the type-safe enums BlockField::all() and TransactionField::all() is correct
and requires no changes; keep these enum-based fields as-is and ensure any
downstream code that previously expected string field names has been updated to
accept the enum types (or use a conversion method) so compilation succeeds.
| #[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq)] | ||
| pub struct Query { | ||
| /// The block to start the query from | ||
| pub from_block: u64, | ||
| /// The block to end the query at. If not specified, the query will go until the | ||
| /// end of data. Exclusive, the returned range will be [from_block..to_block). | ||
| /// | ||
| /// The query will return before it reaches this target block if it hits the time limit | ||
| /// configured on the server. The user should continue their query by putting the | ||
| /// next_block field in the response into from_block field of their next query. This implements | ||
| /// pagination. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub to_block: Option<u64>, | ||
| /// List of log selections, these have an OR relationship between them, so the query will return logs | ||
| /// that match any of these selections. | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub logs: Vec<LogSelection>, | ||
| /// List of transaction selections, the query will return transactions that match any of these selections | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub transactions: Vec<TransactionSelection>, | ||
| /// List of trace selections, the query will return traces that match any of these selections | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub traces: Vec<TraceSelection>, | ||
| /// List of block selections, the query will return blocks that match any of these selections | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub blocks: Vec<BlockSelection>, | ||
| /// Weather to include all blocks regardless of if they are related to a returned transaction or log. Normally | ||
| /// the server will return only the blocks that are related to the transaction or logs in the response. But if this | ||
| /// is set to true, the server will return data for all blocks in the requested range [from_block, to_block). | ||
| #[serde(default, skip_serializing_if = "is_default")] | ||
| pub include_all_blocks: bool, | ||
| /// Field selection. The user can select which fields they are interested in, requesting less fields will improve | ||
| /// query execution time and reduce the payload size so the user should always use a minimal number of fields. | ||
| #[serde(default, skip_serializing_if = "is_default")] | ||
| pub field_selection: FieldSelection, | ||
| /// Maximum number of blocks that should be returned, the server might return more blocks than this number but | ||
| /// it won't overshoot by too much. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub max_num_blocks: Option<usize>, | ||
| /// Maximum number of transactions that should be returned, the server might return more transactions than this number but | ||
| /// it won't overshoot by too much. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub max_num_transactions: Option<usize>, | ||
| /// Maximum number of logs that should be returned, the server might return more logs than this number but | ||
| /// it won't overshoot by too much. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub max_num_logs: Option<usize>, | ||
| /// Maximum number of traces that should be returned, the server might return more traces than this number but | ||
| /// it won't overshoot by too much. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub max_num_traces: Option<usize>, | ||
| /// Selects join mode for the query, | ||
| /// Default: join in this order logs -> transactions -> traces -> blocks | ||
| /// JoinAll: join everything to everything. For example if logSelection matches log0, we get the | ||
| /// associated transaction of log0 and then we get associated logs of that transaction as well. Applites similarly | ||
| /// to blocks, traces. | ||
| /// JoinNothing: join nothing. | ||
| #[serde(default, skip_serializing_if = "is_default")] | ||
| pub join_mode: JoinMode, | ||
| } | ||
|
|
||
| /// Used to skip serializing a defaulted serde field if | ||
| /// the value matches the default value. | ||
| fn is_default<T: Default + PartialEq>(t: &T) -> bool { | ||
| t == &T::default() | ||
| } |
There was a problem hiding this comment.
Minor typo in documentation.
Line 65: "Applites similarly" should be "Applies similarly"
Otherwise, the Query structure is well-documented and clearly defines the semantics of block ranges, selections, and join modes.
🤖 Prompt for AI Agents
In hypersync-net-types/src/query.rs around lines 63 to 66, there's a typo in the
doc comment: replace "Applites similarly" with "Applies similarly" so the
sentence reads correctly; update the comment text only (no code logic changes)
and run a quick doc build or linter to confirm formatting.
| let block_fields = if fs.has_block() { | ||
| let block_list = fs.get_block()?; | ||
| (0..block_list.len()) | ||
| .map(|i| { | ||
| BlockField::from_capnp( | ||
| block_list | ||
| .get(i) | ||
| .ok() | ||
| .unwrap_or(hypersync_net_types_capnp::BlockField::Number), | ||
| ) | ||
| }) | ||
| .collect::<BTreeSet<_>>() | ||
| } else { | ||
| BTreeSet::new() | ||
| }; | ||
|
|
||
| let transaction_fields = | ||
| if fs.has_transaction() { | ||
| let tx_list = fs.get_transaction()?; | ||
| (0..tx_list.len()) | ||
| .map(|i| { | ||
| TransactionField::from_capnp(tx_list.get(i).ok().unwrap_or( | ||
| hypersync_net_types_capnp::TransactionField::BlockHash, | ||
| )) | ||
| }) | ||
| .collect::<BTreeSet<_>>() | ||
| } else { | ||
| BTreeSet::new() | ||
| }; | ||
|
|
||
| let log_fields = | ||
| if fs.has_log() { | ||
| let log_list = fs.get_log()?; | ||
| (0..log_list.len()) | ||
| .map(|i| { | ||
| LogField::from_capnp(log_list.get(i).ok().unwrap_or( | ||
| hypersync_net_types_capnp::LogField::TransactionHash, | ||
| )) | ||
| }) | ||
| .collect::<BTreeSet<_>>() | ||
| } else { | ||
| BTreeSet::new() | ||
| }; | ||
|
|
||
| let trace_fields = | ||
| if fs.has_trace() { | ||
| let trace_list = fs.get_trace()?; | ||
| (0..trace_list.len()) | ||
| .map(|i| { | ||
| TraceField::from_capnp(trace_list.get(i).ok().unwrap_or( | ||
| hypersync_net_types_capnp::TraceField::TransactionHash, | ||
| )) | ||
| }) | ||
| .collect::<BTreeSet<_>>() | ||
| } else { | ||
| BTreeSet::new() | ||
| }; |
There was a problem hiding this comment.
Silent substitution of fallback field values masks errors.
Lines 280-286, 297-299, 311-313, and 325-327 use .ok().unwrap_or(...) with hardcoded fallback enum values when deserializing field selections:
- BlockField defaults to
Number - TransactionField defaults to
BlockHash - LogField defaults to
LogField::TransactionHash - TraceField defaults to
TraceField::TransactionHash
This approach silently substitutes arbitrary fields when:
- The Cap'n Proto enum value is unknown (e.g., from a newer schema version)
- The data is corrupted
- There's a deserialization error
This could result in queries returning incorrect data without any indication to the user.
Consider instead:
- Propagating the error:
block_list.get(i)?without the.ok().unwrap_or() - Returning a specific error when an unknown enum value is encountered
let block_fields = if fs.has_block() {
let block_list = fs.get_block()?;
(0..block_list.len())
- .map(|i| {
- BlockField::from_capnp(
- block_list
- .get(i)
- .ok()
- .unwrap_or(hypersync_net_types_capnp::BlockField::Number),
- )
- })
+ .map(|i| block_list.get(i).map(BlockField::from_capnp))
+ .collect::<Result<BTreeSet<_>, _>>()?
- .collect::<BTreeSet<_>>()
} else {
BTreeSet::new()
};Apply similar fixes for transaction, log, and trace fields.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
hypersync-net-types/src/__generated__/hypersync_net_types_capnp.rsis excluded by!**/__generated__/**
📒 Files selected for processing (2)
hypersync-client/tests/api_test.rs(4 hunks)hypersync-net-types/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hypersync-client/tests/api_test.rs
🧰 Additional context used
🪛 checkmake (0.2.2)
hypersync-net-types/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test_release
- GitHub Check: lint
🔇 Additional comments (1)
hypersync-net-types/Makefile (1)
2-2: Let me examine the complete Makefile to understand the full context and verify what line 3 references:Now let me check the complete Makefile to see line 3 and verify the full context:
All concerns in the review comment have been verified and resolved—no changes needed.
Based on verification of the codebase:
- The capnp compiler generates files following the pattern {schema_name}_capnp.rs, and the file
hypersync_net_types_capnp.rscorrectly follows this convention.- The schema file
hypersync_net_types.capnpexists athypersync-net-types/hypersync_net_types.capnp, which is in the correct working directory relative to the Makefile.- The output directory
./src/__generated__exists at the specified path and correctly receives the generated artifacts.- The capnp compiler generates source code in the specified directory with a default of the current directory, and the command correctly specifies
./src/__generated__for output.The Makefile command is properly configured and all path references are accurate.
| generate_capnp_types: | ||
| capnp compile hypersync_net_types.capnp -o rust:./src/__generated__ | ||
| rustfmt src/__generated__/hypersync_net_types_capnp.rs |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add .PHONY declaration and clean target for Makefile best practices.
The target needs .PHONY declaration since it doesn't produce a file with its name. Without it, the target won't execute if a file named generate_capnp_types exists. Additionally, a clean target should remove generated artifacts for build hygiene.
Apply this diff to add .PHONY and a clean target:
+.PHONY: generate_capnp_types clean
+
generate_capnp_types:
capnp compile hypersync_net_types.capnp -o rust:./src/__generated__
rustfmt src/__generated__/hypersync_net_types_capnp.rs
+
+clean:
+ rm -f src/__generated__/hypersync_net_types_capnp.rs🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In hypersync-net-types/Makefile lines 1 to 3, the makefile lacks a .PHONY
declaration for the generate_capnp_types target and has no clean target; add a
.PHONY: generate_capnp_types (and include clean there) and implement a clean
target that removes generated artifacts (e.g., delete
src/__generated__/hypersync_net_types_capnp.rs and/or the entire
src/__generated__ directory) so generated files are removed and the phony target
always runs.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores