Jp/hack negative queries #80
Conversation
WalkthroughRefactors hypersync-net-types to introduce a generic Selection and new Filter types (BlockFilter, LogFilter, TransactionFilter, TraceFilter) with From conversions; updates hypersync-client code/tests to construct selections via Selection::from(Filter); bumps hypersync-net-types and hypersync-client versions; reformats CI workflow and adds rustfmt/clippy to lint job. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Client as hypersync-client
participant Filter as *Filter (e.g., LogFilter)
participant Selection as Selection<T>
participant Query as Query Builder
Dev->>Client: call helper (e.g., logs_of_event)
Client->>Filter: build Filter struct (fields populated)
Filter-->>Client: Filter instance
Client->>Selection: Selection::from(Filter)
Selection-->>Client: Selection<Filter> (include=Filter, exclude=None)
Client->>Query: assemble Query using Selection<Filter>
Query-->>Dev: returned Query / executed request
note right of Selection: Selection allows later exclude modifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
🔇 Additional comments (2)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
JonoPrest
force-pushed
the
jp/hack-negative-queries
branch
from
237777d to
b566eab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yaml(1 hunks)hypersync-client/src/preset_query.rs(4 hunks)hypersync-client/tests/api_test.rs(2 hunks)hypersync-net-types/Cargo.toml(1 hunks)hypersync-net-types/src/lib.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
hypersync-client/tests/api_test.rs (1)
hypersync-net-types/src/lib.rs (1)
from(27-32)
hypersync-client/src/preset_query.rs (1)
hypersync-net-types/src/lib.rs (1)
from(27-32)
🪛 actionlint (1.7.7)
.github/workflows/ci.yaml
22-22: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
23-23: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-45: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
46-46: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
68-68: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
69-69: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (14)
.github/workflows/ci.yaml (2)
72-72: Good addition of explicit component dependencies.Explicitly declaring
rustfmtandclippyas required components ensures the lint job has the necessary tools. This is a best practice for CI reliability.
22-40: LGTM: Consistent formatting improvements.The indentation and structure changes improve readability and maintain consistency across all three jobs. The workflow logic remains unchanged aside from the beneficial addition of explicit components in the lint job.
Also applies to: 45-63, 68-87
hypersync-client/src/preset_query.rs (4)
6-8: LGTM! Import updates align with the new API.The imports correctly bring in the new
LogFilterandTransactionFiltertypes required for the refactored selection constructors.
86-89: LGTM! Correct usage of the new From-based construction.The
LogSelection::from(LogFilter { ... })pattern correctly leverages the new genericSelection<T>API, preserving the original filter semantics while adopting the new type structure.
121-125: LGTM! Consistent with the new filter construction pattern.The construction correctly uses
LogSelection::from(LogFilter { ... })with topics and address fields, maintaining the original query semantics.
175-178: LGTM! Transaction filter construction follows the established pattern.The
TransactionSelection::from(TransactionFilter { ... })correctly constructs the selection with thefromfield, consistent with the refactored API.hypersync-client/tests/api_test.rs (2)
8-8: LGTM! Import correctly updated for the new API.The addition of
TransactionFilterto the imports enables the test to use the new filter construction pattern.
461-464: LGTM! Test correctly updated to use the new From-based construction.The test properly constructs
TransactionSelectionviaTransactionSelection::from(TransactionFilter { ... }), using thefrom_filterfield with a bloom filter. This maintains the original test semantics while adopting the refactored API.hypersync-net-types/src/lib.rs (6)
26-33: LGTM! From implementation provides ergonomic construction.The
From<T>implementation allows users to constructSelection<T>from just a filter, defaultingexcludetoNone. This maintains backward compatibility for users who don't need negative filtering while enabling the new feature.
35-47: LGTM! BlockFilter correctly encapsulates block filtering logic.The
BlockSelectiontype alias andBlockFilterstruct cleanly separate the selection wrapper from the filter criteria. The hash and miner fields are well-documented with their empty-vector semantics.
49-63: LGTM! LogFilter properly encapsulates log filtering criteria.The
LogSelectionalias andLogFilterstruct correctly expose address, address_filter (bloom), and topics fields with clear documentation.
65-108: LGTM! TransactionFilter comprehensively covers transaction filtering.The
TransactionSelectionalias andTransactionFilterstruct provide extensive filtering capabilities including from/to addresses (with bloom filter support), sighash, status, type, contract address, hash, and authorization list. Field documentation is clear about AND/OR relationships.
120-145: LGTM! TraceFilter follows consistent design pattern.The
TraceSelectionalias andTraceFilterstruct maintain consistency with other filter types, providing from/to/address filtering (with bloom support), call_type, reward_type, kind, and sighash fields.
209-216: LGTM! Documentation added to JoinMode variants.The added documentation clarifies the behavior of each
JoinModevariant, improving API usability.
| - uses: actions/checkout@v3 | ||
| - uses: actions-rs/toolchain@v1 |
There was a problem hiding this comment.
Upgrade outdated GitHub Actions.
The static analysis tool correctly identifies that these actions are outdated and may fail on GitHub Actions:
actions/checkout@v3uses Node 16, which is deprecated. Upgrade toactions/checkout@v4.actions-rs/toolchain@v1is from an archived repository and is no longer maintained. Consider usingdtolnay/rust-toolchainor the officialactions/setup-rustinstead.
Apply this diff to upgrade the actions across all jobs:
- - uses: actions/checkout@v3
- - uses: actions-rs/toolchain@v1
+ - uses: actions/checkout@v4
+ - uses: dtolnay/rust-toolchain@stable
- with:
- toolchain: stable
- override: trueNote: dtolnay/rust-toolchain uses the version as a tag (e.g., @stable), so the with block is no longer needed.
Also applies to: 45-46, 68-69
🧰 Tools
🪛 actionlint (1.7.7)
22-22: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
23-23: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ci.yaml lines 22-23 (and similarly update lines 45-46 and
68-69): replace the outdated actions by upgrading actions/checkout@v3 to
actions/checkout@v4 and remove the archived actions-rs/toolchain@v1; instead use
a maintained Rust action such as actions/setup-rust@v1 (or
dtolnay/rust-toolchain@stable if you prefer tag syntax) and adjust any with:
block accordingly (dtolnay variant does not need a with block,
actions/setup-rust keeps the toolchain input).
There was a problem hiding this comment.
Please fix this as a separate PR
There was a problem hiding this comment.
@JasoonS, understood! I'll defer this change to a separate PR to keep this one focused on the negative queries work.
Would you like me to create a GitHub issue to track the GitHub Actions upgrade (actions/checkout@v3 → v4 and replacing actions-rs/toolchain@v1 with a maintained alternative like dtolnay/rust-toolchain)?
hypersync-net-types/Cargo.toml
Outdated
| [package] | ||
| name = "hypersync-net-types" | ||
| version = "0.10.0" | ||
| version = "0.10.1" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the version bump is appropriate for the API changes.
The version was bumped from 0.10.0 to 0.10.1 (patch), but the changes introduce new generic Selection<T> types and convert existing concrete types to type aliases. While the From<T> implementation may preserve some backward compatibility for construction, any code that pattern-matches or explicitly constructs these types will break.
For semver 0.x.y, breaking changes typically warrant a minor version bump (0.10.0 -> 0.11.0). Consider whether this patch bump is appropriate.
Let me verify if there are any external dependents that might be affected:
🏁 Script executed:
#!/bin/bash
# Description: Check if hypersync-net-types is used by external crates or only internally
# Expected: Find usage patterns that might break with this change
# Check for any Cargo.toml files that depend on hypersync-net-types
rg -t toml 'hypersync-net-types' --no-heading
# Look for direct struct construction patterns that would break
rg -n 'BlockSelection\s*\{' --type rust
rg -n 'LogSelection\s*\{' --type rust
rg -n 'TransactionSelection\s*\{' --type rust
rg -n 'TraceSelection\s*\{' --type rustLength of output: 201
Bump crate version to 0.11.0 — introducing the generic Selection<T> types and converting the old concrete types into aliases is a breaking change under 0.x semver rules; a patch bump to 0.10.1 isn’t sufficient. Update this crate’s version to 0.11.0 and adjust the hypersync-client dependency to "0.11" accordingly.
🤖 Prompt for AI Agents
In hypersync-net-types/Cargo.toml around line 3, the crate version was
incorrectly bumped to 0.10.1 for a breaking change; update the version to
"0.11.0" and then update the hypersync-client dependency reference to "0.11" to
match the new minor version so consumers see the semver-breaking change.
| #[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq)] | ||
| pub struct BlockSelection { | ||
| pub struct Selection<T> { | ||
| /// Filters where matching values should be included in the response | ||
| /// Default::default() means include everything | ||
| #[serde(default, flatten)] | ||
| pub include: T, | ||
| /// Filters where matching values should be excluded from the response | ||
| /// None means exclude nothing, Some(Default::default()) means exclude everything | ||
| #[serde(default)] | ||
| pub exclude: Option<T>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document the exclude field behavior and validate the design.
The new Selection<T> generic introduces an include/exclude pattern that enables negative queries (matching the PR title). However, the exclude field lacks documentation on:
- How
excludeinteracts withinclude(e.g., is it applied after include filtering?) - What
Some(Default::default())means in practice (exclude everything that passes include?) - Server-side implementation requirements for this pattern
Additionally, the #[serde(flatten)] on include will merge its fields into the parent during serialization, which may cause naming conflicts if T has an exclude field.
Consider adding comprehensive documentation:
#[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq)]
pub struct Selection<T> {
/// Filters where matching values should be included in the response
- /// Default::default() means include everything
+ /// Default::default() means include everything.
+ /// Items must match the include filter to be considered for the response.
#[serde(default, flatten)]
pub include: T,
/// Filters where matching values should be excluded from the response
- /// None means exclude nothing, Some(Default::default()) means exclude everything
+ /// None means exclude nothing. Some(Default::default()) means exclude everything.
+ /// Exclusion is applied after inclusion, so an item matching both include and exclude
+ /// filters will be excluded from the response.
#[serde(default)]
pub exclude: Option<T>,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq)] | |
| pub struct BlockSelection { | |
| pub struct Selection<T> { | |
| /// Filters where matching values should be included in the response | |
| /// Default::default() means include everything | |
| #[serde(default, flatten)] | |
| pub include: T, | |
| /// Filters where matching values should be excluded from the response | |
| /// None means exclude nothing, Some(Default::default()) means exclude everything | |
| #[serde(default)] | |
| pub exclude: Option<T>, | |
| } | |
| #[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq)] | |
| pub struct Selection<T> { | |
| /// Filters where matching values should be included in the response | |
| /// Default::default() means include everything. | |
| /// Items must match the include filter to be considered for the response. | |
| #[serde(default, flatten)] | |
| pub include: T, | |
| /// Filters where matching values should be excluded from the response | |
| /// None means exclude nothing. Some(Default::default()) means exclude everything. | |
| /// Exclusion is applied after inclusion, so an item matching both include and exclude | |
| /// filters will be excluded from the response. | |
| #[serde(default)] | |
| pub exclude: Option<T>, | |
| } |
Checked, they all build and they were part of the workspace to begin with 👍🏼 |
Summary by CodeRabbit
New Features
Refactor
Tests
Chores