fix: cli args for torii and katana with serde default #2692
Conversation
WalkthroughOhayo, sensei! This pull request introduces several modifications to the command-line options and configuration structures in the Katana and Torii crates. A new 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
crates/torii/cli/src/args.rs (1)
235-236: Consider adding more edge cases to the test, sensei!While the current test validates basic CLI precedence, consider adding test cases for:
- Partial indexing configurations
- Missing indexing section in config
- Multiple indexing options combined
Example test case to add:
# Test partial configuration let partial_content = r#" [indexing] transactions = true # other fields intentionally omitted "#;Also applies to: 249-249
crates/katana/cli/src/options.rs (1)
Line range hint
28-53: Ohayo sensei! The MetricsOptions struct looks solid!The implementation is well-structured with proper feature flags, documentation, and default values. The
requires = "metrics"constraint ensures logical parameter dependencies.One minor suggestion: Consider adding a comment explaining that metrics collection still occurs even when the server is disabled, as this is an important implementation detail.
/// Enable metrics. /// /// For now, metrics will still be collected even if this flag is not set. This only - /// controls whether the metrics server is started or not. + /// controls whether the metrics server is started or not. This behavior allows for + /// metrics collection in all environments while providing flexibility in how they + /// are exposed. #[arg(long)]crates/torii/cli/src/options.rs (2)
173-209: Consider optimizing the merge implementation, sensei!While the merge implementation is functionally correct, we could make it more maintainable:
- The repeated pattern of checking defaults could be abstracted into a helper function
- The cloning of vectors could be optimized for large collections
Consider this refactoring:
impl IndexingOptions { pub fn merge(&mut self, other: Option<&Self>) { if let Some(other) = other { - if self.events_chunk_size == DEFAULT_EVENTS_CHUNK_SIZE { - self.events_chunk_size = other.events_chunk_size; - } - // ... similar patterns for other fields + fn merge_if_default<T: PartialEq + Clone>(current: &mut T, other: &T, default: T) { + if *current == default { + *current = other.clone(); + } + } + + merge_if_default(&mut self.events_chunk_size, &other.events_chunk_size, DEFAULT_EVENTS_CHUNK_SIZE); + merge_if_default(&mut self.blocks_chunk_size, &other.blocks_chunk_size, DEFAULT_BLOCKS_CHUNK_SIZE); + // ... similar pattern for other fields // Special handling for Vec types to avoid unnecessary cloning + if self.contracts.is_empty() { + self.contracts = other.contracts.iter().cloned().collect(); + } } } }
Line range hint
1-209: Consider adding documentation about default behavior, sensei!While the implementation is solid, it would be helpful to add documentation that explains:
- Which fields can be omitted from configuration files
- What default values will be used when fields are omitted
- How the merge behavior works for partial configurations
This documentation would help users understand how to effectively use partial configurations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/katana/cli/src/options.rs(5 hunks)crates/torii/cli/src/args.rs(4 hunks)crates/torii/cli/src/options.rs(10 hunks)
🔇 Additional comments (10)
crates/torii/cli/src/args.rs (2)
207-210: Ohayo! Good test configuration setup
The explicit default value for transactions in the test configuration provides a clear baseline for testing the merge behavior.
111-111: Ohayo sensei! Verify the merge implementation for indexing options
The new merge pattern looks good and aligns with the PR objective of supporting partial configurations. However, we should verify that the merge method properly handles all indexing options.
✅ Verification successful
Ohayo sensei! The merge implementation looks solid and well-implemented
The merge implementation in crates/torii/cli/src/options.rs properly handles all indexing options with appropriate default value checks. It follows a consistent pattern where each field is only overwritten if it has the default value, which aligns perfectly with the PR's goal of supporting partial configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the IndexingOptions merge implementation
ast-grep --pattern 'impl $_ for IndexingOptions {
$$$
fn merge($_, $_) {
$$$
}
$$$
}'
Length of output: 99
Script:
#!/bin/bash
# Let's try a broader search for IndexingOptions and its implementation
rg -A 10 "struct IndexingOptions"
# Also search for any merge implementations related to IndexingOptions
rg -A 10 "impl.*IndexingOptions"
# Search for any files containing IndexingOptions to understand the context
fd IndexingOptions
Length of output: 2638
crates/katana/cli/src/options.rs (3)
140-142: LGTM! Chain ID configuration looks perfect, sensei!
The optional chain ID with #[serde(default)] aligns perfectly with the PR objectives for partial configuration. The custom parser and clear documentation about hex string handling are excellent additions.
358-367: Default functions implementation looks great!
The new default functions for metrics configuration follow the established pattern and are properly feature-flagged. Good consistency with other default implementations in the file.
83-85: Verify CORS configuration handling
While the #[serde(default)] addition aligns with the PR objectives for partial configuration, please ensure that the default None value results in a secure default CORS policy in the server implementation.
✅ Verification successful
Based on the search results, I can now generate the final response as the CORS implementation has been found and verified:
CORS implementation is secure with proper defaults
The CORS configuration is properly handled in the codebase:
- When
http_cors_originsisNone, no CORS layer is added (most restrictive) - When origins are specified:
- Single
"*"origin usesAllowOrigin::mirror_request() - Specific origins are validated as proper URIs
- Single
- Default headers and methods are strictly defined
- Implementation uses
tower_http::corswhich is a well-maintained library
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CORS implementation to verify secure defaults
rg -A 5 "cors" --type rust
Length of output: 10330
crates/torii/cli/src/options.rs (5)
63-63: Ohayo! LGTM - Proper default handling for optional relay paths
The addition of #[serde(default)] for local_key_path and cert_path aligns perfectly with the PR objective of allowing partial configurations.
Also applies to: 74-74
105-105: LGTM - Proper default handling for indexing options
The addition of #[serde(default)] attributes enables partial configuration for indexing options, which aligns with the PR objectives.
Also applies to: 133-133, 144-144, 154-154
216-216: LGTM - Proper default handling for events options
The addition of #[serde(default)] attributes enables proper deserialization of partial configurations for events options.
Also applies to: 226-226
254-254: LGTM - Proper default handling for CORS origins
The addition of #[serde(default)] for http_cors_origins correctly enables optional CORS configuration.
272-272: LGTM - Proper default handling for metrics toggle
The addition of #[serde(default)] for the metrics field aligns with the default implementation and enables optional metrics configuration.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2692 +/- ##
=======================================
Coverage 56.34% 56.35%
=======================================
Files 411 411
Lines 52703 52742 +39
=======================================
+ Hits 29698 29723 +25
- Misses 23005 23019 +14 ☔ View full report in Codecov by Sentry. |
This PR will ensure the configuration files can only contain part of the fields, which will imply default value for the others.
Summary by CodeRabbit
New Features
MetricsOptionsstruct for enhanced metrics server configuration.ServerOptions.IndexingOptionswith a merging method for better configuration handling.Bug Fixes
indexingoptions to ensure accurate configuration application.Documentation