refactor: move cli args to separate crates #2681
Conversation
WalkthroughOhayo, sensei! This pull request introduces several updates across multiple 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: 3
🧹 Outside diff range and nitpick comments (13)
crates/katana/cli/src/lib.rs (2)
3-5: Clean module organization, sensei!The separation into
node,options, andutilsmodules provides a clear and maintainable structure. This modular approach aligns perfectly with the PR's objective of better CLI argument organization.Consider documenting the responsibility of each module in the module-level documentation (//! comments) to help future contributors understand the codebase organization better.
7-8: Consider making the options re-exports more explicit, sensei!While the specific re-exports from the
nodemodule are great, the wildcard re-export fromoptionsmight make it harder to track the public API surface.Consider listing out specific re-exports from the options module:
pub use node::{NodeArgs, NodeArgsConfig}; -pub use options::*; +pub use options::{ + // List specific types/functions here + // Example: OptionType1, OptionType2 +};bin/katana/src/cli/mod.rs (1)
29-29: Ohayo! Good separation of execution logic, sensei!The execution flow change nicely separates concerns by delegating to
node::execute. Consider adding a brief comment explaining this architectural decision for future maintainers.+ // Delegate execution to node module for better separation of concerns node::execute(&self.node.with_config_file()?)bin/torii/Cargo.toml (1)
Line range hint
1-100: Consider standardizing dependency version managementOhayo sensei! I notice we have a mix of workspace-managed versions and specifically pinned versions. For better maintainability, consider:
Moving these direct version dependencies to workspace-level management:
ctrlc = "3.4"either = "1.9.0"http-body = "0.4.5"tokio-stream = "0.1.11"tokio-util = "0.7.7"webbrowser = "0.8"clap_config = "0.1.1"Evaluating if the git dependency could be replaced with a published version:
hyper-reverse-proxy = { git = "https://github.com/tarrencev/hyper-reverse-proxy" }Cargo.toml (1)
39-39: Ohayo! The new CLI workspace members look good, sensei!The addition of separate CLI crates for both Katana and Torii aligns perfectly with the modularization objective. This separation will make the CLI components more maintainable and reusable.
This modular approach will make it easier to:
- Share CLI configurations across different components
- Test CLI components in isolation
- Maintain version control of CLI features independently
Also applies to: 49-49
crates/torii/cli/src/options.rs (2)
10-21: Ohayo! The constants look good, but could use some documentation.The change to make these constants public aligns well with the PR objectives for better CLI arg modularity. However, adding documentation comments would make it easier for other developers to understand the purpose and impact of each constant.
Consider adding doc comments like this:
+/// Default HTTP server address (localhost) pub const DEFAULT_HTTP_ADDR: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST); +/// Default HTTP server port pub const DEFAULT_HTTP_PORT: u16 = 8080;
Line range hint
47-54: Ohayo sensei! Found a small documentation error in the help text.The help text for
websocket_portincorrectly mentions WebRTC transport instead of WebSocket transport.Apply this fix:
/// Port to serve Libp2p WebRTC transport #[arg( long = "relay.websocket_port", value_name = "PORT", default_value_t = DEFAULT_RELAY_WEBSOCKET_PORT, - help = "Port to serve Libp2p WebRTC transport." + help = "Port to serve Libp2p WebSocket transport." )]crates/torii/cli/src/args.rs (6)
26-34: Ohayo, sensei! Inconsistent documentation fordb_dir.There is a mismatch between the comments and the help message for
db_dir. The comments mention "file," while the help message refers to "directory." This might confuse users about what path to provide.Consider standardizing the terminology. Here's a suggested change:
/// Database filepath (ex: indexer.db). If specified file doesn't exist, it will be -/// created. Defaults to in-memory database. +/// created. Defaults to in-memory database. #[arg(long)] #[arg( value_name = "PATH", - help = "Database filepath. If specified directory doesn't exist, it will be created. \ + help = "Database filepath. If specified file doesn't exist, it will be created. \ Defaults to in-memory database." )] pub db_dir: Option<PathBuf>,
68-68: Ohayo, sensei! Enhance error context when reading config file.When reading the configuration file, adding context to the error can help users diagnose issues more effectively.
Consider adjusting the error handling:
- toml::from_str(&std::fs::read_to_string(path)?)? + toml::from_str( + &std::fs::read_to_string(path) + .with_context(|| format!("Failed to read config file at {}", path.display()))? + )?
81-85: Ohayo, sensei! Avoid usingunwrap()on URL parsing.Using
unwrap()onUrl::parsecan cause a panic if the URL is invalid. AlthoughDEFAULT_RPC_URLis a constant, it's safer to handle potential errors gracefully.Consider handling the potential error:
- if self.rpc == Url::parse(DEFAULT_RPC_URL).unwrap() { + if self.rpc == Url::parse(DEFAULT_RPC_URL).expect("Invalid default RPC URL") {Or, better yet, parse the URL once at the start and handle any errors:
pub const DEFAULT_RPC_URL: &str = "http://0.0.0.0:5050"; +lazy_static::lazy_static! { + static ref DEFAULT_RPC_URL_PARSED: Url = Url::parse(DEFAULT_RPC_URL).expect("Invalid default RPC URL"); +} // ... if self.rpc == *DEFAULT_RPC_URL_PARSED { if let Some(rpc) = config.rpc { self.rpc = rpc; } }
95-97: Ohayo, sensei! Consider implementing recursive configuration merging.The current configuration merging operates only at the top level. Implementing recursive merging would allow more granular control over nested configurations, enhancing flexibility for users.
164-164: Ohayo, sensei! File extension mismatch in test configuration.In the
test_cli_precedencefunction, the test writes TOML content to a file namedtorii-config2.json. For clarity and consistency, consider changing the file extension to.toml.Suggested change:
- let path = std::env::temp_dir().join("torii-config2.json"); + let path = std::env::temp_dir().join("torii-config2.toml");
224-224: Ohayo, sensei! File extension mismatch in test configuration.In the
test_config_fallbackfunction, the test writes TOML content to a file namedtorii-config.json. To maintain consistency, consider changing the file extension to.toml.Suggested change:
- let path = std::env::temp_dir().join("torii-config.json"); + let path = std::env::temp_dir().join("torii-config.toml");
📜 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 (15)
Cargo.toml(4 hunks)bin/katana/Cargo.toml(2 hunks)bin/katana/src/cli/mod.rs(3 hunks)bin/katana/src/cli/node.rs(1 hunks)bin/katana/src/main.rs(0 hunks)bin/torii/Cargo.toml(1 hunks)bin/torii/src/main.rs(1 hunks)crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/lib.rs(1 hunks)crates/katana/cli/src/node.rs(1 hunks)crates/katana/cli/src/utils.rs(1 hunks)crates/torii/cli/Cargo.toml(1 hunks)crates/torii/cli/src/args.rs(1 hunks)crates/torii/cli/src/lib.rs(1 hunks)crates/torii/cli/src/options.rs(1 hunks)
💤 Files with no reviewable changes (1)
- bin/katana/src/main.rs
✅ Files skipped from review due to trivial changes (3)
- crates/katana/cli/Cargo.toml
- crates/torii/cli/Cargo.toml
- crates/torii/cli/src/lib.rs
🔇 Additional comments (12)
crates/katana/cli/src/lib.rs (1)
1-1: Ohayo! Nice compiler attribute configuration, sensei!
The conditional warning for unused crate dependencies is a great practice. It helps keep the dependency tree clean during development while avoiding false positives during testing.
bin/katana/Cargo.toml (2)
13-13: Ohayo! The dependency change aligns with the refactoring goal.
The addition of katana-cli as a workspace dependency correctly implements the PR's objective of moving CLI arguments to separate crates.
37-37: Feature flag update looks good, sensei!
The slot feature has been properly updated to use the CLI-specific feature flag katana-cli/slot instead of the direct dependency, maintaining the functionality while improving modularity.
Let's verify the workspace structure and feature availability:
bin/katana/src/cli/mod.rs (2)
17-17: Ohayo! Clean type update, sensei!
The field type change is consistent with the new import while maintaining the flatten behavior for CLI parsing.
7-7: Ohayo! Import change looks good but let's verify dependencies
The move of NodeArgs to katana_cli aligns well with the modularization objective, sensei!
Let's verify the impact of this change:
✅ Verification successful
The previous results show that NodeArgs is still referenced from node.rs and katana-cli is properly added as a dependency, but let's verify the actual implementation and usage to ensure consistency.
Ohayo! The import change is perfectly safe, sensei!
The move of NodeArgs to katana-cli is properly implemented:
- The dependency is correctly added in Cargo.toml files
- All usages of
NodeArgsnow reference it fromkatana_cli - The node.rs file correctly imports it via
katana_cli::node::NodeArgs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if NodeArgs is still referenced from the old location
rg "node::NodeArgs"
# Verify that katana_cli is properly added as a dependency
rg "katana-cli" -g "Cargo.toml"
Length of output: 339
Script:
#!/bin/bash
# Let's check the content of node.rs to understand the usage
cat bin/katana/src/cli/node.rs
# Check if there are any other references to NodeArgs in the codebase
rg "NodeArgs" -A 2
Length of output: 11204
bin/torii/Cargo.toml (1)
36-36: Ohayo! The torii-cli workspace dependency looks good!
The addition of torii-cli as a workspace dependency aligns perfectly with the PR's objective of moving CLI arguments to separate crates, sensei! This will help improve modularity and reusability.
crates/katana/cli/src/utils.rs (1)
76-76: Ohayo! Verify test data location, sensei!
The test file path has been simplified. Let's ensure the test data is properly organized.
✅ Verification successful
Let me search for more context about the test data organization.
This will help us understand the test data organization patterns and locate the correct path for the genesis.json file.
Ohayo! The test data path is correct, sensei!
The path change is valid as ./test-data/genesis.json exists at crates/katana/cli/test-data/genesis.json. This matches the new test file location used in the CLI tests. The change aligns with the project's test data organization, where each crate maintains its own test data directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test data organization
# Check if the test data exists at the new location
if [ -f "./test-data/genesis.json" ]; then
echo "✓ Test data found at new location"
else
echo "✗ Test data not found at new location"
fi
# Check for any other test files following the old pattern
echo "Checking for other test files following old pattern..."
fd -g "**/tests/test-data/genesis.json"
# Check for any other test files following the new pattern
echo "Checking for other test files following new pattern..."
fd -g "**/test-data/genesis.json"
Length of output: 438
Script:
#!/bin/bash
# Let's check the actual test file content and surrounding context
rg -B 5 -A 5 "genesis\.json" --type rust
# Also check if there are any other genesis.json files in the repository
fd "genesis.json"
# Look for test data directory patterns
fd "test-data"
fd "tests/test-data"
Length of output: 5439
Cargo.toml (1)
108-108: The workspace dependencies are properly configured!
The new CLI dependencies are correctly referenced with relative paths, maintaining consistency with other workspace dependencies.
Let's verify the crate structure exists:
Also applies to: 117-117
✅ Verification successful
Ohayo sensei! The CLI crates structure is properly configured! ✨
The verification confirms that both CLI crates exist and are properly structured:
crates/katana/cliexists with a valid Cargo.toml that uses workspace dependenciescrates/torii/cliexists with a valid Cargo.toml that uses workspace dependencies
Both crates follow the workspace configuration pattern consistently, with proper directory structure and dependency management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CLI crates exist and have proper Cargo.toml files
# Check if the CLI crates exist and have Cargo.toml files
for crate in "crates/katana/cli" "crates/torii/cli"; do
if [ ! -f "${crate}/Cargo.toml" ]; then
echo "Missing Cargo.toml in ${crate}"
exit 1
fi
done
Length of output: 198
Script:
#!/bin/bash
# Let's verify the existence and content of the CLI crates more thoroughly
echo "=== Checking katana CLI crate ==="
if [ -d "crates/katana/cli" ]; then
ls -la crates/katana/cli/
if [ -f "crates/katana/cli/Cargo.toml" ]; then
echo "\nContents of katana CLI Cargo.toml:"
cat crates/katana/cli/Cargo.toml
fi
fi
echo -e "\n=== Checking torii CLI crate ==="
if [ -d "crates/torii/cli" ]; then
ls -la crates/torii/cli/
if [ -f "crates/torii/cli/Cargo.toml" ]; then
echo "\nContents of torii CLI Cargo.toml:"
cat crates/torii/cli/Cargo.toml
fi
fi
Length of output: 2202
bin/torii/src/main.rs (1)
32-32: Ohayo, Sensei! Importing ToriiArgs from torii_cli enhances modularity
The import statement correctly brings in ToriiArgs from the torii_cli crate, aligning with the goal of separating CLI arguments into dedicated crates. This improves the modularity and reusability of the codebase.
crates/torii/cli/src/args.rs (2)
149-193: Ohayo, sensei! Excellent unit test for CLI precedence.
The test_cli_precedence function thoroughly verifies that command-line arguments take precedence over configuration file settings. This ensures robust and predictable behavior for users.
195-263: Ohayo, sensei! Excellent unit test for configuration fallback.
The test_config_fallback function effectively tests that configuration file values are correctly used when command-line arguments are not provided, ensuring reliable fallback mechanisms.
crates/katana/cli/src/node.rs (1)
110-122: Implementation of config method is clear and correct
Ohayo, sensei! The config method effectively aggregates configurations by calling helper methods and constructs the overall Config object accurately.
| pub const DEFAULT_HTTP_ADDR: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST); | ||
| pub const DEFAULT_HTTP_PORT: u16 = 8080; | ||
| pub const DEFAULT_METRICS_ADDR: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST); | ||
| pub const DEFAULT_METRICS_PORT: u16 = 9200; | ||
| pub const DEFAULT_EVENTS_CHUNK_SIZE: u64 = 1024; | ||
| pub const DEFAULT_BLOCKS_CHUNK_SIZE: u64 = 10240; | ||
| pub const DEFAULT_POLLING_INTERVAL: u64 = 500; | ||
| pub const DEFAULT_MAX_CONCURRENT_TASKS: usize = 100; | ||
|
|
||
| pub const DEFAULT_RELAY_PORT: u16 = 9090; | ||
| pub const DEFAULT_RELAY_WEBRTC_PORT: u16 = 9091; | ||
| pub const DEFAULT_RELAY_WEBSOCKET_PORT: u16 = 9092; |
There was a problem hiding this comment.
💡 Codebase verification
Found duplicate metrics constants with different values
Ohayo sensei! Found duplicate definitions of metrics-related constants with different values:
-
DEFAULT_METRICS_ADDRis defined in:crates/torii/cli/src/options.rs(LOCALHOST)crates/katana/node/src/config/metrics.rs(LOCALHOST)
-
DEFAULT_METRICS_PORTis defined in:crates/torii/cli/src/options.rs(9200)crates/katana/node/src/config/metrics.rs(9100)
The metrics constants should be consolidated to avoid confusion and potential conflicts. Consider moving these to a shared location or ensuring the values are intentionally different between Torii and Katana.
🔗 Analysis chain
Verify no duplicate constants exist in the codebase.
Since these constants are now public and part of the CLI separation, let's ensure there are no duplicate definitions elsewhere.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any duplicate definitions of these constants
for const in "DEFAULT_HTTP_ADDR" "DEFAULT_HTTP_PORT" "DEFAULT_METRICS_ADDR" "DEFAULT_METRICS_PORT" "DEFAULT_EVENTS_CHUNK_SIZE" "DEFAULT_BLOCKS_CHUNK_SIZE" "DEFAULT_POLLING_INTERVAL" "DEFAULT_MAX_CONCURRENT_TASKS" "DEFAULT_RELAY_PORT" "DEFAULT_RELAY_WEBRTC_PORT" "DEFAULT_RELAY_WEBSOCKET_PORT"; do
echo "Checking for duplicate definitions of $const:"
rg "const\s+$const\s*:" --type rust
done
Length of output: 6440
bin/katana/src/cli/node.rs
Outdated
| let filter = if args.development.dev { | ||
| &format!("{DEFAULT_LOG_FILTER},server=debug") | ||
| } else { | ||
| DEFAULT_LOG_FILTER | ||
| }; |
There was a problem hiding this comment.
Fix type mismatch in let filter assignment
Ohayo sensei, in the let filter assignment, when args.development.dev is true, you're using &format!(...) which returns a &String, while the else branch returns a &str. This causes a type mismatch because the two branches must return the same type.
To resolve the issue, consider returning a String from both branches:
-let filter = if args.development.dev {
- &format!("{DEFAULT_LOG_FILTER},server=debug")
-} else {
- DEFAULT_LOG_FILTER
-};
+let filter = if args.development.dev {
+ format!("{DEFAULT_LOG_FILTER},server=debug")
+} else {
+ DEFAULT_LOG_FILTER.to_string()
+};Then, when using EnvFilter::try_new(filter)?, you can pass &filter to match the expected &str type.
📝 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 filter = if args.development.dev { | |
| &format!("{DEFAULT_LOG_FILTER},server=debug") | |
| } else { | |
| DEFAULT_LOG_FILTER | |
| }; | |
| let filter = if args.development.dev { | |
| format!("{DEFAULT_LOG_FILTER},server=debug") | |
| } else { | |
| DEFAULT_LOG_FILTER.to_string() | |
| }; |
crates/katana/cli/src/node.rs
Outdated
| if !self.silent { | ||
| self.silent = config.silent.unwrap_or_default(); | ||
| } | ||
|
|
||
| if !self.no_mining { | ||
| self.no_mining = config.no_mining.unwrap_or_default(); | ||
| } |
There was a problem hiding this comment.
Potential issue with merging boolean flags: consider using Option<bool>
Ohayo, sensei! In the with_config_file method, the logic for merging boolean flags like silent and no_mining may not handle cases where the command-line arguments are explicitly set to false. Since boolean flags default to false, it's challenging to distinguish between an unset value and one explicitly set to false.
Consider changing the boolean fields in NodeArgs to Option<bool> to differentiate between unset and set values. This adjustment will allow for accurate merging of command-line arguments and configuration file settings.
Apply this diff to fix the issue:
- pub silent: bool,
+ pub silent: Option<bool>,And update the merging logic in with_config_file:
- if !self.silent {
+ if self.silent.is_none() {
self.silent = config.silent.unwrap_or_default();
}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2681 +/- ##
==========================================
- Coverage 57.57% 57.51% -0.06%
==========================================
Files 404 406 +2
Lines 51139 51197 +58
==========================================
+ Hits 29441 29447 +6
- Misses 21698 21750 +52 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
crates/katana/cli/src/utils.rs (3)
36-51: Enhance test coverage for genesis parsingThe current test only verifies successful parsing. Consider adding tests for:
- Invalid JSON format
- Missing required fields
- Shell expansion edge cases
- File not found scenarios
#[cfg(test)] mod tests { use super::*; use tempfile::NamedTempFile; use std::io::Write; #[test] fn test_parse_genesis_various_cases() { // Test invalid JSON let mut invalid_json = NamedTempFile::new().unwrap(); write!(invalid_json, "{{invalid}}").unwrap(); assert!(parse_genesis(invalid_json.path().to_str().unwrap()).is_err()); // Test missing required fields let mut missing_fields = NamedTempFile::new().unwrap(); write!(missing_fields, "{{}}").unwrap(); assert!(parse_genesis(missing_fields.path().to_str().unwrap()).is_err()); // Test file not found assert!(parse_genesis("/nonexistent/path").is_err()); } }Also applies to: 194-203
82-125: Consider optimizing the ASCII art displayThe ASCII art banner increases binary size. Consider:
- Making it optional via a feature flag
- Moving it to a separate resource file
- Adding a --no-banner flag
163-192: Optimize string formatting in print_genesis_accountsConsider using a string builder pattern or format_args! for more efficient string formatting, especially when dealing with multiple accounts.
fn print_genesis_accounts<'a, Accounts>(accounts: Accounts) where Accounts: Iterator<Item = (&'a ContractAddress, &'a GenesisAccountAlloc)>, { + use std::fmt::Write; + let mut output = String::with_capacity(1024); println!( r" PREFUNDED ACCOUNTS ==================" ); for (addr, account) in accounts { if let Some(pk) = account.private_key() { - println!( - r" -| Account address | {addr} -| Private key | {pk:#x} -| Public key | {:#x}", - account.public_key() - ) + writeln!(output, "\n| Account address | {addr}").unwrap(); + writeln!(output, "| Private key | {pk:#x}").unwrap(); + writeln!(output, "| Public key | {:#x}", account.public_key()).unwrap(); } else { - println!( - r" -| Account address | {addr} -| Public key | {:#x}", - account.public_key() - ) + writeln!(output, "\n| Account address | {addr}").unwrap(); + writeln!(output, "| Public key | {:#x}", account.public_key()).unwrap(); } } + print!("{output}"); }crates/katana/cli/src/node.rs (2)
Line range hint
116-125: Ohayo, sensei! Enhance error context in execute and start_node methodsConsider adding more descriptive error contexts to help with debugging:
pub fn execute(&self) -> Result<()> { self.init_logging()?; tokio::runtime::Builder::new_multi_thread() .enable_all() .build() - .context("failed to build tokio runtime")? + .context("failed to build tokio runtime: check system resources and thread limits")? .block_on(self.start_node()) } async fn start_node(&self) -> Result<()> { // Build the node let config = self.config()?; - let node = katana_node::build(config).await.context("failed to build node")?; + let node = katana_node::build(config).await + .context("failed to build node: verify configuration and system requirements")?;
520-520: Ohayo, sensei! Consider making test data paths configurableThe hardcoded test data paths "./test-data/genesis.json" might cause issues if the test directory structure changes. Consider:
- Using a constant or environment variable for the test data directory
- Moving test data to a standard location like
tests/data/- Using relative paths from the test file's location
+ const TEST_DATA_DIR: &str = "./test-data"; + const GENESIS_FILE: &str = "genesis.json"; + #[test] fn genesis_with_fixed_gas_prices() { let config = NodeArgs::parse_from([ "katana", "--genesis", - "./test-data/genesis.json", + &format!("{}/{}", TEST_DATA_DIR, GENESIS_FILE),Also applies to: 576-576
📜 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 (5)
bin/katana/Cargo.toml(1 hunks)bin/katana/src/cli/mod.rs(2 hunks)crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/node.rs(8 hunks)crates/katana/cli/src/utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/katana/src/cli/mod.rs
🔇 Additional comments (11)
bin/katana/Cargo.toml (2)
10-10: Ohayo sensei! Dependencies restructuring looks good!
The addition of katana-cli and removal of direct dependencies aligns perfectly with the PR's objective of moving CLI arguments to separate crates. Using workspace dependencies is also a solid practice for maintaining version consistency.
26-29: Verify feature flag propagation
The feature flags have been updated to use the new katana-cli crate, but let's verify that all necessary features are properly transferred and accessible.
✅ Verification successful
Let me gather more specific information about the feature dependencies.
This will help us verify the complete feature dependency chain and ensure proper propagation between the crates.
Feature flags are properly propagated, sensei! ✨
The feature flag chain is correctly implemented:
bin/katanacorrectly depends onkatana-cli/slotin its default featureskatana-cliproperly manages the slot feature withkatana-slot-controllerdependency- The starknet-messaging feature chain flows correctly:
bin/katana->katana-cli->katana-node->katana-core
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify feature flags in katana-cli crate match the removed dependencies
# Check if all features from katana-core and katana-slot-controller are available in katana-cli
echo "Checking katana-cli Cargo.toml for feature definitions..."
rg -A 5 '^\[features\]' $(fd -t f 'Cargo.toml' | grep 'katana-cli')
# Verify no remaining references to old dependencies
echo "Checking for any remaining references to removed dependencies..."
rg -l 'katana-core|katana-slot-controller' $(fd -t f 'Cargo.toml')
Length of output: 7517
Script:
#!/bin/bash
# Check the actual feature dependencies in the relevant Cargo.toml files
echo "Checking katana-cli feature dependencies..."
cat crates/katana/cli/Cargo.toml
echo -e "\nChecking katana-core feature dependencies..."
cat crates/katana/core/Cargo.toml
echo -e "\nChecking katana-node feature dependencies..."
cat crates/katana/node/Cargo.toml
Length of output: 4003
crates/katana/cli/Cargo.toml (5)
1-6: Ohayo! Package metadata looks good, sensei!
The package configuration properly inherits workspace-level settings while explicitly setting the package name.
8-13: Main dependencies are well structured!
Good use of workspace versioning and proper configuration of optional dependencies.
30-32: Dev dependencies are properly configured!
Good use of workspace versioning for test dependencies.
34-37: Verify the impact of default features
The "slot" feature is enabled by default, which automatically includes katana-slot-controller. Let's verify if this aligns with the expected default behavior across the workspace.
✅ Verification successful
Ohayo sensei! After analyzing the feature configuration, I can now provide the final response:
Default slot feature is properly configured ✨
The default slot feature is well-aligned across the workspace:
- In
katana-primitives, theslotfeature only enables the minimalcontrollerfeature - In
katana-cli, it correctly pulls in both the controller dependency and primitives' slot feature - No other crates require the slot feature as a dependency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how slot feature is used in other crates
echo "Checking slot feature usage:"
rg 'slot.*=.*\[' -g 'Cargo.toml' -A 2
echo -e "\nChecking katana-slot-controller usage:"
rg 'katana-slot-controller.*=' -g 'Cargo.toml'
Length of output: 655
Script:
#!/bin/bash
# Check if slot feature is used in dependencies or build requirements
echo "Checking dependencies requiring slot feature:"
rg 'features.*=.*slot' -g 'Cargo.toml'
echo -e "\nChecking where katana-primitives is used with slot feature:"
rg 'katana-primitives.*slot' -g 'Cargo.toml'
echo -e "\nChecking controller feature usage in primitives:"
cat crates/katana/primitives/Cargo.toml
Length of output: 1870
21-22: Verify fixed dependency versions across workspace
The fixed versions for serde_json and shellexpand might cause version conflicts if these packages are used elsewhere in the workspace.
crates/katana/cli/src/utils.rs (2)
53-80: Ohayo! Clean enum implementation, sensei!
The LogFormat enum is well-structured with appropriate trait implementations for clap integration and display formatting.
127-161: Consider extracting contract constants to configuration
The function uses hardcoded contract addresses and class hashes. Consider moving these to a configuration file or environment variables for better flexibility.
crates/katana/cli/src/node.rs (2)
Line range hint 1-31: Ohayo, sensei! Documentation and imports look good!
The file documentation and imports have been properly updated to reflect the new CLI-focused purpose of this module.
Line range hint 33-365: Ohayo, sensei! Consider using Option<bool> for boolean flags
The boolean flags silent and no_mining in NodeArgs might not handle cases where they are explicitly set to false in the config file. Since boolean flags default to false, it's challenging to distinguish between unset values and explicit false values.
Consider changing these fields to Option<bool> to differentiate between unset and set values:
pub struct NodeArgs {
- pub silent: bool,
- pub no_mining: bool,
+ pub silent: Option<bool>,
+ pub no_mining: Option<bool>,And update the merging logic in with_config_file:
- if !self.silent {
+ if self.silent.is_none() {
self.silent = config.silent.unwrap_or_default();
}
- if !self.no_mining {
+ if self.no_mining.is_none() {
self.no_mining = config.no_mining.unwrap_or_default();
}| pub fn parse_seed(seed: &str) -> [u8; 32] { | ||
| let seed = seed.as_bytes(); | ||
|
|
||
| if seed.len() >= 32 { | ||
| unsafe { *(seed[..32].as_ptr() as *const [u8; 32]) } | ||
| } else { | ||
| let mut actual_seed = [0u8; 32]; | ||
| seed.iter().enumerate().for_each(|(i, b)| actual_seed[i] = *b); | ||
| actual_seed | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Consider a safer implementation for seed parsing
The current implementation uses unsafe code which could lead to undefined behavior if the memory is not properly aligned. Consider using a safer alternative:
pub fn parse_seed(seed: &str) -> [u8; 32] {
let seed = seed.as_bytes();
if seed.len() >= 32 {
- unsafe { *(seed[..32].as_ptr() as *const [u8; 32]) }
+ let mut result = [0u8; 32];
+ result.copy_from_slice(&seed[..32]);
+ result
} else {
let mut actual_seed = [0u8; 32];
seed.iter().enumerate().for_each(|(i, b)| actual_seed[i] = *b);
actual_seed
}
}📝 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.
| pub fn parse_seed(seed: &str) -> [u8; 32] { | |
| let seed = seed.as_bytes(); | |
| if seed.len() >= 32 { | |
| unsafe { *(seed[..32].as_ptr() as *const [u8; 32]) } | |
| } else { | |
| let mut actual_seed = [0u8; 32]; | |
| seed.iter().enumerate().for_each(|(i, b)| actual_seed[i] = *b); | |
| actual_seed | |
| } | |
| } | |
| pub fn parse_seed(seed: &str) -> [u8; 32] { | |
| let seed = seed.as_bytes(); | |
| if seed.len() >= 32 { | |
| let mut result = [0u8; 32]; | |
| result.copy_from_slice(&seed[..32]); | |
| result | |
| } else { | |
| let mut actual_seed = [0u8; 32]; | |
| seed.iter().enumerate().for_each(|(i, b)| actual_seed[i] = *b); | |
| actual_seed | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
crates/torii/cli/Cargo.toml (1)
18-20: Consider adding more testing utilities, sensei!While
assert_matchesandcaminoare good choices, consider adding common testing utilities like:
test-casefor parameterized testspretty_assertionsfor readable test outputcrates/katana/core/src/service/messaging/mod.rs (1)
97-97: Ohayo sensei! ThePartialEqaddition looks good!Adding
PartialEqtoMessagingConfigis a sensible change that aligns well with moving CLI args to separate crates, as it enables configuration comparison functionality. However, consider also addingEqsince the struct doesn't have any floating-point fields that would make equality non-transitive.-#[derive(Debug, Default, Deserialize, Clone, Serialize, PartialEq)] +#[derive(Debug, Default, Deserialize, Clone, Serialize, PartialEq, Eq)]crates/torii/cli/src/args.rs (3)
12-12: Document DEFAULT_RPC_URL usage contextOhayo sensei! Consider adding a doc comment explaining that this URL is for local development/testing purposes.
-pub const DEFAULT_RPC_URL: &str = "http://0.0.0.0:5050"; +/// Default RPC URL for local development/testing. +/// In production environments, this should be overridden via CLI args or config file. +pub const DEFAULT_RPC_URL: &str = "http://0.0.0.0:5050";
70-75: Enhance error handling with contextConsider adding context to the errors for better debugging.
pub fn with_config_file(mut self) -> Result<Self> { let config: ToriiArgsConfig = if let Some(path) = &self.config { - toml::from_str(&std::fs::read_to_string(path)?)? + std::fs::read_to_string(path) + .map_err(|e| anyhow::anyhow!("Failed to read config file {}: {}", path.display(), e))?; + .and_then(|content| toml::from_str(&content) + .map_err(|e| anyhow::anyhow!("Failed to parse config file {}: {}", path.display(), e)))? } else { return Ok(self); };
188-312: Add negative test casesThe tests cover the happy path well, but consider adding tests for:
- Invalid config file format
- Missing config file
- Invalid URLs
- Invalid world addresses
Example test case:
#[test] fn test_invalid_config_file() { let content = r#" invalid_toml_content "#; let path = std::env::temp_dir().join("invalid-config.toml"); std::fs::write(&path, content).unwrap(); let args = vec!["torii", "--config", path.to_str().unwrap()]; let result = ToriiArgs::parse_from(args).with_config_file(); assert!(result.is_err()); }crates/katana/cli/src/options.rs (2)
Line range hint
61-88: Ohayo sensei! Consider validating CORS origins.The
ServerOptionsstruct is well-designed, but thehttp_cors_originsfield might benefit from validation to ensure proper CORS origin format and security.Consider adding a custom parser for CORS origins:
fn parse_cors_origins(s: &str) -> Result<Vec<String>, String> { let origins: Vec<String> = s.split(',') .map(|origin| origin.trim().to_string()) .filter(|origin| !origin.is_empty()) .collect(); // Validate each origin for origin in &origins { if !origin.starts_with("http://") && !origin.starts_with("https://") && origin != "*" { return Err(format!("Invalid CORS origin: {}", origin)); } } Ok(origins) }Then update the field definition:
#[arg(long = "http.cors_origins", value_parser = parse_cors_origins)]
338-350: Ohayo sensei! Consider adding documentation for default functions.While the default functions are correctly implemented, they would benefit from documentation explaining their purpose and usage in the context of serde deserialization.
Add documentation like this:
/// Returns the default HTTP address for the RPC server. /// Used by serde when deserializing configuration. #[cfg(feature = "server")] fn default_http_addr() -> IpAddr { DEFAULT_RPC_ADDR }crates/katana/cli/src/node.rs (1)
Line range hint
94-120: Add documentation for NodeArgsConfig fieldsOhayo! Consider adding documentation comments for each field to explain their purpose and expected values, especially for fields that have specific constraints or behaviors.
#[derive(Debug, Serialize, Deserialize, Default)] +/// Configuration structure that mirrors NodeArgs but with all fields optional +/// for flexible configuration file support pub struct NodeArgsConfig { + /// When true, suppresses all startup output pub silent: Option<bool>, + /// When true, disables automatic and interval mining pub no_mining: Option<bool>, // ... add docs for other fields
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
crates/katana/cli/Cargo.toml(1 hunks)crates/katana/cli/src/node.rs(13 hunks)crates/katana/cli/src/options.rs(4 hunks)crates/katana/core/src/service/messaging/mod.rs(1 hunks)crates/torii/cli/Cargo.toml(1 hunks)crates/torii/cli/src/args.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/cli/Cargo.toml
🔇 Additional comments (10)
crates/torii/cli/Cargo.toml (3)
1-7: Ohayo! Package metadata looks great, sensei!
The package configuration follows Rust workspace best practices by inheriting common fields from the workspace while explicitly setting the package name.
8-17: Dependencies are well-organized, sensei!
All dependencies use workspace inheritance which is excellent for maintaining consistent versions across the project. The selected dependencies are appropriate for a CLI crate, covering argument parsing, error handling, and configuration management.
22-24: Verify the purpose of the empty server feature, sensei!
The "server" feature is currently empty but included in default features. Let's verify its intended usage.
✅ Verification successful
Ohayo, sensei!
The empty server feature is correctly utilized for conditional compilation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the server feature in the codebase
echo "Searching for #[cfg(feature = \"server\")] usage..."
rg --type rust "cfg.*feature.*server"
echo "\nSearching for conditional dependencies using the server feature..."
rg "server.*=" crates/torii/cli/Cargo.toml
Length of output: 3327
crates/torii/cli/src/args.rs (1)
18-67: Consider making world_address required
The world_address seems to be a critical parameter for indexing. Consider making it required unless there's a specific reason for it to be optional.
✅ Verification successful
Ohayo, sensei.
No need to make world_address required
The world_address is correctly handled as optional across the codebase, ensuring flexibility and robustness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if world_address is used as required in other parts of the codebase
rg -A 5 "world_address" --type rust
Length of output: 60290
crates/katana/cli/src/options.rs (3)
15-15: Ohayo sensei! Clean import with proper feature gating.
The import of RPC constants is correctly feature-gated and well-organized.
Line range hint 89-98: Ohayo sensei! Clean Default implementation.
The Default implementation for ServerOptions is well-structured and properly uses the predefined constants.
Line range hint 61-350: Ohayo sensei! Verify server feature usage.
The server-related code is properly feature-gated, but let's verify the feature flag usage across the codebase.
✅ Verification successful
Ohayo sensei! Server feature usage is properly gated and declared.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of the "server" feature flag
# Check for any ungated server-related code
rg -l "DEFAULT_RPC|ServerOptions|http_addr|http_port|max_connections" | \
xargs rg -L '#\[cfg\(feature = "server"\)\]'
# Check Cargo.toml files for proper feature declaration
fd Cargo.toml | xargs rg 'server.*=.*\['
Length of output: 1695
crates/katana/cli/src/node.rs (3)
Line range hint 379-386: Improve boolean flag merging logic
Ohayo! The current boolean flag merging logic might not correctly handle cases where flags are explicitly set to false in CLI arguments.
Consider changing the boolean fields to Option<bool> as suggested in the previous review. The current implementation:
if !self.silent {
self.silent = config.silent.unwrap_or_default();
}Cannot distinguish between "unset" and "explicitly set to false" in CLI args.
Line range hint 456-678: Test coverage looks comprehensive!
Ohayo, sensei! The test suite thoroughly covers:
- Default configurations
- Custom configurations
- Gas price combinations
- Config file and CLI argument interaction
Line range hint 170-179: Consider thread safety implications
Ohayo, sensei! The change from self to &self is good for reusability, but since this allows shared access, we should ensure thread safety if the NodeArgs instance might be accessed from multiple threads.
✅ Verification successful
Ohayo, sensei! Since no Arc or Mutex usages were found with NodeArgs, I'll further check if it's being accessed concurrently in other ways.
Ohayo, sensei! Verified that NodeArgs isn't accessed concurrently without synchronization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential concurrent usage of NodeArgs
rg -A 5 "NodeArgs.*=.*Arc::new"
rg -A 5 "NodeArgs.*=.*Mutex::new"
Length of output: 69
Script:
#!/bin/bash
# Search for NodeArgs being used within tokio::spawn blocks
rg 'tokio::spawn\s*\(' -A 5 | rg 'NodeArgs'
# Search for NodeArgs being moved into async blocks
rg 'async\s*\|' -A 5 | rg 'NodeArgs'
Length of output: 80
| let mut config = | ||
| ToriiArgsConfig { world_address: args.world_address, ..Default::default() }; | ||
|
|
||
| config.world_address = args.world_address; | ||
| config.rpc = |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant world_address assignment
The world_address is assigned twice:
- In the struct initialization
- Immediately after initialization
let mut config =
- ToriiArgsConfig { world_address: args.world_address, ..Default::default() };
+ ToriiArgsConfig { ..Default::default() };
config.world_address = args.world_address;📝 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 mut config = | |
| ToriiArgsConfig { world_address: args.world_address, ..Default::default() }; | |
| config.world_address = args.world_address; | |
| config.rpc = | |
| let mut config = | |
| ToriiArgsConfig { ..Default::default() }; | |
| config.world_address = args.world_address; | |
| config.rpc = |
crates/katana/cli/src/node.rs
Outdated
| /// Configuration file | ||
| #[arg(long)] | ||
| config: Option<PathBuf>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance config file error handling
Ohayo, sensei! The config file handling could be more robust with better error messages.
/// Configuration file
#[arg(long)]
-config: Option<PathBuf>,
+config: Option<PathBuf>,
+
+#[arg(skip)]
+config_loaded: bool,And in with_config_file:
let config: NodeArgsConfig = if let Some(path) = &self.config {
- toml::from_str(&std::fs::read_to_string(path)?)?
+ if !path.exists() {
+ return Err(anyhow::anyhow!("Config file not found: {}", path.display()));
+ }
+ let content = std::fs::read_to_string(path)
+ .with_context(|| format!("Failed to read config file: {}", path.display()))?;
+ toml::from_str(&content)
+ .with_context(|| format!("Failed to parse TOML from config file: {}", path.display()))?Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/katana/cli/src/node.rs (2)
121-165: Consider enhancing error handling in TryFrom implementation.Ohayo, sensei! While the implementation is good, we could improve error handling by providing more context when the config merge fails.
impl TryFrom<NodeArgs> for NodeArgsConfig { type Error = anyhow::Error; fn try_from(args: NodeArgs) -> Result<Self> { // Ensure the config file is merged with the CLI arguments. - let args = args.with_config_file()?; + let args = args.with_config_file() + .with_context(|| "failed to merge config file with CLI arguments")?;
256-265: Consider consolidating feature-gated code blocks.Ohayo, sensei! The feature-gated code for server and metrics configurations is spread across multiple locations. Consider consolidating these into a single module to improve maintainability.
Consider moving all server-related feature-gated code into a separate module like
server_config.rs:#[cfg(feature = "server")] mod server_config { pub fn build_rpc_config(args: &NodeArgs) -> RpcConfig { RpcConfig { apis, port: args.server.http_port, addr: args.server.http_addr, max_connections: args.server.max_connections, cors_origins: args.server.http_cors_origins.clone(), } } pub fn build_metrics_config(args: &NodeArgs) -> Option<MetricsConfig> { if args.metrics.metrics { Some(MetricsConfig { addr: args.metrics.metrics_addr, port: args.metrics.metrics_port, }) } else { None } } }Also applies to: 354-362
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/katana/cli/src/node.rs(13 hunks)
🔇 Additional comments (2)
crates/katana/cli/src/node.rs (2)
57-59: LGTM! Config file support added.
The addition of the config file option enhances flexibility in node configuration.
168-168: LGTM! Method signature changes improve efficiency.
The change from self to &self in execute and start_node methods prevents unnecessary moves, allowing for better resource management.
Also applies to: 177-177
crates/katana/cli/src/node.rs
Outdated
| @@ -644,7 +580,7 @@ mod test { | |||
| let config = NodeArgs::parse_from([ | |||
| "katana", | |||
| "--genesis", | |||
| "./tests/test-data/genesis.json", | |||
| "./test-data/genesis.json", | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: Test data path resolution may be incorrect.
Ohayo, sensei! No valid genesis file found in the specified test data paths.
- ./crates/katana/primitives/src/genesis/test-genesis.json
- ./crates/katana/cli/test-data/genesis.json
🔗 Analysis chain
Verify test data path resolution.
Ohayo, sensei! The test is using a hardcoded path to test data. This might cause issues when running tests from different directories.
Also applies to: 639-639
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if test data exists and is accessible
fd -t f "genesis.json" -x echo "Found test data at: {}"
# Verify the content of genesis.json files
fd -t f "genesis.json" -x cat {} | jq -r 'if (.number == 0 and .parent_hash == "0x999" and .timestamp == 5123512314) then "Valid genesis file found at: {}" else empty end'
Length of output: 369
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
bin/torii/src/main.rs (2)
32-32: Ohayo sensei! Clean architectural separation achieved!The migration of CLI arguments to
torii-clicrate aligns perfectly with the modular design principles. This separation of concerns will make the codebase more maintainable and reusable.
Line range hint
47-209: Ohayo sensei! Consider further modularization of service initialization!While moving CLI args to a separate crate was a great first step, the
mainfunction is still handling multiple responsibilities. Consider extracting service initialization into a dedicated builder or factory pattern.This could look like:
struct ToriiServer { engine: Engine<Arc<JsonRpcClient<HttpTransport>>>, proxy: Arc<Proxy>, graphql: GraphQLServer, grpc: GrpcServer, relay: Relay, } impl ToriiServer { async fn new(args: ToriiArgs) -> Result<Self> { // Current initialization logic moved here } async fn start(self) -> Result<()> { // Current service spawning logic moved here } } #[tokio::main] async fn main() -> anyhow::Result<()> { let args = ToriiArgs::parse().with_config_file()?; let server = ToriiServer::new(args).await?; server.start().await }This would:
- Make the code more testable
- Separate initialization from runtime concerns
- Make it easier to modify individual services
| let database_path = if let Some(db_dir) = args.db_dir { | ||
| // Create the directory if it doesn't exist | ||
| std::fs::create_dir_all(&db_dir)?; | ||
| // Set the database file path inside the directory | ||
| db_dir.join("torii.db") | ||
| } else { | ||
| tempfile.path().to_path_buf() | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Let's enhance the database path handling robustness!
The current implementation could benefit from additional error handling and validation:
- Check directory permissions before creation
- Validate path is writable
- Handle potential race conditions in directory creation
Consider this enhanced implementation:
let database_path = if let Some(db_dir) = args.db_dir {
- // Create the directory if it doesn't exist
- std::fs::create_dir_all(&db_dir)?;
- // Set the database file path inside the directory
- db_dir.join("torii.db")
+ // First check if directory exists and is writable
+ if db_dir.exists() && !db_dir.is_dir() {
+ return Err(anyhow::anyhow!("Database path exists but is not a directory"));
+ }
+
+ // Create directory with specific permissions
+ std::fs::create_dir_all(&db_dir)
+ .map_err(|e| anyhow::anyhow!("Failed to create database directory: {}", e))?;
+
+ // Verify directory is writable
+ let db_path = db_dir.join("torii.db");
+ std::fs::OpenOptions::new()
+ .write(true)
+ .create(true)
+ .open(&db_path)
+ .map_err(|e| anyhow::anyhow!("Database path is not writable: {}", e))?;
+
+ db_path
} else {
tempfile.path().to_path_buf()
};📝 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 database_path = if let Some(db_dir) = args.db_dir { | |
| // Create the directory if it doesn't exist | |
| std::fs::create_dir_all(&db_dir)?; | |
| // Set the database file path inside the directory | |
| db_dir.join("torii.db") | |
| } else { | |
| tempfile.path().to_path_buf() | |
| }; | |
| let database_path = if let Some(db_dir) = args.db_dir { | |
| // First check if directory exists and is writable | |
| if db_dir.exists() && !db_dir.is_dir() { | |
| return Err(anyhow::anyhow!("Database path exists but is not a directory")); | |
| } | |
| // Create directory with specific permissions | |
| std::fs::create_dir_all(&db_dir) | |
| .map_err(|e| anyhow::anyhow!("Failed to create database directory: {}", e))?; | |
| // Verify directory is writable | |
| let db_path = db_dir.join("torii.db"); | |
| std::fs::OpenOptions::new() | |
| .write(true) | |
| .create(true) | |
| .open(&db_path) | |
| .map_err(|e| anyhow::anyhow!("Database path is not writable: {}", e))?; | |
| db_path | |
| } else { | |
| tempfile.path().to_path_buf() | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
crates/katana/cli/src/lib.rs (1)
3-6: Clean module organization aligning with the refactoring goals!The module structure nicely separates concerns:
args: Core argument handlingfile: Configuration file managementoptions: CLI optionsutils: Shared utilitiesThis organization supports the PR's goal of making CLI arguments more modular and reusable.
Consider documenting each module's responsibility in the module declarations to help future contributors understand the codebase structure better.
crates/katana/cli/src/file.rs (3)
10-27: Ohayo sensei! Consider adding field-level documentation.While the struct has a high-level doc comment, adding documentation for individual fields would improve the configuration's usability and maintainability. This is especially important for a configuration struct that will be used across different crates.
Example improvement:
/// Node arguments configuration file. #[derive(Debug, Serialize, Deserialize, Default)] pub struct NodeArgsConfig { + /// When true, disables block mining pub no_mining: Option<bool>, + /// Time between blocks in milliseconds pub block_time: Option<u64>, // ... add docs for other fields
30-33: Enhance error context for better debugging.The error handling could be more descriptive to help users identify configuration issues more easily.
Consider this improvement:
pub fn read(path: impl AsRef<Path>) -> Result<Self> { - let file = std::fs::read_to_string(path)?; - Ok(toml::from_str(&file)?) + let path = path.as_ref(); + let file = std::fs::read_to_string(path) + .map_err(|e| anyhow::anyhow!("Failed to read config file {}: {}", path.display(), e))?; + toml::from_str(&file) + .map_err(|e| anyhow::anyhow!("Failed to parse config file {}: {}", path.display(), e)) }
53-62: Consider extracting the default value comparison pattern.The repeated pattern for handling default values could be simplified using a helper function.
Consider this improvement:
+fn option_if_not_default<T: Default + PartialEq>(value: T) -> Option<T> { + if value == T::default() { None } else { Some(value) } +} + impl TryFrom<NodeArgs> for NodeArgsConfig { // ... - node_config.logging = - if args.logging == LoggingOptions::default() { None } else { Some(args.logging) }; - node_config.starknet = - if args.starknet == StarknetOptions::default() { None } else { Some(args.starknet) }; + node_config.logging = option_if_not_default(args.logging); + node_config.starknet = option_if_not_default(args.starknet); // ... apply to other fieldscrates/katana/cli/src/utils.rs (3)
36-41: Ohayo sensei! Consider adding file existence checkThe function should verify if the file exists before attempting to load it to provide a more user-friendly error message.
pub fn parse_genesis(value: &str) -> Result<Genesis> { let path = PathBuf::from(shellexpand::full(value)?.into_owned()); + if !path.exists() { + anyhow::bail!("Genesis file not found: {}", path.display()); + } let genesis = Genesis::try_from(GenesisJson::load(path)?)?; Ok(genesis) }
43-51: Ohayo sensei! Enhance error message for block number parsingConsider providing a more descriptive error message that includes the invalid value.
- let num = value.parse::<BlockNumber>().context("could not parse block number")?; + let num = value.parse::<BlockNumber>() + .with_context(|| format!("invalid block number: {}", value))?;
127-161: Ohayo sensei! Consider extracting template stringsThe hardcoded template strings could be moved to constants or a separate configuration file for better maintainability.
+const CONTRACT_TEMPLATE: &str = r" +| Contract | {} +| Address | {} +| Class Hash | {:#064x} +"; + fn print_genesis_contracts(chain: &ChainSpec, account_class_hash: Option<ClassHash>) { println!( - r" -| Contract | ETH Fee Token -| Address | {} -| Class Hash | {:#064x} - -| Contract | STRK Fee Token -| Address | {} -| Class Hash | {:#064x}", + concat!( + CONTRACT_TEMPLATE, + "\n", + CONTRACT_TEMPLATE + ), + "ETH Fee Token", chain.fee_contracts.eth, DEFAULT_LEGACY_ERC20_CLASS_HASH, + "STRK Fee Token", chain.fee_contracts.strk, DEFAULT_LEGACY_ERC20_CLASS_HASH );crates/katana/cli/src/args.rs (4)
Line range hint
300-316: Potential issue with merging boolean options inwith_config_filemethodOhayo sensei!
In the
with_config_filemethod, the logic for merging boolean fields likeno_miningmay override command-line arguments unintentionally. Specifically:if !self.no_mining { self.no_mining = config.no_mining.unwrap_or_default(); }If
self.no_miningisfalse(mining is enabled via CLI), it will be set to the value from the config file, possibly overriding the user's intent. Consider checking if the CLI option was provided instead.You might update the merging logic to:
if matches!(self.no_mining, false) && config.no_mining.is_some() { self.no_mining = config.no_mining.unwrap_or_default(); }
Line range hint
375-512: Add unit tests for the new configuration merging logicOhayo sensei!
To ensure the robustness of the
with_config_filemethod, it would be beneficial to add unit tests that cover various scenarios of merging command-line arguments with configuration files. This will help prevent regression and ensure that precedence rules are correctly applied.
508-508: Check the validity of the genesis file path in testsOhayo sensei!
In the test cases, the path to the genesis file is set to
"./test-data/genesis.json". Please ensure that this file exists at the specified location in the test environment to prevent test failures due to missing files.
74-78: Conditional compilation may affect CLI options availabilityOhayo sensei!
The
metricsandserveroptions are conditionally compiled with#[cfg(feature = "server")]. Be aware that when theserverfeature is disabled, these options won't be available, which might cause confusion if users expect these options in all builds.Consider providing feedback or documentation to inform users about the feature-dependent availability of these options.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/katana/cli/src/args.rs(13 hunks)crates/katana/cli/src/file.rs(1 hunks)crates/katana/cli/src/lib.rs(1 hunks)crates/katana/cli/src/utils.rs(1 hunks)
🔇 Additional comments (9)
crates/katana/cli/src/lib.rs (1)
1-1: Ohayo sensei! Nice compiler attribute for dependency hygiene!
The warn(unused_crate_dependencies) attribute is a good practice to keep dependencies clean in production code while allowing flexibility during testing.
crates/katana/cli/src/file.rs (1)
40-41: Verify config file merging implementation.
The comment suggests that config file is merged with CLI arguments, but we should verify this behavior in the with_config_file method.
✅ Verification successful
Ohayo, sensei! I'm generating a new script to search for any instances of with_config_file in the codebase. Let's see what we find! 🌅
Ohayo, sensei! Verified that the with_config_file method correctly merges the config file with the CLI arguments. 🌟
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the with_config_file implementation
ast-grep --pattern 'fn with_config_file($$$) {
$$$
}'
Length of output: 57
Script:
#!/bin/bash
# Search for any definition or usage of `with_config_file`
rg 'with_config_file' --context 3
Length of output: 4114
crates/katana/cli/src/utils.rs (2)
24-34: Ohayo sensei! The previous review comment about unsafe seed parsing is still valid
The current implementation uses unsafe code which could lead to undefined behavior. Please refer to the previous review comment for the suggested safer implementation.
53-80: Ohayo sensei! Clean enum implementation
The LogFormat enum and its trait implementations are well-structured and follow Rust best practices.
crates/katana/cli/src/args.rs (5)
Line range hint 100-109: Changing method receivers to &self may affect future modifications
Ohayo sensei!
The methods execute and start_node have been changed to take &self instead of consuming self. While this is efficient, ensure that these methods won't need ownership of self in the future, especially if modifications or ownership transfers are required.
26-27: Verify the existence and accessibility of NodeArgsConfig
Ohayo sensei!
Ensure that the NodeArgsConfig struct in crate::file is properly defined and accessible. This will prevent potential compilation errors due to missing modules or incorrect paths.
188-201: Ensure the RpcConfig defaults are appropriate when server feature is disabled
Ohayo sensei!
In the rpc_config method, when the server feature is disabled, default values are used for the RpcConfig. Verify that these defaults are appropriate and won't lead to issues like binding to unintended addresses or ports.
286-294: Confirm that metrics are appropriately disabled without the server feature
Ohayo sensei!
The metrics_config function returns None when the server feature is not enabled. Ensure that this doesn't inadvertently disable necessary metrics or conflict with other components expecting metrics to be available.
33-33: Deriving Clone for NodeArgs
Ohayo sensei!
Good job on adding Clone to the NodeArgs struct derivations. This will facilitate easier copying of instances when needed.
| pub use args::NodeArgs; | ||
| pub use options::*; |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei!
Missing NodeArgs definition in args.rs!
The NodeArgs struct is not found in the crates/katana/cli/src/args.rs.
- Ensure that the
NodeArgsstruct is properly defined withinargs.rs.
🔗 Analysis chain
Clean and focused re-exports!
The selective re-export of NodeArgs and wildcard re-export of options provide a good balance between explicit and convenient public API design.
Let's verify the module structure and exports:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the module structure and exports
# Check if all declared modules exist
for module in args file options utils; do
echo "Checking module: $module"
fd --type f "^${module}\.rs$" "crates/katana/cli/src"
done
# Look for NodeArgs definition
ast-grep --pattern 'struct NodeArgs {
$$$
}'
# Check options module exports
rg --type rust "pub (struct|enum|fn|type|const|static)" "crates/katana/cli/src/options.rs"
Length of output: 1255
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn parse_genesis_file() { | ||
| let path = "./test-data/genesis.json"; | ||
| parse_genesis(path).unwrap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Enhance test coverage
Consider:
- Adding test cases for invalid genesis files
- Documenting the required test data file structure
- Adding tests for other utility functions
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn parse_genesis_file() {
let path = "./test-data/genesis.json";
parse_genesis(path).unwrap();
}
+
+ #[test]
+ fn parse_genesis_file_not_found() {
+ assert!(parse_genesis("./non-existent.json").is_err());
+ }
+
+ #[test]
+ fn test_parse_block_hash_or_number() {
+ assert!(matches!(
+ parse_block_hash_or_number("0x1234").unwrap(),
+ BlockHashOrNumber::Hash(_)
+ ));
+ assert!(matches!(
+ parse_block_hash_or_number("1234").unwrap(),
+ BlockHashOrNumber::Num(1234)
+ ));
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| /// Configuration file | ||
| #[arg(long)] | ||
| config: Option<PathBuf>, |
There was a problem hiding this comment.
The config field should be public to ensure accessibility
Ohayo sensei!
I noticed that the config field in the NodeArgs struct is not declared as pub, unlike the other fields. This might lead to issues when trying to access it from outside the struct. Consider adding the pub keyword to make it public.
Apply this diff to fix the issue:
/// Configuration file
#[arg(long)]
- config: Option<PathBuf>,
+ pub config: Option<PathBuf>,📝 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.
| /// Configuration file | |
| #[arg(long)] | |
| config: Option<PathBuf>, | |
| /// Configuration file | |
| #[arg(long)] | |
| pub config: Option<PathBuf>, |
This PR moves the CLI args for Torii and Katana out of the binary, into dedicated crates.
Even if for now the
katana-cliis a bit big due to the configs being pulled out from the respective katana crates, it makes it easier for slot to re-use the argument for configuration without having to reimplement them.Summary by CodeRabbit
New Features
ServerOptionsstruct for HTTP-RPC server configuration.Bug Fixes
Documentation
Chores