patch: More work to get wildcard addresses to work (includes local patch to skar-client for testing) #11
Conversation
|
Sorry this is the main branch. The rebase is WIP. |
WalkthroughThis update introduces a local patch for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HypersyncClient
participant skar-client
participant Server
User->>HypersyncClient: Send Query
HypersyncClient->>skar-client: Convert & Mutate Query (normalize logs, extract selectors)
skar-client->>Server: Send HTTP Query
Server-->>skar-client: Query Response (blocks, txs, logs, traces)
skar-client-->>HypersyncClient: QueryResponse
HypersyncClient->>HypersyncClient: convert_response_to_events (parse, synthesize events)
HypersyncClient-->>User: Events (including synthesized from tx inputs)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
patch/skar-client-0.16.3/src/config.rs (1)
11-11: Fix typo in comment.There's a typo in the comment: "Timout" should be "Timeout".
- /// Timout treshold for a single http request in milliseconds, default is 30 seconds (30_000ms) + /// Timeout threshold for a single http request in milliseconds, default is 30 seconds (30_000ms)patch/skar-client-0.16.3/src/rayon_async.rs (1)
3-16: Consider improving error handling and adding documentation.The function correctly bridges Rayon's sync execution with Tokio's async runtime. However, there are two areas for improvement:
Silent error handling: Line 12 uses
.ok()which silently drops send errors. If the receiver is dropped before the result is sent, this will fail silently.Missing documentation: Consider adding documentation explaining the function's purpose and usage patterns.
+/// Spawns a closure on the Rayon thread pool and returns a receiver for the result. +/// +/// This function bridges Rayon's synchronous parallelism with Tokio's async runtime. +/// The closure will be executed on a Rayon thread and the result sent back via +/// a oneshot channel. +/// +/// # Note +/// If the receiver is dropped before the task completes, the result will be lost. pub fn spawn<F, T>(func: F) -> oneshot::Receiver<T> where F: 'static + FnOnce() -> T + Send, T: 'static + Send + Sync, { let (tx, rx) = oneshot::channel(); rayon::spawn(move || { let res = func(); - tx.send(res).ok(); + if tx.send(res).is_err() { + // Receiver was dropped, result is lost + } }); rx }patch/skar-client-0.16.3/test-data/nameless.abi.json (1)
1-1: Consider formatting the JSON for better readability.The ABI definitions appear correct and include both standard ERC721 events and complex lockup stream events with proper type definitions. However, the single-line format makes it difficult to review and maintain.
Consider formatting the JSON for better readability:
[ { "type": "event", "name": "Approval", "inputs": [ { "name": "owner", "type": "address", "indexed": true }, // ... other fields ], "anonymous": false }, // ... other events ]This would make the file easier to review, debug, and maintain while preserving the same functionality.
patch/skar-client-0.16.3/src/types.rs (1)
81-104: Consider adding bounds checking for column indexThe
columnmethod finds the column by name and accesses it by index. While the implementation looks correct, consider adding an explicit bounds check or usinggetinstead of indexing to make the error handling more explicit.impl ArrowBatch { pub fn column<T: 'static>(&self, name: &str) -> Result<&T> { match self .schema .fields .iter() .enumerate() .find(|(_, f)| f.name == name) { Some((idx, _)) => { let col = self .chunk .columns() .get(idx) - .context("get column")? + .ok_or_else(|| anyhow!("column index {} out of bounds", idx))? .as_any() .downcast_ref::<T>() - .context("cast column type")?; + .ok_or_else(|| anyhow!("failed to cast column '{}' to expected type", name))?; Ok(col) } None => Err(anyhow!("field {} not found in schema", name)), } } }patch/skar-client-0.16.3/src/parquet_out.rs (2)
200-205: Consider using drop guard pattern for cleanupThe manual dropping of senders could be replaced with a drop guard pattern to ensure cleanup happens even if the function returns early due to an error.
+ // Create a guard that will drop senders when it goes out of scope + struct SenderGuard { + blocks: Option<mpsc::Sender<ArrowBatch>>, + transactions: Option<mpsc::Sender<ArrowBatch>>, + logs: Option<mpsc::Sender<ArrowBatch>>, + traces: Option<mpsc::Sender<ArrowBatch>>, + decoded_logs: Option<mpsc::Sender<ArrowBatch>>, + } + + impl Drop for SenderGuard { + fn drop(&mut self) { + // Senders are automatically dropped + } + } + + let mut guard = SenderGuard { + blocks: Some(blocks_sender), + transactions: Some(transactions_sender), + logs: Some(logs_sender), + traces: Some(traces_sender), + decoded_logs: Some(decoded_logs_sender), + }; // ... rest of the function using guard.blocks.as_mut().unwrap() etc ... - std::mem::drop(blocks_sender); - std::mem::drop(transactions_sender); - std::mem::drop(logs_sender); - std::mem::drop(traces_sender); - std::mem::drop(decoded_logs_sender); + // Explicitly drop the guard to close channels + drop(guard);
509-509: Document the rationale for ROW_GROUP_MAX_ROWS valueThe constant
ROW_GROUP_MAX_ROWS = 10_000affects memory usage and performance. Consider adding a comment explaining why this specific value was chosen.+// Row group size chosen to balance memory usage and compression efficiency. +// Larger values improve compression but increase memory usage during encoding. const ROW_GROUP_MAX_ROWS: usize = 10_000;src/lib.rs (1)
572-588: Optimize topic matching algorithmThe current implementation has O(n*m) complexity where n is the number of matches and m is the number of logs. Consider optimizing this for better performance with large datasets.
let matches: Vec<[u8; 4]> = query .transactions .iter() .flat_map(|item| item.sighash.iter().map(|v: &FixedSizeData<4>| v.map(|v| v))) .collect(); + // Convert to HashSet for O(1) lookups + let matches_set: std::collections::HashSet<[u8; 4]> = matches.iter().copied().collect(); + let map: BTreeMap<[u8; 4], String> = matches .iter() .filter_map(|v| { let full_topic = query .logs .iter() .find_map(|l| l.topics[0].iter().find(|u| u[0..4] == *v)); full_topic.map(|full_topic| (*v, prefix_hex::encode(full_topic.to_vec()))) }) .collect(); // Later in the code: - if matches.is_empty() || !matches.contains(&selector.unwrap_or_default()) { + if matches_set.is_empty() || !matches_set.contains(&selector.unwrap_or_default()) {patch/skar-client-0.16.3/tests/api_test.rs (3)
46-102: Well-structured ordering testThis test properly verifies that logs are returned in ascending order by (block_number, log_index). The assertion logic is correct.
Minor suggestion: Consider improving the assertion message for better debugging:
- assert!(last < number, "last: {:?};number: {:?};", last, number); + assert!(last < number, "logs not in order: previous ({:?}) >= current ({:?})", last, number);
108-155: Consider adding more detailed assertionsWhile the test verifies that logs can be decoded, it only checks the count. Consider asserting on the decoded log content to ensure decoding is working correctly.
let decoded_logs = decoder.decode_logs(&res.data.logs).unwrap().unwrap(); assert_eq!(decoded_logs.len(), 1); + // Verify decoded log content + let decoded_log = &decoded_logs[0]; + assert_eq!(decoded_log.address(), address.as_slice(), "decoded address should match"); + // Add more assertions based on expected ENS token events
210-215: Add assertions to verify ABI parsingThe test only ensures parsing doesn't panic. Consider adding assertions to verify the parsed ABI structure:
fn parse_nameless_abi() { let path = get_file_path("nameless.abi.json"); let abi = std::fs::read_to_string(path).unwrap(); - let _abi: JsonAbi = serde_json::from_str(&abi).unwrap(); + let abi: JsonAbi = serde_json::from_str(&abi).unwrap(); + assert!(!abi.events.is_empty(), "ABI should contain events"); + // Verify specific properties of nameless ABI if applicable }patch/skar-client-0.16.3/src/column_mapping.rs (2)
111-187: Consistent mapping implementationThe type mapping functions follow a consistent pattern and correctly handle Binary and UInt64 source types. The
unwrap()calls afterdowncast_ref()are safe due to the preceding type checks in the match arms.Consider adding safety comments to document why the unwraps are safe:
// SAFETY: We checked data_type() is Binary, so downcast will succeed binary_to_target_array(col.as_any().downcast_ref::<BinaryArray<i32>>().unwrap())
189-206: Consider more descriptive error messagesThe binary conversion functions are well-implemented. Consider providing more context in the error message to aid debugging:
fn binary_to_target<T: TryFrom<U256>>(src: &[u8]) -> Result<T> { let big_num = U256::from_be_slice(src); big_num .try_into() - .map_err(|_e| anyhow!("failed to cast number to requested type")) + .map_err(|_e| anyhow!("failed to cast U256 value to target type (value too large)")) }patch/skar-client-0.16.3/src/lib.rs (1)
195-262: Consider extracting common retry logicThe retry logic in
send_with_retryis very similar toget_height_with_retry. While the current implementation is correct, you could reduce duplication by extracting a generic retry helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.gitignore(0 hunks)Cargo.toml(1 hunks)patch/skar-client-0.16.3/.cargo-ok(1 hunks)patch/skar-client-0.16.3/.cargo_vcs_info.json(1 hunks)patch/skar-client-0.16.3/Cargo.toml(1 hunks)patch/skar-client-0.16.3/Cargo.toml.orig(1 hunks)patch/skar-client-0.16.3/src/column_mapping.rs(1 hunks)patch/skar-client-0.16.3/src/config.rs(1 hunks)patch/skar-client-0.16.3/src/decode.rs(1 hunks)patch/skar-client-0.16.3/src/lib.rs(1 hunks)patch/skar-client-0.16.3/src/parquet_out.rs(1 hunks)patch/skar-client-0.16.3/src/rayon_async.rs(1 hunks)patch/skar-client-0.16.3/src/transport_format.rs(1 hunks)patch/skar-client-0.16.3/src/types.rs(1 hunks)patch/skar-client-0.16.3/test-data/ens_token_abi.json(1 hunks)patch/skar-client-0.16.3/test-data/erc20.abi.json(1 hunks)patch/skar-client-0.16.3/test-data/nameless.abi.json(1 hunks)patch/skar-client-0.16.3/tests/api_test.rs(1 hunks)src/decode.rs(1 hunks)src/lib.rs(10 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🧬 Code Graph Analysis (2)
patch/skar-client-0.16.3/src/config.rs (3)
index.d.ts (1)
Config(36-43)src/lib.rs (1)
new(31-35)patch/skar-client-0.16.3/src/lib.rs (1)
new(40-51)
patch/skar-client-0.16.3/src/column_mapping.rs (3)
index.d.ts (1)
ColumnMapping(22-27)patch/skar-client-0.16.3/src/types.rs (1)
chunk(91-97)patch/skar-client-0.16.3/src/parquet_out.rs (3)
chunk(231-242)schema(273-282)schema(367-371)
🔇 Additional comments (27)
patch/skar-client-0.16.3/src/config.rs (2)
5-14: Well-structured configuration module.The Config struct is well-designed with appropriate types and serde integration. The use of
NonZeroU64for timeout ensures a valid timeout value, and theUrltype provides built-in URL validation.
16-18: Safe default timeout implementation.The default timeout function is correctly implemented. Using
unwrap()is safe here since 30000 is guaranteed to be non-zero.patch/skar-client-0.16.3/.cargo-ok (1)
1-1: Standard Cargo metadata file.This is a standard
.cargo-okfile that Cargo uses to validate patch directories. The content is correct.patch/skar-client-0.16.3/.cargo_vcs_info.json (1)
1-6: Standard Cargo VCS metadata file.This is a standard
.cargo_vcs_info.jsonfile that Cargo uses to track version control information for the patch. The JSON structure and content are correct.Cargo.toml (1)
9-13: Proper Cargo patch configuration.The patch configuration is correctly set up to override the
skar-clientdependency with the local patch. The metadata section and patch.crates-io redirect are properly structured.src/decode.rs (1)
123-130: Confirm Zero-Address Fallback Removal and Test CoverageThe logic in src/decode.rs (around lines 123–130) now immediately returns None on a failed decode, with no fallback attempt against the zero address. Please verify that:
- This change is intentional and fully replaces the old zero-address fallback in favor of the new wildcard-address behavior.
- No other parts of the codebase still expect or rely on decoding with the zero address.
- Unit and integration tests cover the failure path and correctly exercise the wildcard address logic.
patch/skar-client-0.16.3/test-data/erc20.abi.json (1)
1-222: LGTM! Standard ERC20 ABI is correctly defined.This is a proper standard ERC20 ABI definition with all required functions and events. The JSON structure is well-formed and includes:
- All standard ERC20 functions with correct signatures and mutability
- Proper event definitions with indexed parameters
- Correct type specifications for all inputs/outputs
patch/skar-client-0.16.3/src/transport_format.rs (1)
7-39: LGTM! Well-designed transport format abstraction.The
TransportFormattrait provides a clean abstraction for different data formats, and theArrowIpcimplementation is solid:
- Proper error handling with context for debugging
- Efficient schema sharing using
Arc- Correct use of Arrow2 IPC reader APIs
- Clean iterator-based chunk processing
The static methods in the trait are appropriate for this use case where format selection is typically done at compile time.
patch/skar-client-0.16.3/Cargo.toml (1)
1-133: LGTM! Dependencies are well-configured.The Cargo.toml file shows a comprehensive set of dependencies with appropriate feature selections:
- Arrow2 includes necessary features for IPC and Parquet operations
- Tokio configured with required async runtime features
- Reqwest uses rustls-tls for secure HTTP communication
- Proper separation of dev-dependencies
- Version constraints appear reasonable
The dependency selection supports the crate's functionality for blockchain data processing and async operations.
patch/skar-client-0.16.3/Cargo.toml.orig (1)
45-47: All local path dependencies are correctly referenced and directories exist
- Verified that the
net-types,format, andschemadirectories exist relative topatch/skar-client-0.16.3.- No further action needed.
patch/skar-client-0.16.3/src/parquet_out.rs (1)
759-759: Fix error message grammarThere's a grammatical error in the error message.
- return Err(anyhow!("Internal error: Parsed type doesn't match resolved type. This should never happen.")); + return Err(anyhow!("Internal error: Parsed type doesn't match resolved type. This should never happen"));Likely an incorrect or invalid review comment.
src/lib.rs (1)
86-86: Clarify wildcard address detection logic across the codebaseI noticed that the code treats
0x0000…as a “no address” marker by checking only the first 19 bytes for zero ([0u8; 19]), whereas Ethereum addresses are 20 bytes long. Should we instead verify all 20 bytes ([0u8; 20])? Otherwise the 20th byte is never inspected.This pattern shows up in multiple places:
src/lib.rs(around line 86)patch/skar-client-0.16.3/src/decode.rs(two similar checks)Please confirm whether omitting the final byte is intentional. If not, update the checks (and comments) to cover all 20 bytes or adjust the comment to explain why only 19 are used.
patch/skar-client-0.16.3/tests/api_test.rs (6)
104-106: LGTM!Good use of
env!("CARGO_MANIFEST_DIR")to ensure test data paths are resolved correctly regardless of where tests are run from.
159-208: Good edge case testExcellent test coverage for the edge case of decoding a zero-value ERC20 transfer event. The test properly constructs the event data and verifies the decoded result.
275-303: LGTM!Good test coverage for the preset query helper. Assertions properly verify both the block count and presence of transactions.
305-333: LGTM!Consistent with the previous test pattern and properly validates the preset query for transaction hashes.
335-358: LGTM!Well-structured test for log queries with proper assertions on both the result count and block progression.
360-392: LGTM!Comprehensive test for event-specific log queries with proper use of hex literals for Ethereum data.
patch/skar-client-0.16.3/src/column_mapping.rs (3)
16-52: LGTM!Well-structured type definitions with appropriate serde attributes for configuration serialization. The
Fromtrait implementation provides clean conversion to Arrow types.
81-94: Good documentation of limitationsThe warning comment appropriately documents that this function doesn't validate type compatibility. This is a reasonable design choice for schema transformation.
96-109: LGTM!Clean dispatcher pattern with a helpful boxing utility function.
patch/skar-client-0.16.3/test-data/ens_token_abi.json (1)
1-2: LGTM!Valid ABI JSON file for testing. The minified format is acceptable for test data.
patch/skar-client-0.16.3/src/lib.rs (5)
1-30: LGTM!Well-organized imports and module structure with appropriate public re-exports for the crate's API.
60-107: Well-implemented retry logicExcellent implementation of exponential backoff with jitter for retries. The retry logic properly caps the backoff at 5 seconds and adds randomized jitter to prevent thundering herd issues.
109-161: LGTM!Well-designed streaming implementation with:
- Proper chunking of large block ranges
- Concurrent request handling with buffering
- Clean error propagation through the channel
264-333: LGTM!Thorough parsing implementation with proper handling of optional fields and appropriate error contexts. The high nesting limits are reasonable for complex blockchain data structures.
335-464: Excellent API designThe preset query methods provide a clean, user-friendly API for common query patterns. Good use of generics with TryInto bounds for flexible type handling.
| let event = match self.contracts.get(address) { | ||
| Some(contract) => match contract.get(topic0) { | ||
| Some(event) => event, | ||
| None => { | ||
| if let Some(event) = self.contracts.iter().find_map(|(address, contract)| { | ||
| if (*address).as_slice()[0..19] == [0u8; 19] { | ||
| contract.get(topic0) | ||
| } else { | ||
| None | ||
| } | ||
| }) { | ||
| event | ||
| } else { | ||
| return Ok(None); | ||
| } | ||
| } | ||
| }, | ||
| None => { | ||
| if let Some(event) = self.contracts.iter().find_map(|(address, contract)| { | ||
| if (*address).as_slice()[0..19] == [0u8; 19] { | ||
| contract.get(topic0) | ||
| } else { | ||
| None | ||
| } | ||
| }) { | ||
| event | ||
| } else { | ||
| return Ok(None); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated wildcard address lookup logic
The wildcard address lookup logic is duplicated. Consider extracting it into a helper method to improve maintainability and reduce code duplication.
+ fn find_wildcard_event(&self, topic0: &[u8]) -> Option<&DynSolEvent> {
+ self.contracts.iter().find_map(|(address, contract)| {
+ if (*address).as_slice()[0..19] == [0u8; 19] {
+ contract.get(topic0)
+ } else {
+ None
+ }
+ })
+ }
+
#[inline]
pub fn decode(
&self,
address: &[u8],
topic0: &[u8],
topics: &[Option<&[u8]>],
data: &[u8],
) -> Result<Option<DecodedEvent>> {
let event = match self.contracts.get(address) {
Some(contract) => match contract.get(topic0) {
Some(event) => event,
None => {
- if let Some(event) = self.contracts.iter().find_map(|(address, contract)| {
- if (*address).as_slice()[0..19] == [0u8; 19] {
- contract.get(topic0)
- } else {
- None
- }
- }) {
- event
- } else {
+ if let Some(event) = self.find_wildcard_event(topic0) {
+ event
+ } else {
return Ok(None);
}
}
},
None => {
- if let Some(event) = self.contracts.iter().find_map(|(address, contract)| {
- if (*address).as_slice()[0..19] == [0u8; 19] {
- contract.get(topic0)
- } else {
- None
- }
- }) {
- event
- } else {
+ if let Some(event) = self.find_wildcard_event(topic0) {
+ event
+ } else {
return Ok(None);
}
}
};📝 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.
| let event = match self.contracts.get(address) { | |
| Some(contract) => match contract.get(topic0) { | |
| Some(event) => event, | |
| None => { | |
| if let Some(event) = self.contracts.iter().find_map(|(address, contract)| { | |
| if (*address).as_slice()[0..19] == [0u8; 19] { | |
| contract.get(topic0) | |
| } else { | |
| None | |
| } | |
| }) { | |
| event | |
| } else { | |
| return Ok(None); | |
| } | |
| } | |
| }, | |
| None => { | |
| if let Some(event) = self.contracts.iter().find_map(|(address, contract)| { | |
| if (*address).as_slice()[0..19] == [0u8; 19] { | |
| contract.get(topic0) | |
| } else { | |
| None | |
| } | |
| }) { | |
| event | |
| } else { | |
| return Ok(None); | |
| } | |
| } | |
| }; | |
| fn find_wildcard_event(&self, topic0: &[u8]) -> Option<&DynSolEvent> { | |
| self.contracts.iter().find_map(|(address, contract)| { | |
| if (*address).as_slice()[0..19] == [0u8; 19] { | |
| contract.get(topic0) | |
| } else { | |
| None | |
| } | |
| }) | |
| } | |
| #[inline] | |
| pub fn decode( | |
| &self, | |
| address: &[u8], | |
| topic0: &[u8], | |
| topics: &[Option<&[u8]>], | |
| data: &[u8], | |
| ) -> Result<Option<DecodedEvent>> { | |
| let event = match self.contracts.get(address) { | |
| Some(contract) => match contract.get(topic0) { | |
| Some(event) => event, | |
| None => { | |
| if let Some(event) = self.find_wildcard_event(topic0) { | |
| event | |
| } else { | |
| return Ok(None); | |
| } | |
| } | |
| }, | |
| None => { | |
| if let Some(event) = self.find_wildcard_event(topic0) { | |
| event | |
| } else { | |
| return Ok(None); | |
| } | |
| } | |
| }; | |
| // ... rest of decode ... | |
| } |
🤖 Prompt for AI Agents
In patch/skar-client-0.16.3/src/decode.rs around lines 47 to 77, the logic for
wildcard address lookup is duplicated in two match arms. Extract this repeated
code into a separate helper function that iterates over contracts and returns
the event for the wildcard address if found. Then replace both duplicated blocks
with calls to this new helper function to improve maintainability and reduce
duplication.
| .decode( | ||
| address, | ||
| topic0, | ||
| &[Some(topic0), topic1, topic2, topic3], |
There was a problem hiding this comment.
Remove duplicate topic0 in topics array
The topic0 is included twice in the topics array - once as the second element and implicitly as part of the array. This seems incorrect as topic0 should only appear once.
let decoded = self
.decode(
address,
topic0,
- &[Some(topic0), topic1, topic2, topic3],
+ &[topic1, topic2, topic3],
data,
)
.context("decode event")?;📝 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.
| &[Some(topic0), topic1, topic2, topic3], | |
| let decoded = self | |
| .decode( | |
| address, | |
| topic0, | |
| &[topic1, topic2, topic3], | |
| data, | |
| ) | |
| .context("decode event")?; |
🤖 Prompt for AI Agents
In patch/skar-client-0.16.3/src/decode.rs at line 148, the topics array includes
`topic0` twice, once explicitly and once implicitly. Remove the duplicate
`topic0` so that it only appears once in the array to correct the topics list.
| fn mutate_query(&self, query: &mut skar_net_types::Query) { | ||
| let mut transactions = Vec::<TransactionSelection>::new(); | ||
|
|
||
| // 0x0000... is a special address that means no address | ||
| query.logs = query | ||
| .logs | ||
| .iter() | ||
| .map(|log| { | ||
| let mut log = log.clone(); | ||
| let address = log.address.clone(); | ||
| if address.into_iter().all(|v| v.as_ref()[0..19] == [0u8; 19]) { | ||
| log.address = vec![]; | ||
| } else { | ||
| let topics = log.topics.clone(); | ||
| let sighash: Option<Vec<FixedSizeData<32>>> = topics.first().cloned(); | ||
| if let Some(sighash) = sighash { | ||
| let sighash = sighash | ||
| .iter() | ||
| .map(|v| { | ||
| let mut data = [0u8; 4]; | ||
| data.copy_from_slice(&v.as_slice()[0..4]); | ||
| FixedSizeData::from(data) | ||
| }) | ||
| .collect(); | ||
| transactions.push(TransactionSelection { | ||
| to: log.address.clone(), | ||
| from: vec![], | ||
| sighash, | ||
| status: None, | ||
| }); | ||
| }; | ||
| } | ||
| log | ||
| }) | ||
| .collect(); | ||
| query.transactions = transactions; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for the mutate_query method
This method implements core functionality for wildcard address handling but lacks documentation explaining its purpose and behavior.
+ /// Mutates the query to handle wildcard addresses and extract transaction selections.
+ ///
+ /// - Wildcard addresses (0x0000...) are normalized by clearing the address field
+ /// - Transaction selections are built from log topics to enable transaction filtering
fn mutate_query(&self, query: &mut skar_net_types::Query) {🤖 Prompt for AI Agents
In src/lib.rs between lines 75 and 111, the mutate_query method lacks
documentation. Add a clear doc comment above the method explaining that it
processes query logs to handle wildcard addresses by replacing special zeroed
addresses with empty vectors, extracts and truncates sighash topics for
transaction selection, and updates the query's transactions accordingly. This
will clarify the method's purpose and behavior for future maintainers.
| log: Log { | ||
| block_hash: transaction.block_hash.clone(), | ||
| block_number: transaction.block_number, | ||
| log_index: transaction_index, |
There was a problem hiding this comment.
Potential log_index conflict in synthesized events
The synthesized log uses transaction_index as log_index, which could conflict with actual logs that have the same index. Consider using a different indexing strategy or documenting this behavior.
log_index: transaction_index,
+ // TODO: This could conflict with real logs. Consider using a different indexing strategy
+ // such as negative indices or a separate counter for synthesized logs📝 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.
| log_index: transaction_index, | |
| log_index: transaction_index, | |
| // TODO: This could conflict with real logs. Consider using a different indexing strategy | |
| // such as negative indices or a separate counter for synthesized logs |
🤖 Prompt for AI Agents
In src/lib.rs at line 624, the synthesized event's log_index is set to
transaction_index, which may conflict with actual log indices. To fix this,
implement a distinct indexing strategy for synthesized events, such as
offsetting the transaction_index by a fixed number or using a separate counter,
or clearly document this behavior to avoid confusion.
| let selector: Option<[u8; 4]> = if let Some(selector) = selector { | ||
| if selector.len() >= 4 { | ||
| if let Ok(decoded) = prefix_hex::decode::<[u8; 4]>(&selector[0..10]) { | ||
| Some(decoded) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
| if matches.is_empty() || !matches.contains(&selector.unwrap_or_default()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve selector extraction robustness
The selector extraction uses hardcoded string slicing [0..10] which assumes "0x" prefix plus 8 hex characters. Consider making this more robust.
- let selector: Option<[u8; 4]> = if let Some(selector) = selector {
- if selector.len() >= 4 {
- if let Ok(decoded) = prefix_hex::decode::<[u8; 4]>(&selector[0..10]) {
- Some(decoded)
- } else {
- None
- }
- } else {
- None
- }
- } else {
- None
- };
+ let selector: Option<[u8; 4]> = selector
+ .as_ref()
+ .and_then(|s| {
+ // Handle both "0x" prefixed and non-prefixed hex strings
+ let hex_str = s.strip_prefix("0x").unwrap_or(s);
+ if hex_str.len() >= 8 {
+ prefix_hex::decode::<[u8; 4]>(&hex_str[0..8]).ok()
+ } else {
+ None
+ }
+ });📝 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.
| let selector: Option<[u8; 4]> = if let Some(selector) = selector { | |
| if selector.len() >= 4 { | |
| if let Ok(decoded) = prefix_hex::decode::<[u8; 4]>(&selector[0..10]) { | |
| Some(decoded) | |
| } else { | |
| None | |
| } | |
| } else { | |
| None | |
| } | |
| } else { | |
| None | |
| }; | |
| if matches.is_empty() || !matches.contains(&selector.unwrap_or_default()) { | |
| continue; | |
| } | |
| let selector: Option<[u8; 4]> = selector | |
| .as_ref() | |
| .and_then(|s| { | |
| // Handle both "0x"-prefixed and non-prefixed hex strings | |
| let hex_str = s.strip_prefix("0x").unwrap_or(s); | |
| if hex_str.len() >= 8 { | |
| prefix_hex::decode::<[u8; 4]>(&hex_str[0..8]).ok() | |
| } else { | |
| None | |
| } | |
| }); | |
| if matches.is_empty() || !matches.contains(&selector.unwrap_or_default()) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In src/lib.rs around lines 591 to 606, the selector extraction uses a hardcoded
slice [0..10] assuming the selector string always starts with "0x" followed by 8
hex characters. To improve robustness, modify the code to dynamically handle the
"0x" prefix and ensure the slice covers exactly 8 hex characters after the
prefix. Validate the selector string length accordingly before decoding to avoid
panics or incorrect decoding.
| #[tokio::test(flavor = "multi_thread")] | ||
| #[ignore] | ||
| async fn test_parquet_out() { | ||
| env_logger::try_init().ok(); | ||
|
|
||
| let client = Client::new(Config { | ||
| url: URL.parse().unwrap(), | ||
| bearer_token: None, | ||
| http_req_timeout_millis: 20000.try_into().unwrap(), | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| let path = format!("{}/{}", temp_dir().to_string_lossy(), uuid::Uuid::new_v4()); | ||
|
|
||
| let query: Query = serde_json::from_value(serde_json::json!({ | ||
| "from_block": 19277345, | ||
| "to_block": 19277346, | ||
| "logs": [{ | ||
| "address": ["0xdAC17F958D2ee523a2206206994597C13D831ec7"], | ||
| "topics": [["0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef"]], | ||
| }], | ||
| "transactions": [{}], | ||
| "include_all_blocks": true, | ||
| "field_selection": { | ||
| "log": ["block_number"], | ||
| } | ||
| })) | ||
| .unwrap(); | ||
|
|
||
| client | ||
| .create_parquet_folder( | ||
| query, | ||
| ParquetConfig { | ||
| path, | ||
| hex_output: true, | ||
| batch_size: 100, | ||
| concurrency: 10, | ||
| retry: false, | ||
| column_mapping: ColumnMapping { | ||
| block: maplit::btreemap! { | ||
| "number".to_owned() => skar_client::DataType::Float32, | ||
| }, | ||
| transaction: maplit::btreemap! { | ||
| "value".to_owned() => skar_client::DataType::Float64, | ||
| }, | ||
| log: Default::default(), | ||
| trace: Default::default(), | ||
| decoded_log: maplit::btreemap! { | ||
| "value".to_owned() => skar_client::DataType::Float64, | ||
| }, | ||
| }, | ||
| event_signature: None, | ||
| }, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify Parquet output files
The test creates Parquet files but doesn't verify the output. Consider:
- Checking that files were created in the expected directory
- Verifying the Parquet file content or at least file existence
client
.create_parquet_folder(
query,
ParquetConfig {
path,
// ... config ...
},
)
.await
.unwrap();
+
+ // Verify output files were created
+ let output_dir = std::path::Path::new(&path);
+ assert!(output_dir.exists(), "Output directory should exist");
+
+ // Check for expected Parquet files
+ let entries = std::fs::read_dir(output_dir).unwrap();
+ let parquet_files: Vec<_> = entries
+ .filter_map(|e| e.ok())
+ .filter(|e| e.path().extension().map_or(false, |ext| ext == "parquet"))
+ .collect();
+ assert!(!parquet_files.is_empty(), "Should have created Parquet files");📝 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.
| #[tokio::test(flavor = "multi_thread")] | |
| #[ignore] | |
| async fn test_parquet_out() { | |
| env_logger::try_init().ok(); | |
| let client = Client::new(Config { | |
| url: URL.parse().unwrap(), | |
| bearer_token: None, | |
| http_req_timeout_millis: 20000.try_into().unwrap(), | |
| }) | |
| .unwrap(); | |
| let path = format!("{}/{}", temp_dir().to_string_lossy(), uuid::Uuid::new_v4()); | |
| let query: Query = serde_json::from_value(serde_json::json!({ | |
| "from_block": 19277345, | |
| "to_block": 19277346, | |
| "logs": [{ | |
| "address": ["0xdAC17F958D2ee523a2206206994597C13D831ec7"], | |
| "topics": [["0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef"]], | |
| }], | |
| "transactions": [{}], | |
| "include_all_blocks": true, | |
| "field_selection": { | |
| "log": ["block_number"], | |
| } | |
| })) | |
| .unwrap(); | |
| client | |
| .create_parquet_folder( | |
| query, | |
| ParquetConfig { | |
| path, | |
| hex_output: true, | |
| batch_size: 100, | |
| concurrency: 10, | |
| retry: false, | |
| column_mapping: ColumnMapping { | |
| block: maplit::btreemap! { | |
| "number".to_owned() => skar_client::DataType::Float32, | |
| }, | |
| transaction: maplit::btreemap! { | |
| "value".to_owned() => skar_client::DataType::Float64, | |
| }, | |
| log: Default::default(), | |
| trace: Default::default(), | |
| decoded_log: maplit::btreemap! { | |
| "value".to_owned() => skar_client::DataType::Float64, | |
| }, | |
| }, | |
| event_signature: None, | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| } | |
| client | |
| .create_parquet_folder( | |
| query, | |
| ParquetConfig { | |
| path, | |
| // ... config ... | |
| }, | |
| ) | |
| .await | |
| .unwrap(); | |
| + | |
| + // Verify output files were created | |
| + let output_dir = std::path::Path::new(&path); | |
| + assert!(output_dir.exists(), "Output directory should exist"); | |
| + | |
| + // Check for expected Parquet files | |
| + let entries = std::fs::read_dir(output_dir).unwrap(); | |
| + let parquet_files: Vec<_> = entries | |
| + .filter_map(|e| e.ok()) | |
| + .filter(|e| e.path().extension().map_or(false, |ext| ext == "parquet")) | |
| + .collect(); | |
| + assert!(!parquet_files.is_empty(), "Should have created Parquet files"); | |
| } |
🤖 Prompt for AI Agents
In patch/skar-client-0.16.3/tests/api_test.rs between lines 217 and 273, the
test_parquet_out function creates Parquet files but does not verify their
creation or content. To fix this, add assertions after the create_parquet_folder
call to check that files exist in the specified path directory. Optionally, open
the Parquet files and verify their schema or content to ensure correctness. This
will confirm the test's effectiveness in validating Parquet output.
| #[tokio::test(flavor = "multi_thread")] | ||
| #[ignore] | ||
| async fn test_api_arrow_ipc() { | ||
| let client = Client::new(Config { | ||
| url: URL.parse().unwrap(), | ||
| bearer_token: None, | ||
| http_req_timeout_millis: 20000.try_into().unwrap(), | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| let mut block_field_selection = BTreeSet::new(); | ||
| block_field_selection.insert("number".to_owned()); | ||
| block_field_selection.insert("timestamp".to_owned()); | ||
| block_field_selection.insert("hash".to_owned()); | ||
|
|
||
| let res = client | ||
| .send::<ArrowIpc>(&Query { | ||
| from_block: 14000000, | ||
| to_block: None, | ||
| logs: Vec::new(), | ||
| transactions: Vec::new(), | ||
| include_all_blocks: true, | ||
| field_selection: FieldSelection { | ||
| block: block_field_selection, | ||
| log: Default::default(), | ||
| transaction: Default::default(), | ||
| trace: Default::default(), | ||
| }, | ||
| ..Default::default() | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| dbg!(res.next_block); | ||
| } |
There was a problem hiding this comment.
Add assertions to verify test behavior
This test only prints debug output without asserting any expected behavior. Tests should verify that the API returns expected results.
Consider:
- Using the
URLconstant defined at line 157 instead ofURL.parse().unwrap() - Adding assertions to verify the response (e.g., checking
next_blockvalue, verifying returned data) - Using
expect()instead ofunwrap()for better error messages
async fn test_api_arrow_ipc() {
let client = Client::new(Config {
- url: URL.parse().unwrap(),
+ url: URL.parse().expect("valid URL"),
bearer_token: None,
http_req_timeout_millis: 20000.try_into().unwrap(),
})
- .unwrap();
+ .expect("client creation should succeed");
// ... query setup ...
let res = client
.send::<ArrowIpc>(&query)
.await
- .unwrap();
+ .expect("query should succeed");
- dbg!(res.next_block);
+ assert!(res.next_block > 14000000, "next_block should be after from_block");
+ assert!(!res.data.blocks.is_empty(), "should return block data");
}📝 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.
| #[tokio::test(flavor = "multi_thread")] | |
| #[ignore] | |
| async fn test_api_arrow_ipc() { | |
| let client = Client::new(Config { | |
| url: URL.parse().unwrap(), | |
| bearer_token: None, | |
| http_req_timeout_millis: 20000.try_into().unwrap(), | |
| }) | |
| .unwrap(); | |
| let mut block_field_selection = BTreeSet::new(); | |
| block_field_selection.insert("number".to_owned()); | |
| block_field_selection.insert("timestamp".to_owned()); | |
| block_field_selection.insert("hash".to_owned()); | |
| let res = client | |
| .send::<ArrowIpc>(&Query { | |
| from_block: 14000000, | |
| to_block: None, | |
| logs: Vec::new(), | |
| transactions: Vec::new(), | |
| include_all_blocks: true, | |
| field_selection: FieldSelection { | |
| block: block_field_selection, | |
| log: Default::default(), | |
| transaction: Default::default(), | |
| trace: Default::default(), | |
| }, | |
| ..Default::default() | |
| }) | |
| .await | |
| .unwrap(); | |
| dbg!(res.next_block); | |
| } | |
| #[tokio::test(flavor = "multi_thread")] | |
| #[ignore] | |
| async fn test_api_arrow_ipc() { | |
| let client = Client::new(Config { | |
| url: URL.parse().expect("valid URL"), | |
| bearer_token: None, | |
| http_req_timeout_millis: 20000.try_into().unwrap(), | |
| }) | |
| .expect("client creation should succeed"); | |
| let mut block_field_selection = BTreeSet::new(); | |
| block_field_selection.insert("number".to_owned()); | |
| block_field_selection.insert("timestamp".to_owned()); | |
| block_field_selection.insert("hash".to_owned()); | |
| let res = client | |
| .send::<ArrowIpc>(&Query { | |
| from_block: 14000000, | |
| to_block: None, | |
| logs: Vec::new(), | |
| transactions: Vec::new(), | |
| include_all_blocks: true, | |
| field_selection: FieldSelection { | |
| block: block_field_selection, | |
| log: Default::default(), | |
| transaction: Default::default(), | |
| trace: Default::default(), | |
| }, | |
| ..Default::default() | |
| }) | |
| .await | |
| .expect("query should succeed"); | |
| assert!(res.next_block > 14000000, "next_block should be after from_block"); | |
| assert!(!res.data.blocks.is_empty(), "should return block data"); | |
| } |
🤖 Prompt for AI Agents
In patch/skar-client-0.16.3/tests/api_test.rs between lines 10 and 44, the test
function only prints debug output without verifying expected behavior. Replace
URL.parse().unwrap() with the URL constant defined at line 157 for consistency.
Change unwrap() calls to expect() with descriptive error messages to improve
error reporting. Add assertions to check key response fields such as verifying
the value of res.next_block and any other relevant returned data to ensure the
API behaves as expected.
| pub fn apply_to_chunk( | ||
| chunk: &ArrowChunk, | ||
| field_names: &[&str], | ||
| mapping: &BTreeMap<String, DataType>, | ||
| ) -> Result<ArrowChunk> { | ||
| if mapping.is_empty() { | ||
| return Ok(chunk.clone()); | ||
| } | ||
|
|
||
| let columns = chunk | ||
| .columns() | ||
| .par_iter() | ||
| .zip(field_names.par_iter()) | ||
| .map(|(col, &field_name)| { | ||
| let col = match mapping.get(field_name) { | ||
| Some(&dt) => map_column(&**col, dt) | ||
| .context(format!("apply cast to colum '{}'", field_name))?, | ||
| None => col.clone(), | ||
| }; | ||
|
|
||
| Ok(col) | ||
| }) | ||
| .collect::<Result<Vec<_>>>()?; | ||
|
|
||
| Ok(ArrowChunk::new(columns)) | ||
| } |
There was a problem hiding this comment.
Fix typo in error context
There's a typo in the error context string.
Some(&dt) => map_column(&**col, dt)
- .context(format!("apply cast to colum '{}'", field_name))?,
+ .context(format!("apply cast to column '{}'", field_name))?,📝 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.
| pub fn apply_to_chunk( | |
| chunk: &ArrowChunk, | |
| field_names: &[&str], | |
| mapping: &BTreeMap<String, DataType>, | |
| ) -> Result<ArrowChunk> { | |
| if mapping.is_empty() { | |
| return Ok(chunk.clone()); | |
| } | |
| let columns = chunk | |
| .columns() | |
| .par_iter() | |
| .zip(field_names.par_iter()) | |
| .map(|(col, &field_name)| { | |
| let col = match mapping.get(field_name) { | |
| Some(&dt) => map_column(&**col, dt) | |
| .context(format!("apply cast to colum '{}'", field_name))?, | |
| None => col.clone(), | |
| }; | |
| Ok(col) | |
| }) | |
| .collect::<Result<Vec<_>>>()?; | |
| Ok(ArrowChunk::new(columns)) | |
| } | |
| pub fn apply_to_chunk( | |
| chunk: &ArrowChunk, | |
| field_names: &[&str], | |
| mapping: &BTreeMap<String, DataType>, | |
| ) -> Result<ArrowChunk> { | |
| if mapping.is_empty() { | |
| return Ok(chunk.clone()); | |
| } | |
| let columns = chunk | |
| .columns() | |
| .par_iter() | |
| .zip(field_names.par_iter()) | |
| .map(|(col, &field_name)| { | |
| let col = match mapping.get(field_name) { | |
| Some(&dt) => map_column(&**col, dt) | |
| .context(format!("apply cast to column '{}'", field_name))?, | |
| None => col.clone(), | |
| }; | |
| Ok(col) | |
| }) | |
| .collect::<Result<Vec<_>>>()?; | |
| Ok(ArrowChunk::new(columns)) | |
| } |
🤖 Prompt for AI Agents
In patch/skar-client-0.16.3/src/column_mapping.rs between lines 54 and 79, fix
the typo in the error context string from "apply cast to colum '{}'" to "apply
cast to column '{}'". This involves correcting the word "colum" to "column"
inside the format string used in the context method.
| impl Client { | ||
| /// Create a new client with given config | ||
| pub fn new(cfg: Config) -> Result<Self> { | ||
| let http_client = reqwest::Client::builder() | ||
| .no_gzip() | ||
| .http1_only() | ||
| .timeout(Duration::from_millis(cfg.http_req_timeout_millis.get())) | ||
| .tcp_keepalive(Duration::from_secs(7200)) | ||
| .connect_timeout(Duration::from_millis(cfg.http_req_timeout_millis.get())) | ||
| .build() | ||
| .unwrap(); | ||
|
|
||
| Ok(Self { http_client, cfg }) | ||
| } |
There was a problem hiding this comment.
Handle client builder errors properly
The unwrap() on line 48 could panic if client building fails. Return the error instead:
- .build()
- .unwrap();
+ .build()
+ .context("failed to build HTTP client")?;🤖 Prompt for AI Agents
In patch/skar-client-0.16.3/src/lib.rs around lines 38 to 51, the reqwest client
builder uses unwrap() which can cause a panic if building the client fails.
Replace unwrap() with proper error handling by propagating the error using the ?
operator so the new function returns the error instead of panicking.
1 participant
feat: Allow multiple wildcard addresses by varying the last byte of 0x000000...
Added a local patch for the skar-client for testing.
Sorry this was easier to get a built version with the changes to copy into node_modules.
Added non-wildcards to show up as both event and sigHash queries (since it's extremely unlikely there will be an overlap on the first 4 digits of the hash it's capable of monitoring both Events and sigHash of the same items. It will construct fake Log entries to correctly decode the function signature and parameters.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
skar-clientcrate.