feat(katana): added chunk size limit to events method #2644
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes to the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/katana/rpc/rpc-types/src/error/starknet.rs (2)
86-87: Ohayo sensei! Minor style improvement needed in error message.The error message capitalization should be consistent with other variants. Consider changing "Chunk Size" to "Chunk size".
- #[error("Chunk Size is too big")] + #[error("Chunk size is too big")]
86-87: Ohayo sensei! Consider standardizing size limit error handling.For consistency in error reporting, consider:
- Adding similar context fields (requested/maximum) to other size-related errors like
PageSizeTooBig- Updating the
data()method to include size information for all size-related errorsThis would provide a consistent error handling pattern across all size limit validations.
Also applies to: 123-123
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/katana/rpc/rpc-types/src/error/starknet.rs(2 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(3 hunks)
🔇 Additional comments (3)
crates/katana/rpc/rpc-types/src/error/starknet.rs (1)
123-123: Ohayo sensei! The error code implementation looks good!
The error code 10001 is appropriately assigned in the custom error range (10000+), maintaining consistency with existing error codes.
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
74-74: LGTM! Field addition looks good.
The max_chunk_size field is appropriately typed and well-placed within the Inner struct.
814-821: LGTM! Validation logic is robust and secure.
The chunk size validation effectively prevents DoS attacks by:
- Checking against the configured maximum
- Providing clear error context with both requested and maximum values
| @@ -103,7 +104,7 @@ impl<EF: ExecutorFactory> StarknetApi<EF> { | |||
| let blocking_task_pool = | |||
| BlockingTaskPool::new().expect("failed to create blocking task pool"); | |||
| let inner = | |||
| Inner { pool, backend, block_producer, blocking_task_pool, validator, forked_client }; | |||
| Inner { pool, backend, block_producer, blocking_task_pool, validator, forked_client, max_chunk_size: 1000 }; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making max_chunk_size configurable.
Ohayo sensei! The hardcoded value of 1000 should be made configurable via CLI as mentioned in issue #2596. This would allow users to adjust the limit based on their specific requirements.
Consider:
- Adding a CLI parameter for max chunk size
- Passing the configured value through the constructor
- Inner { pool, backend, block_producer, blocking_task_pool, validator, forked_client, max_chunk_size: 1000 };
+ Inner { pool, backend, block_producer, blocking_task_pool, validator, forked_client, max_chunk_size: config.max_chunk_size };Committable suggestion skipped: line range outside the PR's diff.
|
@kariy Can you please take a look at this? Thanks! |
| @@ -71,6 +71,7 @@ struct Inner<EF: ExecutorFactory> { | |||
| block_producer: BlockProducer<EF>, | |||
| blocking_task_pool: BlockingTaskPool, | |||
| forked_client: Option<ForkedClient>, | |||
| max_chunk_size: u64, | |||
There was a problem hiding this comment.
lets make this field into an Option instead. also maybe lets create a config struct, StarknetApiConfig, to encapsulate this value.
There was a problem hiding this comment.
we should also update all the StarknetApi::new functions to accept the new config struct
There was a problem hiding this comment.
make sure to update the RpcConfig with the max_events_page_size option
dojo/crates/katana/node/src/config/rpc.rs
Lines 20 to 28 in 0c70748
i think 'page size' is a much better name in this case. maybe we should just call it page size anywhere else and just leave the chunk size name in the server impl.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
820-827: LGTM! Consider extracting validation to a method.Ohayo sensei! The validation logic looks good and effectively prevents DoS attacks. However, consider extracting it to a separate method for better readability:
impl<EF: ExecutorFactory> StarknetApi<EF> { + fn validate_chunk_size(&self, chunk_size: u64) -> StarknetApiResult<()> { + if chunk_size > self.inner.max_chunk_size { + return Err(StarknetApiError::ChunkSizeTooBig { + requested: chunk_size, + maximum: self.inner.max_chunk_size, + }); + } + Ok(()) + } async fn events(&self, filter: EventFilterWithPage) -> StarknetApiResult<EventsPage> { let EventFilterWithPage { event_filter, result_page_request } = filter; let ResultPageRequest { continuation_token, chunk_size } = result_page_request; - let max_chunk_size = self.inner.max_chunk_size; - if chunk_size > max_chunk_size { - return Err(StarknetApiError::ChunkSizeTooBig { - requested: chunk_size, - maximum: max_chunk_size, - }); - } + self.validate_chunk_size(chunk_size)?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/katana/rpc/rpc/src/starknet/mod.rs(3 hunks)
🔇 Additional comments (2)
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
74-74: Follow up on previous review suggestions.
Ohayo sensei! As suggested in the previous review:
- Consider making this field an
Option<u64> - Move this configuration to a dedicated
StarknetApiConfigstruct - Rename to
max_events_page_sizefor better clarity
107-107: Make max_chunk_size configurable via CLI.
Ohayo sensei! The hardcoded value should be configurable as mentioned in issue #2596.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
crates/katana/node/src/config/rpc.rs (1)
8-8: Ohayo! Add documentation for the page size constant.The default value of 100 seems reasonable, but it would be helpful to document why this specific value was chosen and what factors were considered (e.g., memory usage, typical query patterns).
+/// Default maximum number of events that can be returned in a single page. +/// This limit helps prevent potential DoS attacks through excessive resource consumption. pub const DEFAULT_RPC_PAGE_SIZE: u64 = 100;bin/katana/src/cli/options.rs (1)
86-90: Enhance documentation for security context, sensei!While the field is properly configured, consider adding documentation that explains its security implications:
- /// Maximum page size for event queries. + /// Maximum page size for event queries to prevent potential DoS attacks. + /// Limits the number of events that can be returned in a single request.crates/katana/node/src/lib.rs (2)
281-283: Consider extracting the default page size as a constantThe default value of 1000 should be defined as a named constant at the module level or in a constants module for better maintainability and documentation.
+ const DEFAULT_PAGE_SIZE: u64 = 1000; let starknet_api_config = StarknetApiConfig { - page_size: config.page_size.unwrap_or(1000), // Default value + page_size: config.page_size.unwrap_or(DEFAULT_PAGE_SIZE), };
294-294: Fix formatting: Add space after commaThere should be a space after the comma before
starknet_api_config:- StarknetApi::new(backend.clone(), pool.clone(), block_producer.clone(), validator,starknet_api_config) + StarknetApi::new(backend.clone(), pool.clone(), block_producer.clone(), validator, starknet_api_config)crates/katana/rpc/rpc/src/starknet/mod.rs (2)
62-64: Add documentation for the StarknetApiConfig struct.Ohayo sensei! Consider adding documentation to explain the purpose of this struct and its field:
+/// Configuration for the Starknet API. pub struct StarknetApiConfig { + /// Maximum number of events that can be returned in a single page. pub page_size: u64, }
836-836: Remove extra whitespace.There's an extra blank line that should be removed to maintain consistent formatting.
- self.on_io_blocking_task(move |this| {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
bin/katana/src/cli/node.rs(1 hunks)bin/katana/src/cli/options.rs(4 hunks)crates/katana/node/src/config/rpc.rs(3 hunks)crates/katana/node/src/lib.rs(2 hunks)crates/katana/rpc/rpc-types/src/error/starknet.rs(5 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/rpc/rpc-types/src/error/starknet.rs
🔇 Additional comments (9)
crates/katana/node/src/config/rpc.rs (1)
27-27: Document the page_size field and verify its usage.
The field implementation looks good, but would benefit from documentation explaining its purpose and relationship to event queries.
✅ Verification successful
Ohayo, sensei! Document the page_size field and verify its usage.
The field implementation is correct and its usage across the codebase is consistent. Adding documentation will clarify its purpose and relationship to event queries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify how page_size is used across the codebase
# Check for page_size usage in event-related code
rg -A 5 "page_size" --type rust
# Look for event-related structs and methods
ast-grep --pattern 'fn $name($$$) {
$$$
page_size
$$$
}'
Length of output: 7385
bin/katana/src/cli/options.rs (3)
15-15: Ohayo! Import looks good, sensei!
The DEFAULT_RPC_PAGE_SIZE constant is properly imported alongside other RPC-related constants.
101-101: Default implementation is perfect, sensei!
The page_size field is properly initialized with DEFAULT_RPC_PAGE_SIZE in the Default implementation.
355-357: Helper function looks good! Let's verify the constant value, sensei!
The default_page_size function follows the established pattern for serde defaults.
✅ Verification successful
Ohayo, sensei!
The initial search didn't locate DEFAULT_RPC_PAGE_SIZE. Let's broaden our search to find where it's defined.
Ohayo, sensei!
Verified that DEFAULT_RPC_PAGE_SIZE is set to 100, which is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the DEFAULT_RPC_PAGE_SIZE constant value is reasonable
# Search for the constant definition
rg -p "DEFAULT_RPC_PAGE_SIZE\s*=\s*\d+" crates/katana/node/src/config/rpc.rs
Length of output: 78
Script:
#!/bin/bash
# Description: Search the entire codebase for the DEFAULT_RPC_PAGE_SIZE constant definition
rg "const\s+DEFAULT_RPC_PAGE_SIZE\s*[:=]" --glob "*.rs"
Length of output: 138
crates/katana/node/src/lib.rs (2)
52-52: Ohayo sensei! LGTM - Import statement is well-placed
The import statement for StarknetApiConfig is correctly positioned with other starknet-related imports.
285-294: Verify consistent configuration usage
The changes look good, but let's verify that the page_size configuration is consistently used across the codebase.
✅ Verification successful
Consistent page_size configuration usage verified
Ohayo, sensei. The page_size configuration is consistently used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of page_size configuration
# Check for StarknetApiConfig usage
rg -A 2 "StarknetApiConfig"
# Check for page_size references
rg "page_size"
# Look for potential event-related pagination
ast-grep --pattern 'fn $event_method($$$) {
$$$
chunk_size
$$$
}'
Length of output: 3648
bin/katana/src/cli/node.rs (1)
225-225: Verify the default page size value.
Let's ensure that a reasonable default value is set for the page size parameter.
✅ Verification successful
Ohayo sensei! Default page size is set appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DEFAULT_RPC_PAGE_SIZE or similar constants
rg -A 1 "DEFAULT.*PAGE_SIZE|DEFAULT.*CHUNK_SIZE"
# Search for page_size field definition in ServerOptions
ast-grep --pattern 'struct ServerOptions {
$$$
page_size: $_
$$$
}'
Length of output: 2019
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
79-79: LGTM! Constructor changes are well-implemented.
The changes consistently integrate the new configuration across all constructors, and the use of Option for max_chunk_size aligns with the review feedback.
Also applies to: 88-88, 99-99, 110-110, 115-115
828-835: LGTM! Chunk size validation is well-implemented.
The validation logic is correct and provides good error context by including both the requested and maximum allowed values. The check is performed early in the method, preventing unnecessary work for invalid requests.
Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
Co-authored-by: Ammar Arif <evergreenkary@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
828-835: Consider simplifying the config access pattern.Ohayo sensei! While the validation logic is correct, we can make it more idiomatic by using
as_ref().map_or(true, |config| ...)pattern.Consider this more concise approach:
- if let Some(config) = self.inner.max_chunk_size.as_ref() { - if chunk_size > config.page_size { - return Err(StarknetApiError::PageSizeTooBig { - requested: chunk_size, - max_allowed: config.page_size, - }); - } - } + if self.inner.config.max_event_page_size.as_ref().map_or(false, |&max_size| chunk_size > max_size) { + return Err(StarknetApiError::PageSizeTooBig { + requested: chunk_size, + max_allowed: self.inner.config.max_event_page_size.unwrap(), + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
bin/katana/src/cli/node.rs(1 hunks)bin/katana/src/cli/options.rs(4 hunks)crates/katana/node/src/config/rpc.rs(3 hunks)crates/katana/rpc/rpc-types/src/error/starknet.rs(5 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bin/katana/src/cli/node.rs
- bin/katana/src/cli/options.rs
- crates/katana/node/src/config/rpc.rs
- crates/katana/rpc/rpc-types/src/error/starknet.rs
🔇 Additional comments (2)
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
62-64: LGTM! Clean struct definition.
The StarknetApiConfig struct with optional max_event_page_size field follows the recommended design pattern and naming convention.
79-79: LGTM! Config field added to Inner struct.
The integration of StarknetApiConfig into the Inner struct follows the suggested design pattern.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@kariy can you PTAL, it is not allowing me to resolve the merge conflicts for some reason, i have made the suggested changes |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/katana/rpc/rpc/src/starknet/mod.rs(7 hunks)
🔇 Additional comments (3)
crates/katana/rpc/rpc/src/starknet/mod.rs (3)
62-64: LGTM! Well-structured configuration struct.
Ohayo sensei! The StarknetApiConfig struct is well-designed with an optional max_event_page_size field, allowing for flexible configuration of page size limits.
79-79: LGTM! Clean integration of config field.
Ohayo sensei! The config field is properly integrated into the Inner struct.
88-88: LGTM! Consistent constructor updates.
Ohayo sensei! The constructors are consistently updated to accept and propagate the configuration parameter.
Also applies to: 99-99, 110-110, 115-115
| if let Some(config) = self.inner.max_chunk_size.as_ref() { | ||
| if chunk_size > config.page_size { | ||
| return Err(StarknetApiError::PageSizeTooBig { | ||
| requested: chunk_size, | ||
| max_allowed: config.page_size, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect config field reference and error construction.
Ohayo sensei! There are two issues in the validation code:
- The code references
self.inner.max_chunk_sizeinstead ofself.inner.config.max_event_page_size - The error construction uses incorrect field names
Apply this fix:
- if let Some(config) = self.inner.max_chunk_size.as_ref() {
- if chunk_size > config.page_size {
+ if let Some(max_size) = self.inner.config.max_event_page_size {
+ if chunk_size > max_size {
return Err(StarknetApiError::PageSizeTooBig {
requested: chunk_size,
- max_allowed: config.page_size,
+ max_allowed: max_size,
});
}
}📝 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.
| if let Some(config) = self.inner.max_chunk_size.as_ref() { | |
| if chunk_size > config.page_size { | |
| return Err(StarknetApiError::PageSizeTooBig { | |
| requested: chunk_size, | |
| max_allowed: config.page_size, | |
| }); | |
| } | |
| } | |
| if let Some(max_size) = self.inner.config.max_event_page_size { | |
| if chunk_size > max_size { | |
| return Err(StarknetApiError::PageSizeTooBig { | |
| requested: chunk_size, | |
| max_allowed: max_size, | |
| }); | |
| } | |
| } |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/cli/src/options.rs (1)
95-100: Enhance documentation for security implications.Ohayo, sensei! While the implementation looks good, consider enhancing the documentation to explain the security implications:
- /// Maximum page size for event queries. + /// Maximum page size for event queries to prevent potential DoS attacks. + /// Limits the number of events that can be retrieved in a single request.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/katana/cli/src/options.rs(4 hunks)crates/katana/node/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/node/src/lib.rs
🔇 Additional comments (4)
crates/katana/cli/src/options.rs (4)
16-19: LGTM! Import changes are well-organized.
The new constant DEFAULT_RPC_MAX_EVENT_PAGE_SIZE is properly imported alongside other RPC-related constants.
111-111: LGTM! Default implementation is consistent.
The new field is properly initialized with DEFAULT_RPC_MAX_EVENT_PAGE_SIZE in the Default implementation.
369-371: LGTM! Default function is well-implemented.
The default_page_size function follows the established pattern and correctly provides the default value for serde deserialization.
95-100: Verify the limit is enforced in the RPC handler.
Ohayo again, sensei! Let's verify that this limit is properly enforced in the RPC handler:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2644 +/- ##
==========================================
+ Coverage 56.34% 56.37% +0.02%
==========================================
Files 411 411
Lines 52742 52770 +28
==========================================
+ Hits 29719 29749 +30
+ Misses 23023 23021 -2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Description
This PR adds a Chunk Size Limit to
starknet_getEventsmethodRelated issue
Fixes #2596
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Release Notes
New Features
NodeArgsstruct to allow configuration of maximum event page size through command line arguments.Bug Fixes
Documentation
Tests