fix(katana-rpc): include events from pending block in getEvents
#2375
Conversation
WalkthroughOhayo, sensei! The changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StarknetApi
participant EventService
participant TokenParser
Client->>StarknetApi: Request events with continuation token
StarknetApi->>TokenParser: Parse continuation token
TokenParser-->>StarknetApi: Return parsed token
StarknetApi->>EventService: Retrieve events based on parsed token
EventService-->>StarknetApi: Return filtered events
StarknetApi-->>Client: Send events response
Assessment against linked issues
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (5)
Files skipped from review due to trivial changes (2)
Additional comments not posted (21)
TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
Line range hint
335-552: Enhancements and Simplification Needed in Event HandlingOhayo, sensei! The modifications to the
eventsfunction significantly enhance its functionality, particularly in handling events from pending blocks and managing continuation tokens more robustly. The introduction of theFilteredEventsstruct is a commendable effort to improve code readability and maintainability.However, the complexity introduced by these changes might benefit from further simplification. The TODO comment at line 335 suggests the need for more documentation and possibly finding a simpler solution. It would be wise to address this to ensure the maintainability of the code.
Consider refactoring to simplify the logic and enhance documentation to make the codebase more maintainable and understandable for future contributors.
| } | ||
| Ordering::Greater => (), | ||
| } | ||
| Ordering::Equal => { | ||
| continuation_token.txn_n += 1; | ||
| continuation_token.event_n = 0; | ||
| continue; | ||
|
|
||
| // calculate the remaining capacity based on the chunk size and the current // | ||
| // number of events we have taken. | ||
| let remaining_capacity = chunk_size as usize - all_events.len(); | ||
|
|
||
| // skip events according to the continuation token. | ||
| let filtered_events = filter_events(events.iter(), address, keys.clone()) | ||
| .skip(continuation_token.event_n as usize) | ||
| .take(remaining_capacity) | ||
| .map(|e| EmittedEvent { | ||
| keys: e.keys.clone(), | ||
| data: e.data.clone(), | ||
| transaction_hash: tx_hash, | ||
| block_number: Some(current), | ||
| block_hash: Some(block_hash), | ||
| from_address: e.from_address.into(), | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| // the next time we have to fetch the events, we will start from this index. | ||
| let new_event_n = continuation_token.event_n as usize + filtered_events.len(); | ||
| all_events.extend(filtered_events); | ||
|
|
||
| if all_events.len() >= chunk_size as usize { | ||
| // check if there are still more events that we haven't fetched yet. if yes, | ||
| // we need to return a continuation token that points to the next event to | ||
| // start fetching from. | ||
| if new_event_n < txn_events_len { | ||
| continuation_token.event_n = new_event_n as u64; | ||
| } | ||
| // reset the event index | ||
| else { | ||
| continuation_token.txn_n += 1; | ||
| continuation_token.event_n = 0; | ||
| } | ||
|
|
||
| // if we have reached the last block in the range and there are no more | ||
| // events left AND we still have to fetch from the pending block, we need to | ||
| // increment the block number in the continuation token to point to the | ||
| // pending block. | ||
| if current == to && is_to_pending && new_event_n == txn_events_len { | ||
| continuation_token.txn_n = 0; | ||
| continuation_token.event_n = 0; | ||
| continuation_token.block_n += 1; | ||
| } | ||
|
|
||
| return Ok(EventsPage { | ||
| events: all_events, | ||
| continuation_token: Some(continuation_token.to_string()), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // skip events | ||
| let txn_events = events.iter().skip(continuation_token.event_n as usize); | ||
|
|
||
| let (new_filtered_events, continuation_index) = filter_events_by_params( | ||
| txn_events, | ||
| address, | ||
| keys.clone(), | ||
| Some((chunk_size as usize) - filtered_events.len()), | ||
| ); | ||
|
|
||
| filtered_events.extend(new_filtered_events.iter().map(|e| EmittedEvent { | ||
| from_address: e.from_address.into(), | ||
| keys: e.keys.clone(), | ||
| data: e.data.clone(), | ||
| block_hash: Some(block_hash), | ||
| block_number: Some(i), | ||
| transaction_hash: tx_hash, | ||
| })); | ||
|
|
||
| if filtered_events.len() >= chunk_size as usize { | ||
| let token = if current_block < to | ||
| || continuation_token.txn_n < txn_n as u64 - 1 | ||
| || continuation_index < txn_events_len | ||
| { | ||
| continuation_token.event_n = continuation_index as u64; | ||
| Some(continuation_token.to_string()) | ||
| } else { | ||
| None | ||
| }; | ||
| return Ok(EventsPage { events: filtered_events, continuation_token: token }); | ||
| continuation_token.event_n = 0; | ||
| continuation_token.txn_n += 1; | ||
| } | ||
|
|
||
| continuation_token.txn_n += 1; | ||
| continuation_token.event_n = 0; | ||
| continuation_token.txn_n = 0; | ||
| continuation_token.block_n += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Streamline event fetching logic to reduce complexity.
The block of code from lines 368 to 474, which fetches events across a range of blocks, is quite complex. This complexity can be reduced by breaking down the function into smaller, more manageable parts. For instance, the loop that processes each block and transaction could be refactored into separate functions.
Consider breaking down the event fetching logic into smaller functions to simplify the code and enhance readability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2375 +/- ##
==========================================
+ Coverage 67.86% 68.16% +0.30%
==========================================
Files 359 360 +1
Lines 46959 47138 +179
==========================================
+ Hits 31869 32133 +264
+ Misses 15090 15005 -85 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
Line range hint
336-435: Ohayo sensei! Theeventsfunction is well-structured and handles various scenarios effectively, but consider breaking it down into smaller functions for better readability and maintainability.The
eventsfunction does a great job of handling event retrieval from both regular and pending blocks, using theContinuationTokenfor efficient pagination. The logic for handling different block ranges and calling the appropriate helper functions (fill_events_atandfill_pending_events) is clear and concise.However, the function is quite large and could benefit from being broken down into smaller, more focused functions. This would improve readability and maintainability, making it easier for other developers to understand and modify the code in the future.
Consider extracting the logic for handling different block ranges into separate functions, and possibly moving the
EventBlockIdenum to a separate file.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
Line range hint
336-435: Ohayo sensei! The refactoredeventsfunction looks great, especially the improved handling of pending blocks and continuation tokens.The changes enhance the function's ability to retrieve events from both regular and pending blocks. The use of the
EventBlockIdenum and theContinuationTokenstruct improves code readability and enables efficient event paging.However, the function is still quite complex. Consider further decomposing it into smaller, more focused functions and adding more documentation to explain the logic flow. This will make the code easier to understand and maintain.
kariy
force-pushed
the
katana/pending-events
branch
from
0b9cc87
to
f6e65ef
Compare
kariy
force-pushed
the
katana/pending-events
branch
from
728ddc1
to
44fa590
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (3)
crates/katana/primitives/src/event.rs (3)
14-21: Ohayo, sensei! Great documentation, but there's a tiny typo.The documentation for
ContinuationTokenis clear and informative. It explains the purpose and usage of the struct well.There's a small typo in line 20. Let's fix it:
-/// There JSON-RPC specification does not specify the format of the continuation token, +/// The JSON-RPC specification does not specify the format of the continuation token,
47-68: Sugoi parse method, sensei! Just one tiny suggestion.The updated
parsemethod is a significant improvement:
- Using
&strinstead ofStringis more efficient.- The new parsing logic handles empty parts and invalid formats robustly.
- The
part!macro reduces code duplication effectively.Consider adding a comment explaining the
part!macro for improved readability:// Macro to parse the next part of the token, returning an error if it's missing or empty macro_rules! part { // ... (existing macro implementation) }
Line range hint
79-132: Excellent test coverage, sensei!The updated test cases are comprehensive and cover the new functionality well:
- They now use
&strinstead ofString, matching the updatedparsemethod signature.- The new test cases cover the additional error variants.
Consider adding a test case for the
InvalidTokenerror:#[test] fn parse_invalid_token() { assert_matches!( ContinuationToken::parse("0,0,0,0").unwrap_err(), ContinuationTokenError::InvalidToken ); }This will ensure complete coverage of all error variants.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/katana/primitives/src/event.rs (1)
Line range hint
41-54: Impressive parse method upgrade, sensei!The new implementation is more efficient and provides better error handling. Great job on improving the robustness of the parsing logic!
There's a minor inconsistency in variable naming. Consider updating
receipt_ntotxn_nfor consistency with the struct field name:- let receipt_n = + let txn_n = u64::from_str_radix(arr[1], 16).map_err(ContinuationTokenError::ParseFailed)?; - Ok(ContinuationToken { block_n, txn_n: receipt_n, event_n }) + Ok(ContinuationToken { block_n, txn_n, event_n })
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/katana/primitives/src/event.rs (1)
Line range hint
41-53: Impressive parse method upgrade, sensei!The
parsemethod looks much better now. Accepting&strinstead ofStringis more efficient, and the error handling is top-notch.Consider using
array_chunksinstead ofcollecting into aVecfor even better performance:- let arr: Vec<&str> = token.split(',').collect(); - if arr.len() != 3 { + let mut chunks = token.split(',').array_chunks::<3>(); + if let Some([block, txn, event]) = chunks.next() { + if chunks.next().is_some() { + return Err(ContinuationTokenError::InvalidToken); + } + // Parse block, txn, and event here + } else { return Err(ContinuationTokenError::InvalidToken); } - let block_n = - u64::from_str_radix(arr[0], 16).map_err(ContinuationTokenError::ParseFailed)?; - let receipt_n = - u64::from_str_radix(arr[1], 16).map_err(ContinuationTokenError::ParseFailed)?; - let event_n = - u64::from_str_radix(arr[2], 16).map_err(ContinuationTokenError::ParseFailed)?;This approach avoids allocating a
Vecand provides a more idiomatic way to ensure exactly three elements.
* feat: add sierra to cairo debug information * Add walnut flag to sozo execute command * Pass rpc url to handle_transaction_result * Add walnut flag to sozo migrate apply command * Move walnut_debug_transaction to walnut crate * Cargo fmt * Keep only one global walnut flag * Add comments * Add walnut flag to sozo execute command * Pass rpc url to handle_transaction_result * Add walnut flag to sozo migrate apply command * Move walnut_debug_transaction to walnut crate * Cargo fmt * Keep only one global walnut flag * Add comments * Resolve conflicts * Fix lint errors * Put the walnut crate under the /sozo dir * Add constants with API and app URLs * Warn where we fail silently * Remove unnecessary comments * Check Walnut API key before migration * Add doc comments * Disable walnut flag in auto_authorize * chore; use debug for pending tx log (#2383) Update engine.rs * refactor(katana-rpc): `getEvents` include pending block (#2375) * refactor(katana): move predeployedAccounts under DevApi and remove KatanaApi (#2310) fix: move predeployedAccounts under DevApi and remove KatanaApi * remove world and indexers table in favour of contracts commit-id:98359f5f * opt(torii): batch query execution in sync_range commit-id:72f22f88 * refactor(torii): make select block cancel safe commit-id:8fbc8e6d * opt(torii): use hashmap instead of vector of event processors commit-id:7303cc72 * opt(torii): fetch receipts along with blocks instead of fetching them individually commit-id:b6db4cb5 * opt(torii): avoid re-processing of transactions in certain case fix: #2355 commit-id:a510b985 * wip * chore(dojo-world): enable manifest feature on `migration` feature * fmt * refactor: move walnut config into WalnutDebugger * fix: ensure only WalnutDebugger is exposed * fix: restore default dojo_dev.toml * dont print in library code * use concrete error types in walnut/verification * use concrete types again * remove unecessary util function * use json method instead * fix: fix test --------- Co-authored-by: glihm <dev@glihm.net> Co-authored-by: Larko <59736843+Larkooo@users.noreply.github.com> Co-authored-by: Ammar Arif <evergreenkary@gmail.com> Co-authored-by: lambda-0x <0xlambda@protonmail.com>
resolves #2328
the
starknet_getEventsmethod will now return pending events. though you can explicitly specify the pending block, there is also a case where pending block will be implicitly used, that is when either thefromortois set toNonein the json rpc request params.the events extracting logic could probably be better tho.
Summary by CodeRabbit
New Features
ContinuationTokenfor improved event query paging.utilsmodule for better organization of event handling functions.Bug Fixes
Documentation
Refactor
StarknetApiimplementation.