fix: katana granular config for starknet #2677
Conversation
WalkthroughOhayo, sensei! This pull request introduces modifications to the 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 (1)
bin/katana/src/cli/options.rs (1)
111-121: Ohayo sensei! The merge implementation looks good, but could use some documentation.The merge logic is well-implemented, correctly handling both the environment options and the optional genesis field. However, adding documentation would help clarify the merging behavior.
Consider adding documentation like this:
impl StarknetOptions { + /// Merges another StarknetOptions into this one. + /// + /// # Arguments + /// * `other` - The StarknetOptions to merge from + /// + /// # Behavior + /// - Environment options are always merged + /// - Genesis is only set if the current genesis is None pub fn merge(&mut self, other: Option<&Self>) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/katana/src/cli/node.rs(6 hunks)bin/katana/src/cli/options.rs(2 hunks)
🔇 Additional comments (4)
bin/katana/src/cli/options.rs (1)
158-174: Ohayo sensei! The merge logic needs documentation and verification.
The implementation looks solid but has two areas for improvement:
- Documentation to explain the merging behavior
- Verification of the default value preservation logic
Add documentation to clarify the merging behavior:
impl EnvironmentOptions {
+ /// Merges another EnvironmentOptions into this one.
+ ///
+ /// # Arguments
+ /// * `other` - The EnvironmentOptions to merge from
+ ///
+ /// # Behavior
+ /// - chain_id is only set if the current chain_id is None
+ /// - validate_max_steps is only updated if at default value
+ /// - invoke_max_steps is only updated if at default value
pub fn merge(&mut self, other: Option<&Self>) {Let's verify the default value behavior:
✅ Verification successful
Ohayo sensei! The merge behavior is correctly implemented and consistent with the codebase
The verification confirms that:
- The default values are properly imported from
katana_node::config::execution - These constants are consistently used across the codebase:
- As default values in CLI args via
#[arg(default_value_t)] - As default values in the struct's implementation
- In the merge logic comparison
- As default values in CLI args via
- The merge behavior correctly preserves non-default values while allowing overrides of default values
The suggested documentation addition would still be valuable for clarity, but the implementation is solid and behaves as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the default values in the code match the imported constants
# Search for the DEFAULT constants and their usage
rg "DEFAULT_(VALIDATION|INVOCATION)_MAX_STEPS" -A 2
Length of output: 3159
bin/katana/src/cli/node.rs (3)
363-363: Ohayo sensei! The config merging looks good!
The change to always merge the starknet config using the merge method is a good improvement, as it allows for more flexible configuration management.
508-509: LGTM! New imports are properly organized.
The addition of FromStr and Felt imports supports the new chain ID configuration functionality.
Also applies to: 519-519
695-696: Test coverage looks comprehensive, sensei!
The test cases properly verify:
- The new
invoke_max_stepsconfiguration - Chain ID configuration and parsing
- CLI arguments taking precedence over config file values
However, it would be good to add a negative test case for invalid chain ID values.
Let's verify if there are any other chain ID related tests:
Also applies to: 713-714, 721-721, 736-736
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2677 +/- ##
==========================================
- Coverage 57.59% 57.57% -0.02%
==========================================
Files 404 404
Lines 51101 51139 +38
==========================================
+ Hits 29431 29444 +13
- Misses 21670 21695 +25 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
bin/katana/src/cli/options.rs (2)
111-121: Ohayo! The merge implementation looks good, sensei!The merge logic correctly handles both the environment options and genesis field. Consider adding documentation to explain the merge behavior, especially regarding the precedence rules when merging configurations.
Add documentation like this:
impl StarknetOptions { + /// Merges another StarknetOptions into this one. + /// The current values take precedence over the other values. + /// The environment options are merged recursively. + /// The genesis is only set if the current value is None. pub fn merge(&mut self, other: Option<&Self>) {
225-249: Excellent implementation of development options merging, sensei!The merge logic correctly handles all development-related settings. The use of default values as triggers for updates is consistent with the pattern used in other merge implementations.
Consider extracting the default value comparisons into a helper trait to reduce code duplication across merge implementations. Here's a suggestion:
trait ConfigMerge { fn should_update(&self, default: Self) -> bool; } impl ConfigMerge for bool { fn should_update(&self, default: bool) -> bool { *self == default } } impl ConfigMerge for u16 { fn should_update(&self, default: u16) -> bool { *self == default } } // Then in DevOptions: if self.total_accounts.should_update(DEFAULT_DEV_ACCOUNTS) { self.total_accounts = other.total_accounts; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/katana/src/cli/node.rs(6 hunks)bin/katana/src/cli/options.rs(3 hunks)
🔇 Additional comments (5)
bin/katana/src/cli/options.rs (1)
158-174: Clean implementation of environment options merging, sensei!
The merge logic correctly handles all fields and uses default values as triggers for updates. This ensures that CLI-provided values take precedence over configuration file values.
Let's verify that the default constants are correctly imported and used:
✅ Verification successful
Ohayo! The default constants are properly imported and used, sensei!
The verification confirms that:
- The constants are correctly defined in
katana_node::config::execution - They are properly imported in
bin/katana/src/cli/options.rs - The merge implementation correctly uses these constants for comparison
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the default constants usage
rg "DEFAULT_(VALIDATION|INVOCATION)_MAX_STEPS" --type rust
Length of output: 1620
bin/katana/src/cli/node.rs (4)
503-504: LGTM! Clean import organization.
The new imports are properly organized and grouped with related dependencies.
Also applies to: 514-514
708-709: LGTM! Proper chain ID configuration support.
The implementation correctly handles both named and hex chain IDs, with appropriate test coverage.
Also applies to: 731-731
690-691: LGTM! Well-tested invoke max steps configuration.
The new invoke_max_steps configuration is properly implemented and tested, maintaining backward compatibility with default values.
Also applies to: 716-716
363-364: LGTM! Enhanced config merging logic.
The new implementation provides more consistent behavior by always merging configurations using dedicated merge methods.
Let's verify the merge behavior:
Summary by CodeRabbit
New Features
StarknetOptions,EnvironmentOptions, andDevOptions, allowing for more flexible integration of settings.invoke_max_stepsfield for granular control over invocation limits instarknet.environment.Bug Fixes
starknetconfiguration merging to ensure consistent behavior regardless of initial state.Documentation