feat: katana config file #2668
feat: katana config file #2668
Conversation
WalkthroughOhayo, sensei! This pull request introduces several updates primarily focused on dependency management and configuration handling within the project. A new dependency, Changes
Possibly related PRs
🪧 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: 2
🧹 Outside diff range and nitpick comments (8)
crates/katana/node/src/config/rpc.rs (1)
27-27: Consider adding documentation for the cors_origins field, sensei!The field would benefit from documentation explaining:
- The expected format of origins
- Examples of valid values
- Behavior when None (presumably all origins are blocked)
Add documentation like this:
pub struct RpcConfig { pub addr: IpAddr, pub port: u16, pub max_connections: u32, pub apis: HashSet<ApiKind>, + /// List of allowed CORS origins. When None, all CORS requests are blocked. + /// Origins should include protocol, domain and optionally port. + /// Example: vec!["http://localhost:3000".to_string()] pub cors_origins: Option<Vec<String>>, }crates/dojo/test-utils/src/sequencer.rs (1)
Ohayo! We found some inconsistency in CORS naming, sensei!
The codebase still uses
cors_domainincrates/katana/node-bindings/src/lib.rswhile the rest of the codebase has moved tocors_origins. This needs to be standardized for consistency.
crates/katana/node-bindings/src/lib.rs: Update the following to usecors_origins:
http_cors_domainfieldhttp_cors_domain()method🔗 Analysis chain
Line range hint
121-126: Ohayo! The CORS configuration field renaming looks good, sensei!The renaming from
cors_domaintocors_originsaligns well with the standardization effort across the codebase.Let's verify the consistency of this naming across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of cors_origins vs cors_domain # Expect: Only cors_origins should be present in the codebase echo "Checking for any remaining cors_domain usage..." rg "cors_domain" echo "Verifying cors_origins usage..." rg "cors_origins"Length of output: 1193
crates/katana/primitives/src/genesis/mod.rs (1)
50-50: Consider adding serialization tests, sensei!The addition of
DeserializeandPartialEqlooks good! Since this struct contains complex nested types and will be used for configuration, it would be valuable to add round-trip serialization tests.Would you like me to help generate some serialization test cases to ensure the configuration parsing works as expected?
crates/katana/core/src/service/messaging/mod.rs (1)
97-97: Ohayo! Nice serialization enhancement, sensei!The addition of
SerializetoMessagingConfigenables powerful configuration management capabilities. However, I noticed that while the struct uses JSON for file loading, the project seems to be adding TOML support elsewhere.Consider adding TOML support for consistency with other configuration handling in the project:
impl MessagingConfig { /// Load the config from a JSON file. pub fn load(path: impl AsRef<Path>) -> Result<Self, std::io::Error> { let buf = std::fs::read(path)?; - serde_json::from_slice(&buf).map_err(|e| e.into()) + if path.as_ref().extension().map_or(false, |ext| ext == "toml") { + toml::from_slice(&buf).map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e)) + } else { + serde_json::from_slice(&buf).map_err(|e| e.into()) + } } }bin/katana/src/cli/options.rs (3)
4-4: Ohayo, Sensei! Typographical Error in DocumentationThere's a small typo in the comment on line 4: "form" should be "from".
143-190: Ohayo, Sensei! Consider Defaulting Dev Mode to True in Development EnvironmentsCurrently,
devdefaults tofalse, and other development options requiredevto be enabled. If this tool is primarily used in development environments, consider defaultingdevtotrueto enhance usability.
173-173: Ohayo, Sensei! Clarify Comment for Better UnderstandingConsider rephrasing the comment on line 173 for clarity:
Original:
/// Skipping the transaction sender's account validation function.Suggested:
/// Skip the transaction sender's account validation during execution.This clarifies that account validation is being bypassed when this option is enabled.
bin/katana/src/cli/node.rs (1)
329-395: Consider Implementing Recursive Merging for Nested ConfigurationsOhayo, sensei! The current merging logic operates at the top level and does not recursively merge nested structures. For future enhancements, implementing recursive merging would provide more granular control and flexibility over configuration settings.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(1 hunks)bin/katana/Cargo.toml(1 hunks)bin/katana/src/cli/mod.rs(2 hunks)bin/katana/src/cli/node.rs(9 hunks)bin/katana/src/cli/options.rs(1 hunks)bin/katana/src/utils.rs(2 hunks)crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/core/src/service/messaging/mod.rs(2 hunks)crates/katana/node/src/config/metrics.rs(1 hunks)crates/katana/node/src/config/rpc.rs(2 hunks)crates/katana/node/src/lib.rs(1 hunks)crates/katana/primitives/src/block.rs(1 hunks)crates/katana/primitives/src/genesis/mod.rs(3 hunks)
🔇 Additional comments (24)
crates/katana/node/src/config/metrics.rs (1)
9-9: Ohayo! Nice improvement to the config ergonomics, sensei!
Adding the Copy trait is a good choice here since MetricsConfig is a small, stack-friendly struct with all fields already implementing Copy. This makes the config easier to work with by allowing value semantics instead of move semantics.
bin/katana/Cargo.toml (1)
19-29: Ohayo! These dependency additions look perfect, sensei! ✨
The new dependencies align perfectly with the goal of adding config file support:
cainome-cairo-serdefor Cairo type serializationserdefor the core serialization traitstomlfor config file parsing
The use of workspace-level version management (workspace = true) maintains consistency across the project.
crates/katana/node/src/config/rpc.rs (1)
27-27: Ohayo! The field rename improves semantic accuracy.
The change from cors_domain to cors_origins better aligns with web standards terminology, as CORS specifically deals with origins rather than just domains.
bin/katana/src/cli/mod.rs (2)
3-3: Ohayo! New module looks clean and organized, sensei!
The options module addition follows good module organization practices, keeping related functionality together.
29-29: Ohayo! Let's verify the impact of this change, sensei!
The addition of with_config_file()? changes the execution flow by requiring configuration file handling before node execution. While the implementation looks correct, we should ensure this breaking change is properly documented.
Let's check for documentation updates:
✅ Verification successful
Ohayo! The config file change is well-documented and implemented, sensei!
The implementation shows that:
- The change is part of a larger feature commit (67159b8) that properly adds config file support
- The
with_config_file()implementation innode.rscorrectly merges CLI args with config file values, prioritizing CLI args - The change includes comprehensive test coverage verifying the config file functionality
- The implementation uses TOML format and proper error handling with the
?operator
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for documentation updates related to the config file changes
# Check README and documentation files
echo "Checking documentation files..."
rg -l "config.*file" --type md
# Check for examples of config files
echo "Checking for config file examples..."
fd -e toml -e yaml -e json . | grep -i config
Length of output: 331
Script:
#!/bin/bash
# Let's check the implementation and documentation of with_config_file
# Check the implementation
echo "Checking with_config_file implementation..."
ast-grep --pattern 'fn with_config_file'
# Check for any related tests
echo "Checking related tests..."
rg "with_config_file" --type rust -C 2
# Check commit messages for context
echo "Checking commit messages..."
git log -p --grep="config.*file"
# Check for any migration guides or breaking changes docs
echo "Checking for breaking changes documentation..."
fd BREAKING -t f
fd CHANGELOG -t f
fd UPGRADE -t f
Length of output: 49068
bin/katana/src/utils.rs (2)
10-10: Ohayo! The serde import looks perfect, sensei!
The addition of serde traits aligns well with the katana config file feature objective.
41-45: Ohayo! The LogFormat changes are well-crafted, sensei!
The addition of serialization support and default implementation is clean and maintains backward compatibility. The choice of Full as the default variant is logical for maximum visibility.
Let's verify that this enum is properly used in config files:
✅ Verification successful
This will help us understand the usage context better and verify if the serialization changes are properly supported.
Ohayo! The LogFormat changes are properly integrated, sensei!
The verification shows that LogFormat is correctly used throughout the codebase:
- In
cli/options.rs, theLoggingOptionsstruct properly uses LogFormat with serialization support - In
cli/node.rs, LogFormat is correctly handled in the logging configuration - The enum is properly used for both CLI arguments and configuration serialization
The implementation maintains backward compatibility while adding serialization support, and the Full default is consistently used across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of LogFormat in config-related code
# Expect: Config structs that contain LogFormat should also be serializable
# Search for structs containing LogFormat
ast-grep --pattern 'struct $name {
$$$
$_: LogFormat,
$$$
}'
# Verify these structs have proper derive macros
rg -B2 "struct.*LogFormat"
Length of output: 97
Script:
#!/bin/bash
# Let's try a broader search to find LogFormat usage
# First, find direct references to LogFormat
rg -l "LogFormat"
# Then check for any config-related structs that might use it
rg -B3 -A3 "LogFormat"
# Also search for potential config file examples
fd -e yaml -e toml -e json
Length of output: 8687
crates/katana/primitives/src/genesis/mod.rs (2)
12-12: Ohayo! Import change looks good, sensei!
The addition of Deserialize to the serde import is consistent with the new derive attributes.
Line range hint 26-35: Ohayo! The GenesisClass changes look solid, sensei!
The addition of Deserialize aligns well with the config file support objective. The existing skip_serializing attributes are preserved, maintaining the efficient handling of large fields.
Let's verify that we're not accidentally deserializing this struct elsewhere in the codebase:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the usage context better and verify our concerns about deserialization.
Ohayo sensei! The GenesisClass deserialization changes are safe and sound!
Based on the codebase analysis, the GenesisClass struct is only deserialized in controlled contexts:
- In
genesis/json.rsfor genesis configuration loading - In test utilities for setting up test environments
The skip_serializing attributes are properly maintained for the large fields (casm and sierra), and there are no unexpected deserialization points in the codebase that could cause issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential deserialization of GenesisClass
rg -l "GenesisClass.*from|serde.*GenesisClass"
Length of output: 48
Script:
#!/bin/bash
# Let's try a more comprehensive search for GenesisClass usage
rg -A 3 "GenesisClass"
# Also search specifically for serde related imports in files containing GenesisClass
rg --multiline --multiline-dotall "use.*serde.*;\n.*GenesisClass"
# And check for any test files that might be deserializing GenesisClass
fd "test" -e rs -x rg -l "GenesisClass"
Length of output: 115640
Cargo.toml (1)
68-68: Ohayo sensei! The dependency addition looks good!
The new cainome-cairo-serde dependency is properly configured, maintaining consistency by using the same repository and revision as the existing cainome dependency.
Let's verify the dependency usage across the workspace:
crates/katana/core/src/service/messaging/mod.rs (1)
54-54: Ohayo! Clean import addition, sensei!
The serde import is properly scoped and well-placed with other external dependencies.
crates/katana/primitives/src/block.rs (1)
Line range hint 18-22: Ohayo! The addition of PartialEq looks great, sensei! 🎯
Adding PartialEq to the derive attributes is a solid improvement that enables equality comparisons for block identifiers. This is particularly valuable for configuration handling and testing scenarios.
crates/katana/node/src/lib.rs (1)
316-329: LGTM on the CORS configuration update!
The change from cors_domain to cors_origins is a good improvement as it better reflects the purpose of the configuration. The handling of wildcard origins and the overall CORS setup looks correct.
bin/katana/src/cli/options.rs (5)
27-48: Ohayo, Sensei! MetricsOptions Struct Looks Solid
The MetricsOptions struct is well-defined, and the default implementations ensure proper functionality.
60-93: Ohayo, Sensei! ServerOptions Struct is Well-Structured
The ServerOptions struct correctly handles server configurations, and the default values are appropriately set.
95-106: Ohayo, Sensei! StarknetOptions Configuration Handling
The StarknetOptions struct effectively handles the merging of command-line arguments and configuration file values. The use of Option<Genesis> allows for flexibility in genesis configuration.
108-141: Ohayo, Sensei! EnvironmentOptions Default Functions
The default functions default_validate_max_steps and default_invoke_max_steps ensure proper serialization and deserialization of the EnvironmentOptions. This setup maintains consistency across configurations.
218-248: Ohayo, Sensei! Verify Import of cainome_cairo_serde Crate
Ensure that the cainome-cairo-serde crate is properly included in your Cargo.toml and that it's correctly imported. This is essential for the serialization and deserialization of hexadecimal values in GasPriceOracleOptions.
Run the following script to confirm the inclusion of the crate:
✅ Verification successful
Ohayo, Sensei! The cainome-cairo-serde Crate is Properly Configured
The crate is correctly included in both the workspace-level Cargo.toml and properly referenced in bin/katana/Cargo.toml through workspace inheritance. The dependency is sourced from the specified GitHub repository at the pinned revision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `cainome-cairo-serde` is included in Cargo.toml
# Test: Search for the crate in Cargo.toml files.
# Expect: Lines containing `cainome-cairo-serde` as a dependency.
fd Cargo.toml | xargs rg 'cainome-cairo-serde'
Length of output: 240
bin/katana/src/cli/node.rs (6)
45-46: Deriving Serialize, Deserialize, and Default for NodeArgs
Ohayo, sensei! By deriving Serialize, Deserialize, and Default for NodeArgs, we enable configuration parsing from files and ease the default initialization. This enhancement looks good and aligns with Rust's best practices.
103-105: Adding config field to NodeArgs
Introducing the config: Option<PathBuf> field allows users to specify a configuration file path, enhancing flexibility. This addition is a valuable improvement.
108-125: Creation of NodeArgsConfig for Configuration Handling
Ohayo, sensei! The NodeArgsConfig struct provides a way to deserialize configuration options with optional fields. This design facilitates merging configurations from files with command-line arguments. Well done!
260-277: Simplifying Gas Price Configuration Logic
The refactored conditions for setting gas prices enhance code readability and maintainability. The use of if statements directly modifies the fixed_gas_prices when applicable, which is a clear and efficient approach.
289-290: Using Custom Invocation and Validation Max Steps
Setting invocation_max_steps and validation_max_steps from self.starknet.environment provides flexibility for configuring execution limits. This change is beneficial for users requiring custom settings.
683-735: Adding Test for Configuration Merging from File and CLI
Including the config_from_file_and_cli test ensures that the merging logic between the configuration file and CLI arguments works as expected. This test is comprehensive and covers various scenarios.
| let cors = | ||
| config.cors_origins.clone().map(|allowed_origins| match allowed_origins.as_slice() { | ||
| [origin] if origin == "*" => cors.allow_origin(AllowOrigin::mirror_request()), | ||
| origins => cors.allow_origin( | ||
| origins | ||
| .iter() | ||
| .map(|o| { | ||
| let _ = o.parse::<Uri>().expect("Invalid URI"); | ||
|
|
||
| o.parse().expect("Invalid origin") | ||
| }) | ||
| .collect::<Vec<_>>(), | ||
| ), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Let's optimize the URI validation logic.
The code contains duplicate URI parsing which is unnecessary and could be simplified:
let cors =
config.cors_origins.clone().map(|allowed_origins| match allowed_origins.as_slice() {
[origin] if origin == "*" => cors.allow_origin(AllowOrigin::mirror_request()),
origins => cors.allow_origin(
origins
.iter()
.map(|o| {
- let _ = o.parse::<Uri>().expect("Invalid URI");
-
- o.parse().expect("Invalid origin")
+ o.parse::<Uri>()
+ .unwrap_or_else(|e| panic!("Invalid origin URI '{}': {}", o, e))
+ .to_string()
+ .parse()
+ .expect("Failed to parse validated URI as origin")
})
.collect::<Vec<_>>(),
),
});Additionally, consider adding proper error handling instead of using expect to provide better feedback to users when they configure invalid origins.
📝 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.
| let cors = | |
| config.cors_origins.clone().map(|allowed_origins| match allowed_origins.as_slice() { | |
| [origin] if origin == "*" => cors.allow_origin(AllowOrigin::mirror_request()), | |
| origins => cors.allow_origin( | |
| origins | |
| .iter() | |
| .map(|o| { | |
| let _ = o.parse::<Uri>().expect("Invalid URI"); | |
| o.parse().expect("Invalid origin") | |
| }) | |
| .collect::<Vec<_>>(), | |
| ), | |
| }); | |
| let cors = | |
| config.cors_origins.clone().map(|allowed_origins| match allowed_origins.as_slice() { | |
| [origin] if origin == "*" => cors.allow_origin(AllowOrigin::mirror_request()), | |
| origins => cors.allow_origin( | |
| origins | |
| .iter() | |
| .map(|o| { | |
| o.parse::<Uri>() | |
| .unwrap_or_else(|e| panic!("Invalid origin URI '{}': {}", o, e)) | |
| .to_string() | |
| .parse() | |
| .expect("Failed to parse validated URI as origin") | |
| }) | |
| .collect::<Vec<_>>(), | |
| ), | |
| }); |
| pub fn with_config_file(mut self) -> Result<Self> { | ||
| let config: NodeArgsConfig = if let Some(path) = &self.config { | ||
| toml::from_str(&std::fs::read_to_string(path)?)? | ||
| } else { | ||
| return Ok(self); | ||
| }; | ||
|
|
||
| // the CLI (self) takes precedence over the config file. | ||
| // Currently, the merge is made at the top level of the commands. | ||
| // We may add recursive merging in the future. | ||
|
|
||
| if !self.silent { | ||
| self.silent = config.silent.unwrap_or_default(); | ||
| } | ||
|
|
||
| if !self.no_mining { | ||
| self.no_mining = config.no_mining.unwrap_or_default(); | ||
| } | ||
|
|
||
| if self.block_time.is_none() { | ||
| self.block_time = config.block_time; | ||
| } | ||
|
|
||
| if self.db_dir.is_none() { | ||
| self.db_dir = config.db_dir; | ||
| } | ||
|
|
||
| if self.logging == LoggingOptions::default() { | ||
| if let Some(logging) = config.logging { | ||
| self.logging = logging; | ||
| } | ||
| } | ||
|
|
||
| if self.metrics == MetricsOptions::default() { | ||
| if let Some(metrics) = config.metrics { | ||
| self.metrics = metrics; | ||
| } | ||
| } | ||
|
|
||
| if self.server == ServerOptions::default() { | ||
| if let Some(server) = config.server { | ||
| self.server = server; | ||
| } | ||
| } | ||
|
|
||
| if self.starknet == StarknetOptions::default() { | ||
| if let Some(starknet) = config.starknet { | ||
| self.starknet = starknet; | ||
| } | ||
| } | ||
|
|
||
| if self.gpo == GasPriceOracleOptions::default() { | ||
| if let Some(gpo) = config.gpo { | ||
| self.gpo = gpo; | ||
| } | ||
| } | ||
|
|
||
| if self.forking == ForkingOptions::default() { | ||
| if let Some(forking) = config.forking { | ||
| self.forking = forking; | ||
| } | ||
| } | ||
|
|
||
| if self.development == DevOptions::default() { | ||
| if let Some(development) = config.development { | ||
| self.development = development; | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "slot")] | ||
| if self.slot == SlotOptions::default() { | ||
| if let Some(slot) = config.slot { | ||
| self.slot = slot; | ||
| } | ||
| } | ||
|
|
||
| Ok(self) | ||
| } |
There was a problem hiding this comment.
Potential Issue with Merging Boolean Flags in with_config_file
Ohayo, sensei! There might be a logical issue in how boolean flags are merged in the with_config_file method. The current implementation may not accurately prioritize command-line arguments over configuration file settings for boolean flags.
Issue Explanation:
The merging logic uses patterns like:
if !self.silent {
self.silent = config.silent.unwrap_or_default();
}Here's the concern:
- If
self.silentisfalse(default or not set via CLI), the condition!self.silentistrue, andself.silentis set to the value from the config file. - If the user sets
--silentvia CLI (makingself.silenttrue), the config file value is ignored, which is correct.
However, if the user does not set --silent via CLI and the config file does not have silent set (i.e., config.silent is None), config.silent.unwrap_or_default() returns false, and self.silent remains false.
This logic may lead to unexpected results if the default values are not intended to be false. Moreover, it does not consider cases where the user wants to explicitly set false in the config file.
Suggested Fix:
To ensure that command-line arguments take precedence and to accurately merge the boolean flags, consider modifying the merging logic to check if the config file provides a value:
if config.silent.is_some() && !self.silent {
self.silent = config.silent.unwrap_or(false);
}This way, we only overwrite self.silent with the config file value if it's provided (Some) and if the CLI did not set it (self.silent is false).
Apply similar changes to the other boolean flags in the method.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2668 +/- ##
==========================================
+ Coverage 57.35% 57.41% +0.06%
==========================================
Files 402 403 +1
Lines 50645 50818 +173
==========================================
+ Hits 29049 29179 +130
- Misses 21596 21639 +43 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
cainome-cairo-serde,serde, andtoml.Improvements
Bug Fixes
cors_domaintocors_originsfor clarity and consistency.These changes aim to improve the overall functionality and usability of the application, particularly in configuration management and command-line interactions.