feat(katana): config file #2667
Conversation
* feat: add sozo dev back * fix: add debounce logic for better experience
…ine#2660) * Updated functions to clear up naming with serialized * Removed some redundecies in the code * Removed some redundecies in the code * updated benchmarks * chore: bump scarb * fix: rebuild test database --------- Co-authored-by: glihm <[email protected]>
* Updated functions to clear up naming with serialized * Removed some redundecies in the code * Removed some redundecies in the code * updated benchmarks * Reduced events geenrated code and updated to match model convention * feat: add EventValue for ABI * fix: tests --------- Co-authored-by: glihm <[email protected]>
…#2646) * feat(torii): configutation file for all torii cli options * fix: config * skip config * wrap up config * optional world address * hisotrical event message sconfig * remove upgrade * fmt clippy
…ngine#2665) * fix: ensure tests and other commands are using the config world block * refactor: refacto auth including external resources * fix: add world as valid keyword for grant/revoke
* docs: fix typo * docs: clarify syscall behavior
…gine#2640) * fix: ensure proper documentation for the Resource enum * fix: fmt
fix: remove unused code
…ojoengine#2639) * refactor: apply code simplification to break with bool value * tests: regenerate test db * fix: update policies * fix: update policies
WalkthroughOhayo, sensei! This pull request introduces several changes primarily focused on the configuration and functionality of the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (1)
🔇 Additional comments (17)crates/katana/node/src/config/metrics.rs (1)
The default configuration using localhost:9100 follows common metrics server conventions, and the type choices provide good flexibility. bin/katana/Cargo.toml (1)
The addition of Also applies to: 25-25 crates/katana/node/src/config/rpc.rs (1)
The rename from Let's check for any documentation that needs updating: bin/katana/src/cli/mod.rs (1)
The addition of Let's verify the config file handling: Consider:
✅ Verification successfulBased on the shell script results, I can now generate the final response: Ohayo sensei! The config file handling is well-implemented with proper fallbacks The verification shows that the config file handling is properly implemented with good defaults and dev mode support:
The only missing piece is documentation about the config file format, but this doesn't block existing workflows since the 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for default config handling and documentation
# Test 1: Look for default config or --dev related code
rg -A 5 "with_config_file|--dev"
# Test 2: Check if there's documentation about config files
fd -g "README*" -X cat {} | rg -A 10 "config.*file|configuration"
Length of output: 6318 bin/torii/Cargo.toml (1)
The change to use crates/katana/node/src/config/dev.rs (2)
The imports are well-organized and bring in specific gas price constants needed for the default configuration. Line range hint While the individual fields are well-documented, we should ensure there's clear documentation about the gas price configuration behavior, particularly:
✅ Verification successfulGas price configuration behavior is well documented, sensei! The codebase shows clear implementation and documentation of gas price behavior:
The implementation in 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for documentation about gas price configuration behavior
echo "Searching for gas price configuration documentation..."
rg -l "gas.?price" --type md
# Look for related test cases
echo "Searching for related test cases..."
rg -l "fixed_gas_prices" --type rust | grep test
Length of output: 319 Script: #!/bin/bash
# Let's check the content of the test file to understand the behavior
echo "Checking test file content..."
rg -A 10 "fixed_gas_prices" crates/dojo/test-utils/src/sequencer.rs
# Let's also search for gas price related code in the implementation
echo "Searching for gas price implementation..."
ast-grep --pattern 'impl $_ {
$$$
gas_price$$$
$$$
}'
# Look for any comments or documentation about gas prices in rust files
echo "Searching for gas price documentation in code..."
rg -B 2 -A 2 "gas.?price" --type rust
Length of output: 74707 bin/katana/src/utils.rs (2)
The new imports are properly organized and provide the necessary traits for enum serialization, CLI integration, and display formatting. Also applies to: 5-6, 10-10
Let's ensure this new enum is properly integrated with the configuration system and CLI arguments. ✅ Verification successfulLet me explain my approach:
This should give us a better picture of how LogFormat is integrated into the system. LogFormat is properly integrated with the system The verification shows that LogFormat is well-integrated:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check how LogFormat is used in the configuration and CLI system
# Look for LogFormat usage in configuration structs
echo "Checking LogFormat usage in configuration structs..."
rg -p "struct.*LogFormat"
# Look for CLI argument definitions using LogFormat
echo "Checking CLI argument definitions..."
rg -p "LogFormat.*arg"
Length of output: 250 Script: #!/bin/bash
# Let's try a broader search for LogFormat usage
echo "Checking all LogFormat references..."
rg "LogFormat" -A 3 -B 3
# Check for configuration-related files
echo "Checking configuration-related files..."
fd "config|cli" --type f --extension rs
# Check for command-line argument parsing
echo "Checking command-line argument parsing..."
rg "clap::.*LogFormat|LogFormat.*clap" -A 3 -B 3
Length of output: 3526 crates/dojo/test-utils/src/sequencer.rs (1)
The change from Consider adding a doc comment to explain the expected format and purpose of the ✅ Verification successfulOhayo! The CORS configuration change is consistent across the codebase, sensei! The change from
Note: The remaining 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that the CORS configuration field name is consistent across the codebase
# and check for any documentation about the new field.
# Check for all occurrences of the new field name
echo "Checking for cors_domain usage:"
rg "cors_domain" -A 2
# Check if there's any remaining usage of the old field name
echo -e "\nChecking for any remaining allowed_origins:"
rg "allowed_origins"
# Look for documentation about the CORS configuration
echo -e "\nChecking for CORS documentation:"
rg -i "cors|cross.origin" README.md docs/
Length of output: 3762 crates/katana/primitives/src/genesis/mod.rs (3)
The addition of Line range hint The addition of
While adding Let's check for validation implementations: ✅ Verification successfulOhayo sensei! The Genesis deserialization is well-protected! 🛡️ The implementation includes robust validation during deserialization:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Look for validation implementations for Genesis deserialization
# Search for any custom deserialization implementations
rg -A 5 "impl.*Deserialize.*for.*Genesis"
# Search for validation functions that might be called during or after deserialization
rg -A 5 "fn.*validate" crates/katana/primitives/src/genesis/
Length of output: 106 Script: #!/bin/bash
# Let's try a broader search for validation patterns in the Genesis struct
# Look for the Genesis struct definition and its implementation blocks
rg -A 20 "struct Genesis" crates/katana/primitives/src/genesis/
# Search for any impl blocks for Genesis
rg -A 10 "impl Genesis" crates/katana/primitives/src/genesis/
# Look for any TryFrom implementations that might contain validation
rg -A 10 "impl.*TryFrom.*for.*Genesis"
# Search for any methods that might be doing validation
rg -A 5 "(verify|check|valid|ensure)" crates/katana/primitives/src/genesis/
Length of output: 44129 Script: #!/bin/bash
# Let's check for any custom deserialization or validation in the json module
# since that's where the Genesis struct is converted from JSON
# Look for the TryFrom implementation details
rg -A 20 "impl TryFrom<GenesisJson> for Genesis" crates/katana/primitives/src/genesis/
# Check for any error handling or validation in the json module
rg -A 10 "Error|Invalid|Valid" crates/katana/primitives/src/genesis/json.rs
# Look for any tests that might show validation behavior
rg -A 10 "#\[test\]" crates/katana/primitives/src/genesis/
Length of output: 20550 .github/workflows/ci.yml (1)
The reorganization of flags under the Let's verify if there are other katana invocations that need updating: ✅ Verification successfulOhayo sensei! The katana CLI changes are properly contained The search results confirm this is the only instance of katana invocation in the workflow files, so no other updates are needed. The CLI structure change is properly isolated to this single usage. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other katana invocations in workflow files
rg --type yaml 'katana.*--' .github/workflows/
Length of output: 144 crates/katana/runner/src/lib.rs (1)
The change from crates/katana/core/src/service/messaging/mod.rs (1)
The crates/katana/node/src/lib.rs (1)Line range hint The change to use 🪧 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (12)
crates/katana/node/src/config/metrics.rs (2)
9-15: Consider enhancing the documentation, sensei!While the current documentation is clear, it would be helpful to add examples and explain the relationship between these fields and the socket_addr() method.
/// Node metrics configurations. +/// +/// # Examples +/// +/// ``` +/// use katana::config::MetricsConfig; +/// use std::net::{IpAddr, Ipv4Addr}; +/// +/// let config = MetricsConfig { +/// addr: IpAddr::V4(Ipv4Addr::LOCALHOST), +/// port: 9100, +/// }; +/// +/// // Use socket_addr() to get the combined SocketAddr +/// let addr = config.socket_addr(); +/// ``` #[derive(Debug, Copy, Clone)]
17-21: Clean implementation, sensei!The socket_addr method elegantly combines the configuration fields. Consider adding a note about the return value format in the documentation.
- /// Returns the [`SocketAddr`] for the metrics server. + /// Returns the [`SocketAddr`] for the metrics server by combining the configured address and port. + /// + /// The returned address will be in the format `addr:port`. pub fn socket_addr(&self) -> SocketAddr {crates/katana/node/src/config/rpc.rs (1)
Line range hint
40-45: Default implementation maintains secure defaults, sensei!The implementation correctly initializes
cors_domaintoNone, which is secure by default as it effectively disables CORS until explicitly configured.Consider documenting the CORS configuration process in the README or documentation, especially since this is a breaking change that affects API accessibility.
crates/katana/node/src/config/dev.rs (1)
43-53: Implementation looks solid, but could use some documentation love, sensei!The Default implementation is clean and well-structured. However, it would be helpful to add documentation explaining:
- Why these specific default values were chosen
- When developers might want to override these defaults
- The relationship between ETH and STRK gas prices
Add documentation like this:
/// Fixed gas prices for development. #[derive(Debug, Clone)] pub struct FixedL1GasPriceConfig { pub gas_price: GasPrices, pub data_gas_price: GasPrices, } +/// Default implementation for development gas prices. +/// Uses predefined constants that provide reasonable defaults for both ETH and STRK +/// in a development environment. impl std::default::Default for FixedL1GasPriceConfig {bin/katana/src/utils.rs (2)
41-46: The LogFormat enum looks clean and well-structured, sensei!Consider adding documentation comments to describe the purpose of each variant and when to use them. This would help other developers understand the implications of choosing each format.
Add documentation like this:
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize, Default)] +/// Defines the output format for logs pub enum LogFormat { + /// JSON format for machine-readable logs Json, #[default] + /// Full format for human-readable logs Full, }
48-68: The trait implementations are solid, sensei!The
ValueEnumandDisplayimplementations provide good CLI integration and string conversion. However, consider adding aTryFrom<String>implementation to handle parsing from strings in a more robust way, which could be useful for configuration file parsing.Here's a suggested implementation:
impl TryFrom<String> for LogFormat { type Error = anyhow::Error; fn try_from(s: String) -> Result<Self, Self::Error> { match s.to_lowercase().as_str() { "json" => Ok(Self::Json), "full" => Ok(Self::Full), _ => Err(anyhow::anyhow!("Invalid log format: {}", s)), } } }.github/workflows/ci.yml (1)
196-196: Consider documenting the new CLI structure.Since this is part of a config file feature that changes the CLI structure, it would be helpful to document these changes in the README or CLI help text.
Would you like me to help create documentation for the new CLI structure, sensei?
crates/katana/node-bindings/src/lib.rs (5)
257-259: Ohayo, sensei! Consider renaming method for consistencyThe
portmethod setsself.http_port, which could be confusing. To maintain consistency with the field name, consider renaming the method tohttp_port.
275-276: Ohayo, sensei! Rename parameter for clarityIn the
l1_providermethod, the parameter is namedrpc_url, while the field isl1_provider. For consistency and to avoid confusion, consider renaming the parameter tol1_providerorprovider_url.
330-331: Ohayo, sensei! Align parameter name with fieldIn the
http_cors_domainmethod, the parameter is namedallowed_origins, but the field ishttp_cors_domain. For clarity, consider renaming the parameter tohttp_cors_domainto match the field name.
449-463: Ohayo, sensei! Refactor duplicate--devflag handlingThe logic to prevent setting the
--devflag multiple times is repeated. Consider refactoring this into a helper function or consolidating conditions to improve code readability and maintainability.
504-521: Ohayo, sensei! Simplify metrics flag handlingSimilar to the
--devflag handling, the code for managing the--metricsflag has duplicate logic. Refactoring this section could reduce repetition and potential errors.
🛑 Comments failed to post (6)
crates/katana/core/src/service/messaging/mod.rs (1)
97-97:
⚠️ Potential issueOhayo! Consider protecting sensitive data, sensei!
While adding
Serializeis great for config management, we should be careful about serializing theprivate_keyfield. Consider adding#[serde(skip_serializing)]to theprivate_keyfield to prevent accidental exposure in logs or exported configs.Here's a suggested improvement:
#[derive(Debug, Default, Deserialize, Clone, Serialize)] pub struct MessagingConfig { pub chain: String, pub rpc_url: String, pub contract_address: String, pub sender_address: String, + #[serde(skip_serializing)] pub private_key: String, pub interval: u64, pub from_block: u64, }📝 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.#[derive(Debug, Default, Deserialize, Clone, Serialize)] pub struct MessagingConfig { pub chain: String, pub rpc_url: String, pub contract_address: String, pub sender_address: String, #[serde(skip_serializing)] pub private_key: String, pub interval: u64, pub from_block: u64, }crates/katana/node/src/lib.rs (1)
316-328: 💡 Codebase verification
🛠️ Refactor suggestion
Ohayo! The CORS configuration needs attention across multiple services, sensei!
The review comment's concerns are valid and even more critical than initially thought. The same pattern of double URI parsing and unsafe error handling exists in both
katana/nodeandtorii/server:
crates/katana/node/src/lib.rscrates/torii/server/src/proxy.rsBoth implementations:
- Perform redundant URI parsing
- Use
expect()which can panic in production- Handle CORS configuration at runtime instead of during initialization
Additionally, the configuration is defined in
katana/node/src/config/rpc.rsbut lacks proper validation at the config parsing stage.Suggested improvements:
- Move CORS validation to a shared utility
- Implement it during config parsing
- Use proper error handling instead of
expect()🔗 Analysis chain
Ohayo! Let's enhance the CORS configuration robustness, sensei!
The CORS configuration has a few potential improvements:
- The URI validation is performed twice unnecessarily
- Using
expect()could cause panics in production- Origin validation could be moved to the configuration parsing stage
Here's a suggested improvement:
let cors = config.cors_domain.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().unwrap_or_else(|e| { + tracing::error!("Invalid origin URI '{}': {}", o, e); + panic!("Invalid origin URI: {}", e) + }) }) .collect::<Vec<_>>(), ), });Consider moving the origin validation to the configuration parsing stage to fail fast and provide better error messages. This would also eliminate the need for runtime validation.
Let's check if the CORS configuration is used consistently across the codebase:
Let me analyze the CORS configuration implementation and error handling across the codebase to provide a comprehensive verification of the concerns raised in the review comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CORS-related configuration rg -l "cors|allowed_origins"Length of output: 294
Script:
#!/bin/bash # Let's check the CORS configuration in the config file rg -A 10 "cors" crates/katana/node/src/config/rpc.rs # Also check how CORS is used in other files rg -A 5 "cors" crates/torii/server/src/proxy.rs crates/torii/grpc/src/server/mod.rs # Check if there's any error handling for CORS configuration ast-grep --pattern 'parse::<Uri>()'Length of output: 3250
bin/katana/src/cli/node.rs (4)
540-546: 🛠️ Refactor suggestion
Remove Debugging Statements Before Production Use
Ohayo, sensei! The
dbg!macros at lines 540 and 546 are useful during development but should be removed to clean up the console output and avoid unnecessary performance overhead in production.- dbg!(&config); ... - dbg!(&args);📝 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.NodeArgs::from_merged(matches, Some(config)) } else { NodeArgs::from_arg_matches(&matches)? };
860-870:
⚠️ Potential issueAvoid Temporary String Reference in Test
Ohayo, sensei! In the
config_from_file_and_clitest, you're converting thePathBufto a temporaryStringand then taking a reference to it, which may lead to a dangling reference.Consider storing the
Stringbefore taking a reference:- let config = NodeArgs::parse_from([ - "katana", - "--config", - path.to_string_lossy().to_string().as_str(), - "--validate-max-steps", - "1234", - "--dev", - "--dev.no-fee", - ]) + let config_path = path.to_string_lossy().to_string(); + let config = NodeArgs::parse_from([ + "katana", + "--config", + config_path.as_str(), + "--validate-max-steps", + "1234", + "--dev", + "--dev.no-fee", + ]);This ensures the
Stringremains in scope while it's being used.📝 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 config_path = path.to_string_lossy().to_string(); let config = NodeArgs::parse_from([ "katana", "--config", config_path.as_str(), "--validate-max-steps", "1234", "--dev", "--dev.no-fee", ]) .config() .unwrap();
537-545:
⚠️ Potential issueUndefined Type
NodeArgsConfigOhayo, sensei! In the
with_config_filemethod, the typeNodeArgsConfigis used but not defined within the provided code. This will result in a compilation error.To resolve this, you can define
NodeArgsConfigor adjust the code to useNodeArgsdirectly:- let config: NodeArgsConfig = serde_json::from_str(&std::fs::read_to_string(path)?)?; + let config: NodeArgs = serde_json::from_str(&std::fs::read_to_string(path)?)?;Ensure that
NodeArgsimplementsDeserializefromserdeto support this change.Committable suggestion skipped: line range outside the PR's diff.
385-389:
⚠️ Potential issuePossible Type Mismatch in Log Filter Variable
Ohayo, sensei! In the
init_loggingfunction, there might be a type mismatch with thefiltervariable. Whenself.development.devistrue,filteris a&Stringdue to the use offormat!, but whenfalse, it's a&str. This could lead to issues when passingfiltertoEnvFilter::try_new(filter)?, which expects a type that implementsAsRef<str>.To ensure consistent types, consider modifying the code as follows:
- let filter = if self.development.dev { - &format!("{DEFAULT_LOG_FILTER},server=debug") - } else { - DEFAULT_LOG_FILTER - }; + let filter = if self.development.dev { + format!("{DEFAULT_LOG_FILTER},server=debug") + } else { + DEFAULT_LOG_FILTER.to_string() + };Then, when using
filter, pass it as a reference tostr:- let filter = EnvFilter::try_from_default_env().or(EnvFilter::try_new(filter))?; + let filter = EnvFilter::try_from_default_env().or(EnvFilter::try_new(&filter))?;This ensures
filterconsistently implementsAsRef<str>.Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores