-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(katana): forked events #2594
Conversation
8a9a95a
to
200c95c
Compare
WalkthroughOhayo, sensei! This pull request introduces significant enhancements to the handling of continuation tokens and event retrieval in the Katana framework. A new enum, Changes
Possibly related PRs
Suggested reviewers
🪧 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
crates/katana/rpc/rpc/src/starknet/forking.rs (1)
247-249
: Consider creating a follow-up issue for BlockId validation standardization.Your note about parameter validation is insightful. Creating explicit parameters instead of using
EventFilter
directly does enforce better validation. Consider creating a follow-up issue to track the standardization of this approach across other methods that acceptBlockId
.Would you like me to help create a GitHub issue to track this improvement?
crates/katana/rpc/rpc/tests/forking.rs (1)
391-470
: Consider extracting magic numbers into named constants, sensei!The tests are well-structured, but could be improved by:
- Extracting the chunk sizes (5, 100) into named constants like
PARTIAL_CHUNK_SIZE
andFULL_CHUNK_SIZE
- Moving the comment about "89 events" into a constant like
FORK_BLOCK_EVENT_COUNT
This would make the tests more maintainable and self-documenting.
+ const PARTIAL_CHUNK_SIZE: u64 = 5; + const FULL_CHUNK_SIZE: u64 = 100; + const FORK_BLOCK_EVENT_COUNT: usize = 89; // Number of events in FORK_BLOCK_NUMBER #[tokio::test] async fn get_events_partially_from_forked() -> Result<()> { // ... - let result = provider.get_events(filter, None, 5).await?; + let result = provider.get_events(filter, None, PARTIAL_CHUNK_SIZE).await?;crates/katana/rpc/rpc/src/starknet/mod.rs (1)
843-1059
: Ohayo, sensei! Adding documentation toevents_inner
would enhance clarity.The
events_inner
function is central to event fetching but lacks descriptive comments or documentation.Consider adding documentation comments explaining:
- The purpose of the function.
- The parameters and their expected values.
- An overview of how the function handles different cases.
This will aid other developers in understanding and maintaining the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (6)
crates/katana/primitives/src/event.rs
(2 hunks)crates/katana/rpc/rpc/Cargo.toml
(1 hunks)crates/katana/rpc/rpc/src/starknet/forking.rs
(9 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs
(3 hunks)crates/katana/rpc/rpc/src/starknet/read.rs
(2 hunks)crates/katana/rpc/rpc/tests/forking.rs
(5 hunks)
🔇 Additional comments (13)
crates/katana/rpc/rpc/Cargo.toml (1)
48-48
: Ohayo! The addition of similar-asserts looks good, sensei!
The new dev dependency follows the workspace inheritance pattern and will enhance the testing capabilities for the forked events functionality.
crates/katana/rpc/rpc/src/starknet/read.rs (2)
Line range hint 1-21
: Ohayo sensei! Clean import organization.
The removal of BlockTag
from imports while keeping other necessary types shows good hygiene in dependency management.
122-124
: Elegant simplification of event handling, sensei!
The refactoring of get_events
to directly use self.events(filter)
demonstrates:
- Improved separation of concerns
- Better code maintainability
- Reduced complexity
This aligns well with the PR's objective of enhancing forked network event handling.
Let's verify the implementation of the delegated events
method:
✅ Verification successful
The previous pattern might have been too restrictive. Let's try with a more flexible pattern and also use ripgrep as a fallback to ensure we don't miss any implementations.
Ohayo! Your elegant simplification is indeed verified, sensei!
The implementation of events
method in crates/katana/rpc/rpc/src/starknet/mod.rs
shows a robust and well-structured implementation that:
- Properly handles event filtering with pagination
- Processes block ranges through
from_block
andto_block
- Manages continuation tokens for paginated results
- Delegates to
events_inner
for the core processing logic
The refactoring to directly use self.events(filter)
is a clean improvement that maintains all the necessary functionality while reducing code duplication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the events method implementation
ast-grep --pattern 'fn events($_, $_) {
$$$
}'
Length of output: 50
Script:
#!/bin/bash
# Let's try a different pattern to find the events method implementation
ast-grep --pattern 'async fn events($$$) {
$$$
}'
# Also search with ripgrep for any events method
rg "fn events" -A 10
Length of output: 6762
crates/katana/rpc/rpc/src/starknet/forking.rs (2)
27-31
: Ohayo! Nice error handling improvements, sensei!
The new error variants BlockTagNotAllowed
and UnexpectedPendingData
are well-defined and provide clear error messages. This is a good improvement over panic-based handling.
275-277
: Clean error mapping implementation!
The mapping of new error variants to StarknetApiError::UnexpectedError
with descriptive reasons maintains good error context for API consumers.
crates/katana/rpc/rpc/tests/forking.rs (3)
7-7
: LGTM! Clean import additions.
The new imports are well-organized and necessary for the event handling functionality.
Also applies to: 9-9, 13-13
86-86
: Ohayo! Great function renaming, sensei!
The new function names are more descriptive and better reflect their purpose. They follow a consistent get_*
pattern which improves code readability.
Also applies to: 206-206, 326-326
473-551
: Excellent test coverage, sensei!
The tests thoroughly verify:
- Local event fetching
- Boundary conditions between forked and local events
- Event field comparisons
- Edge cases
Particularly impressive is the boundary test that verifies the correct handling of events across the fork boundary.
crates/katana/primitives/src/event.rs (1)
64-64
: Ohayo, sensei! Good addition of Clone
trait.
Deriving the Clone
trait for ContinuationToken
enhances its usability in contexts requiring cloning. This change aligns with Rust best practices.
crates/katana/rpc/rpc/src/starknet/mod.rs (4)
25-25
: Importing MaybeForkedContinuationToken
is necessary and appropriate.
The addition of use katana_primitives::event::MaybeForkedContinuationToken;
is required for handling continuation tokens in the new event fetching logic.
40-40
: Including EventFilterWithPage
and EventsPage
imports is correct.
These imports are essential for the implementation of the new events
method and related functionality.
48-48
: Updating imports with pagination and status types.
Adding ResultPageRequest
, TransactionExecutionStatus
, and TransactionStatus
to the imports ensures that the code has access to the necessary types for handling pagination and transaction statuses.
823-823
: Ensure graceful handling of continuation token parsing errors.
Using the ?
operator on MaybeForkedContinuationToken::parse(&token)?
will propagate any parsing errors. Confirm that this behavior is intentional and provides meaningful error messages to the caller.
Ohayo, sensei! Please verify that the error handling here meets our standards for clarity and user feedback. If enhanced error messages are needed, consider wrapping the error with a descriptive message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
crates/katana/primitives/src/event.rs (2)
64-78
: Ohayo, sensei! The enum design is elegant!The
MaybeForkedContinuationToken
enum is well-structured and documented. The use of String for forked tokens provides good flexibility for handling different provider formats.Consider adding an example in the documentation to show both variants in use:
/// Represents a continuation token that can either be a Katana native [`ContinuationToken`] or a /// continuation token returned by the forked provider. /// /// This is only used in the `starknet_getEvents` API. +/// +/// # Examples +/// ``` +/// use katana_primitives::event::{ContinuationToken, MaybeForkedContinuationToken}; +/// +/// // Native token +/// let native = MaybeForkedContinuationToken::Token(ContinuationToken::default()); +/// +/// // Forked token +/// let forked = MaybeForkedContinuationToken::Forked("provider_specific_token".to_string()); +/// ```
170-185
: Ohayo, sensei! Let's enhance the test coverage!While the current tests cover the basic scenarios, consider adding these edge cases:
- Empty forked token (e.g., "FK_")
- Invalid regular token with FK_ prefix (e.g., "FK_1,2")
- Unicode characters in forked token
Here's a suggested addition:
#[test] fn parse_forked_token_edge_cases() { // Empty forked token let parsed = MaybeForkedContinuationToken::parse("FK_").unwrap(); assert_matches!(parsed, MaybeForkedContinuationToken::Forked(s) => { assert_eq!(s, "") }); // Invalid regular token with FK_ prefix let parsed = MaybeForkedContinuationToken::parse("FK_1,2"); assert_matches!(parsed, Ok(MaybeForkedContinuationToken::Forked(_))); // Unicode characters let parsed = MaybeForkedContinuationToken::parse("FK_🦀").unwrap(); assert_matches!(parsed, MaybeForkedContinuationToken::Forked(s) => { assert_eq!(s, "🦀") }); }crates/katana/rpc/rpc/src/starknet/forking.rs (1)
247-249
: Interesting design note, sensei!The note raises a valid point about parameter validation consistency. Consider creating a tracking issue to standardize parameter validation across methods that accept BlockId.
Would you like me to help create a tracking issue for standardizing parameter validation?
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
842-851
: Add comprehensive documentation for the events_inner method.Ohayo! The TODO comment indicates a need for documentation. This complex method would benefit from detailed documentation explaining:
- Purpose and responsibilities
- Parameter descriptions
- Return value details
- Different block range cases handled
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/katana/primitives/src/event.rs
(3 hunks)crates/katana/rpc/rpc/src/starknet/forking.rs
(9 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs
(3 hunks)
🔇 Additional comments (6)
crates/katana/primitives/src/event.rs (1)
23-23
: Ohayo, sensei! The Clone derive addition looks good!
Adding the Clone trait is appropriate here as it allows the token to be duplicated when needed, which is particularly useful in async contexts or when working with the new forked events feature.
crates/katana/rpc/rpc/src/starknet/forking.rs (3)
27-31
: Ohayo! Nice error handling improvements, sensei!
The new error variants BlockTagNotAllowed
and UnexpectedPendingData
enhance the robustness of the codebase by replacing panics with proper error handling. The error messages are clear and descriptive.
120-120
: Consistent error handling across block-related methods!
The error handling has been standardized across all block-related methods, properly handling pending blocks and block tags. This consistency makes the code more maintainable and predictable.
Also applies to: 151-151, 169-169, 189-189, 211-211, 238-238
279-281
: Clean error mapping implementation!
The error mapping to StarknetApiError
is well-implemented, providing meaningful error messages while maintaining compatibility with the Starknet API error system.
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
821-821
:
Fix the logical condition in keys filtering.
Ohayo, sensei! The condition !(keys.len() == 1 && keys.is_empty())
is logically incorrect as a vector cannot be both of length 1 and empty simultaneously.
-let keys = event_filter.keys.filter(|keys| !(keys.len() == 1 && keys.is_empty()));
+let keys = event_filter.keys.filter(|keys| !keys.is_empty());
Likely invalid or redundant comment.
1063-1067
:
Use a more specific error type for invalid block ranges.
Ohayo, sensei! Using UnexpectedError
for an invalid block range doesn't accurately represent the error condition. Consider adding a dedicated error variant:
-(EventBlockId::Pending, EventBlockId::Num(_)) => {
- Err(StarknetApiError::UnexpectedError {
- reason: "Invalid block range; `from` block must be lower than `to`".to_string(),
- })
-}
+(EventBlockId::Pending, EventBlockId::Num(_)) => {
+ Err(StarknetApiError::InvalidBlockRange {
+ reason: "from block must be lower than to block".to_string(),
+ })
+}
Likely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2594 +/- ##
==========================================
+ Coverage 70.31% 70.41% +0.09%
==========================================
Files 402 402
Lines 51214 51364 +150
==========================================
+ Hits 36012 36166 +154
+ Misses 15202 15198 -4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
crates/katana/rpc/rpc/src/starknet/forking.rs (1)
302-334
: Consider adding tests for the get_events functionality!While the
get_block_hash
tests are well-implemented, it would be beneficial to add tests for the newget_events
functionality to ensure it handles various scenarios correctly:
- Valid block range
- Invalid block range
- Different continuation token scenarios
- Various chunk sizes
Would you like me to help generate the test cases for the
get_events
functionality?crates/katana/rpc/rpc/src/starknet/mod.rs (1)
886-887
: Address the TODO comments about token handling simplification.The token handling logic in both match arms could be simplified by extracting the common functionality into a helper method.
Would you like assistance in implementing the simplified token handling logic?
Also applies to: 960-961
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/katana/rpc/rpc/src/starknet/forking.rs
(10 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs
(3 hunks)crates/katana/rpc/rpc/tests/forking.rs
(6 hunks)
🔇 Additional comments (19)
crates/katana/rpc/rpc/src/starknet/forking.rs (5)
27-31
: LGTM! Well-structured error variants, sensei!
The new error variants BlockTagNotAllowed
and UnexpectedPendingData
are well-defined with clear error messages, improving the error handling capabilities.
62-76
: Ohayo! Clean and robust implementation!
The get_block_number_by_hash
implementation includes proper validation for:
- Pending blocks (returns
UnexpectedPendingData
) - Block range (returns
BlockOutOfRange
) - Proper error propagation
136-136
: Excellent error handling improvements, sensei!
The updates consistently replace panic scenarios with proper error handling using the new error variants across all methods. This makes the codebase more robust and predictable.
Also applies to: 147-147, 167-167, 185-185, 205-205, 227-227, 254-254
266-287
: Consider adding chunk_size validation, sensei!
The get_events
implementation looks solid with proper block range validation. However, as mentioned in previous reviews, consider adding validation for chunk_size
to prevent potential issues with extremely large or zero values.
295-297
: Clean error mapping implementation!
The error mapping for the new variants is appropriate, using UnexpectedError
with descriptive messages.
crates/katana/rpc/rpc/src/starknet/mod.rs (3)
806-840
: LGTM! Clean implementation of the events method.
The async implementation properly handles parameter extraction and provides sensible defaults for block ranges.
1067-1100
: LGTM! Well-structured block resolution logic.
The method effectively handles different block ID types and properly integrates with the forked client. The error handling and documentation are clear.
821-821
:
Fix the logical condition in keys filtering.
The condition !(keys.len() == 1 && keys.is_empty())
is logically incorrect as a list cannot be both of length 1 and empty simultaneously.
-let keys = event_filter.keys.filter(|keys| !(keys.len() == 1 && keys.is_empty()));
+let keys = event_filter.keys.filter(|keys| !keys.is_empty());
Likely invalid or redundant comment.
crates/katana/rpc/rpc/tests/forking.rs (11)
7-9
: Ohayo, sensei! The new imports are appropriately added.
The additions of BlockTag
and MaybeForkedContinuationToken
are necessary for the enhanced event handling and block identification.
13-13
: Ohayo, sensei! Importing EventFilter
and StarknetError
is suitable.
These imports are essential for the new test functions dealing with events and error handling.
23-24
: Ohayo, sensei! Defining FORK_BLOCK_HASH
enhances clarity.
By explicitly defining the forked block hash, the tests become more readable and maintainable.
Line range hint 88-207
: Ohayo, sensei! The get_blocks_from_num
test function is comprehensive.
The function effectively tests block retrieval by number for both forked and local blocks, ensuring proper functionality.
Line range hint 208-327
: Ohayo, sensei! The get_blocks_from_hash
test function covers key scenarios.
Retrieving blocks by hash is crucial, and this test ensures both forked and local blocks are handled correctly.
Line range hint 328-390
: Ohayo, sensei! The get_transactions
test function is robust.
The test accurately verifies transaction retrieval before the forked block and within locally generated blocks.
392-435
: Ohayo, sensei! The get_events_partially_from_forked
test is well-implemented.
It effectively validates event fetching and continuation token handling from the forked chain.
437-480
: Ohayo, sensei! The get_events_all_from_forked
test ensures comprehensive event retrieval.
Fetching all events from the forked block confirms the system's ability to handle complete datasets.
482-510
: Ohayo, sensei! The get_events_local
test correctly validates local event retrieval.
Verifying events from the local chain ensures the local transactions are appropriately processed.
512-564
: Ohayo, sensei! The get_events_forked_and_local_boundary
test covers critical boundary conditions.
Testing across forked and local chain boundaries is essential for ensuring seamless event handling.
566-584
: Ohayo, sensei! The get_events_with_invalid_block_hash
test properly checks error handling.
Validating responses to invalid block hashes enhances the robustness of the system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/katana/rpc/rpc/tests/forking.rs (3)
Line range hint
40-84
: Refactorsetup_test_inner
to reduce code duplicationOhayo, sensei! I noticed that both branches of the
if no_mining
condition insetup_test_inner
share similar logic for transaction processing. Consider refactoring the common code into a single loop to enhance maintainability and readability.Apply this diff to consolidate the code:
async fn setup_test_inner(no_mining: bool) -> (TestSequencer, impl Provider, LocalTestVector) { // ... existing code ... // Create some empty blocks and dummy transactions abigen_legacy!(FeeToken, "crates/katana/rpc/rpc/tests/test_data/erc20.json"); let contract = FeeToken::new(DEFAULT_ETH_FEE_TOKEN_ADDRESS.into(), sequencer.account()); - if no_mining { - // In no mining mode, because we're not producing any blocks, the transactions that we send - // will all be included in the same block (pending). - for _ in 1..=10 { - let amount = Uint256 { low: Felt::ONE, high: Felt::ZERO }; - let res = contract.transfer(&Felt::ONE, &amount).send().await.unwrap(); - dojo_utils::TransactionWaiter::new(res.transaction_hash, &provider).await.unwrap(); - - // Events in pending block don't have block hash and number, so we can safely put - // dummy values here. - txs_vector.push(((0, Felt::ZERO), res.transaction_hash)); - } - } else { - // We're in auto mining; each transaction will create a new block - for i in 1..=10 { - let amount = Uint256 { low: Felt::ONE, high: Felt::ZERO }; - let res = contract.transfer(&Felt::ONE, &amount).send().await.unwrap(); - dojo_utils::TransactionWaiter::new(res.transaction_hash, &provider).await.unwrap(); - - let block_num = FORK_BLOCK_NUMBER + i; - - let block_id = BlockIdOrTag::Number(block_num); - let block = provider.get_block_with_tx_hashes(block_id).await.unwrap(); - let block_hash = match block { - MaybePendingBlockWithTxHashes::Block(b) => b.block_hash, - _ => panic!("Expected a block"), - }; - - txs_vector.push(((block_num, block_hash), res.transaction_hash)); - } - } + for i in 1..=10 { + let amount = Uint256 { low: Felt::ONE, high: Felt::ZERO }; + let res = contract.transfer(&Felt::ONE, &amount).send().await.unwrap(); + dojo_utils::TransactionWaiter::new(res.transaction_hash, &provider).await.expect("Transaction waiting failed"); + + if no_mining { + // Events in pending block don't have block hash and number. + txs_vector.push(((0, Felt::ZERO), res.transaction_hash)); + } else { + // We're in auto mining; each transaction creates a new block + let block_num = FORK_BLOCK_NUMBER + i; + let block_id = BlockIdOrTag::Number(block_num); + let block = provider.get_block_with_tx_hashes(block_id).await.expect("Failed to get block with transaction hashes"); + let block_hash = match block { + MaybePendingBlockWithTxHashes::Block(b) => b.block_hash, + _ => panic!("Expected a block"), + }; + txs_vector.push(((block_num, block_hash), res.transaction_hash)); + } + } (sequencer, provider, txs_vector) }
71-77
: Useexpect
for clearer error messagesOhayo, sensei! Using
expect
instead ofunwrap
can provide better error context in your tests. This enhances debugging when a test fails.Apply these diffs to improve error handling:
dojo_utils::TransactionWaiter::new(res.transaction_hash, &provider).await.unwrap(); -// Change to: +// Change to: +dojo_utils::TransactionWaiter::new(res.transaction_hash, &provider).await.expect("Transaction waiting failed"); let block = provider.get_block_with_tx_hashes(block_id).await.unwrap(); -// Change to: +// Change to: +let block = provider.get_block_with_tx_hashes(block_id).await.expect("Failed to get block with transaction hashes");
553-554
: Minor typo in commentOhayo, sensei! There's a small typo in the comment on line 554.
Apply this diff to correct it:
// This is expected behaviour, as the pending block is not yet closed. -// so there may still more events to come. +// so there may still be more events to come.
ref #2592
this is the last method to allow supporting fetching all types of data from the forked network.
Summary by CodeRabbit
Release Notes
New Features
MaybeForkedContinuationToken
for enhanced handling of continuation tokens.get_events
method for retrieving events based on specified parameters.events
method inStarknetApi
for improved event fetching capabilities.Bug Fixes
ForkedClient
with new error variants to prevent panics.Refactor
get_events
method inStarknetApiServer
for better clarity and reduced complexity.