feat(katana): new command line interface (breaking) #2663
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications to several configuration structures within the Katana CLI and related components. Key changes include the addition and restructuring of fields in the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (8)
crates/katana/node/src/config/metrics.rs (1)
12-15: Clean struct refactoring, sensei!The separation of address and port fields improves configuration flexibility and clarity.
Consider enhancing the field documentation with examples:
/// The address to bind the metrics server to. +/// Example: 127.0.0.1 pub addr: IpAddr, /// The port to bind the metrics server to. +/// Example: 9100 pub port: u16,bin/katana/src/utils.rs (2)
40-44: Add documentation for the LogFormat enum, sensei!While the enum is well-structured, it would benefit from documentation explaining:
- The purpose of the enum
- What each variant represents
- When to use each format
Add this documentation:
#[derive(Debug, Clone, PartialEq)] +/// Specifies the format for logging output +/// +/// # Variants +/// * `Json` - Structured JSON format for machine parsing +/// * `Full` - Human-readable format with full details pub enum LogFormat { Json, Full, }
59-66: Consider making the format strings more DRY, sensei!The string literals are duplicated between
ValueEnumandDisplayimplementations. Consider extracting them into constants.Here's a suggested improvement:
#[derive(Debug, Clone, PartialEq)] pub enum LogFormat { Json, Full, } +impl LogFormat { + const JSON_FORMAT: &'static str = "json"; + const FULL_FORMAT: &'static str = "full"; +} impl ValueEnum for LogFormat { fn value_variants<'a>() -> &'a [Self] { &[Self::Json, Self::Full] } fn to_possible_value(&self) -> Option<PossibleValue> { match self { - Self::Json => Some(PossibleValue::new("json")), - Self::Full => Some(PossibleValue::new("full")), + Self::Json => Some(PossibleValue::new(Self::JSON_FORMAT)), + Self::Full => Some(PossibleValue::new(Self::FULL_FORMAT)), } } } impl Display for LogFormat { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::Json => write!(f, "json"), - Self::Full => write!(f, "full"), + Self::Json => write!(f, "{}", Self::JSON_FORMAT), + Self::Full => write!(f, "{}", Self::FULL_FORMAT), } } }bin/katana/src/cli/node.rs (1)
375-384: Clarify API Enabling Logic in Development ModeOhayo, sensei! In the
rpc_configmethod, the logic for enabling APIs in development mode is sound. However, adding a comment might improve clarity for future contributors.Consider adding a comment:
fn rpc_config(&self) -> RpcConfig { let mut apis = HashSet::from([ApiKind::Starknet, ApiKind::Torii, ApiKind::Saya]); // Only enable `katana` API in dev mode + // This allows access to development-specific endpoints if self.development.dev { apis.insert(ApiKind::Dev); }crates/katana/node-bindings/src/lib.rs (4)
275-276: Inconsistent parameter naming between method and fieldOhayo, sensei! The parameter
rpc_urlin thel1_providermethod doesn't match the field namel1_provider. For consistency and clarity, consider renaming the parameter tol1_provider.Apply this diff to rename the parameter:
- pub fn l1_provider<T: Into<String>>(mut self, rpc_url: T) -> Self { - self.l1_provider = Some(rpc_url.into()); + pub fn l1_provider<T: Into<String>>(mut self, l1_provider: T) -> Self { + self.l1_provider = Some(l1_provider.into()); self }
184-185: Simplify metrics configuration by combining address and portOhayo, sensei! Since
SocketAddrincludes both the IP address and port, you might consider combiningmetrics_addrandmetrics_portinto a singlemetrics_addrfield of typeSocketAddr. This can reduce redundancy and potential mismatches between the address and port.
188-189: Unifyhttp_addrandhttp_portinto a singleSocketAddrOhayo, sensei! Similar to the metrics configuration, combining
http_addrandhttp_portinto a singlehttp_addrfield of typeSocketAddrcould streamline the server options and prevent inconsistencies.
305-309: Clarify documentation for metrics configurationOhayo, sensei! The method
metrics_addrsets both the enabling of metrics and the metrics server address. It might be helpful to update the documentation to reflect that metrics are enabled when this method is called.Consider revising the doc comment:
- /// Enables Prometheus metrics and sets the metrics server address. + /// Sets the metrics server address and enables Prometheus metrics.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
bin/katana/src/cli/node.rs(13 hunks)bin/katana/src/utils.rs(2 hunks)crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/node-bindings/src/lib.rs(6 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(3 hunks)crates/katana/runner/src/lib.rs(1 hunks)
🔇 Additional comments (10)
crates/katana/node/src/config/metrics.rs (3)
1-6: Ohayo! Clean and well-structured constants, sensei!
The default values are sensible choices - localhost for metrics address and port 9100 which follows common practices for metrics servers.
17-21: Excellent implementation of socket_addr, sensei!
The method provides a clean and efficient way to construct a SocketAddr from the separated fields, maintaining good compatibility with existing code.
1-21: Verify consistent usage across the codebase, sensei!
Let's ensure all callers have been updated to use the new separated address and port configuration.
✅ Verification successful
All clear on the metrics configuration front, sensei!
The verification shows that all usages of MetricsConfig have been properly updated to use the new separated address and port fields. Specifically:
- The only construction of
MetricsConfigoccurs inbin/katana/src/cli/node.rsand correctly uses the separatedaddrandportfields - The struct is properly imported and used as an optional config in the main node configuration
- No legacy socket address usage patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct SocketAddr usage in metrics configuration
# that might need updating to the new separate addr/port pattern
echo "Checking for potential missed updates in metrics configuration..."
# Look for SocketAddr usage in relation to metrics
rg -l "SocketAddr.*metrics" --type rust
# Look for direct metrics address assignment that might need updating
rg -l "metrics.*addr.*=.*:" --type rust
Length of output: 265
Script:
#!/bin/bash
# Let's check for any usage of MetricsConfig and its fields
echo "Checking MetricsConfig usage patterns..."
# Find all usages of MetricsConfig struct
rg "MetricsConfig" -A 5 --type rust
# Find where metrics configuration is being constructed
ast-grep --pattern 'MetricsConfig { $$$_ }'
# Check for any test files that might be using metrics configuration
fd "test.*\.rs$" --exec rg -l "metrics.*config" {}
Length of output: 3703
crates/katana/node/src/config/rpc.rs (1)
40-40: Default implementation maintains consistent behavior.
The default implementation correctly initializes cors_domain to None, maintaining the same behavior as the previous allowed_origins field.
bin/katana/src/utils.rs (2)
5-6: Ohayo! The new imports look good, sensei!
The added imports from clap are necessary for implementing the enum's functionality.
46-57: The ValueEnum implementation is perfect, sensei!
The implementation correctly provides CLI argument parsing support with appropriate lowercase values.
crates/dojo/test-utils/src/sequencer.rs (1)
121-121: Ohayo sensei! Verify the CORS domain configuration impact.
The change from allowed_origins to cors_domain represents a semantic shift in how CORS is configured. While the code change is straightforward, we should ensure this doesn't affect existing test scenarios.
Let's check for any test configurations that might be affected:
✅ Verification successful
Ohayo sensei! The CORS configuration change is verified and safe.
The change from allowed_origins to cors_domain in RpcConfig is part of a consistent pattern across the codebase. The search results show:
- The main
RpcConfigstruct incrates/katana/node/src/config/rpc.rsproperly definescors_domain: Option<Vec<String>> - The change in
test-utils/src/sequencer.rscorrectly aligns with this definition - The CORS handling logic in
katana/node/src/lib.rsproperly uses this field for setting up the CORS layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test files that might be using the old CORS configuration
# Test: Look for test files using allowed_origins configuration
rg -t rust "allowed_origins.*RpcConfig" "tests/"
Length of output: 95
Script:
#!/bin/bash
# Let's search in the entire codebase for both old and new CORS configurations
echo "Searching for old CORS config (allowed_origins)..."
rg -g "*.rs" "allowed_origins.*RpcConfig" -A 2 -B 2
echo -e "\nSearching for new CORS config (cors_domain)..."
rg -g "*.rs" "cors_domain.*RpcConfig" -A 2 -B 2
echo -e "\nLooking for RpcConfig struct definition..."
ast-grep --pattern 'struct RpcConfig {
$$$
}'
Length of output: 474
Script:
#!/bin/bash
# Let's try a different approach to find RpcConfig and CORS related code
echo "Searching for RpcConfig struct..."
rg -g "*.rs" "struct RpcConfig" -A 10 -B 2
echo -e "\nSearching for CORS related configurations..."
rg -g "*.rs" -e "cors" -e "allowed_origins" -A 2 -B 2
echo -e "\nLooking for test files that might use RpcConfig..."
fd -e rs -e toml . | grep -i "test"
Length of output: 12714
crates/katana/node/src/lib.rs (1)
Line range hint 110-121: Ohayo! Clean refactor of metrics server initialization, sensei!
The change to use socket_addr() method aligns well with the PR objective of separating metrics address and port. The code is now more maintainable and follows better encapsulation practices.
bin/katana/src/cli/node.rs (1)
14-14: Approved Import of IpAddr
Ohayo, sensei! The addition of use std::net::IpAddr; is appropriate for handling IP addresses in the server configurations.
crates/katana/node-bindings/src/lib.rs (1)
528-529: Validate CORS domain format
Ohayo, sensei! When setting the http_cors_domain, ensure that the provided allowed_origins string is properly formatted and validated. Improper CORS configurations can lead to security vulnerabilities.
You can run the following script to check for common issues in CORS domain settings:
✅ Verification successful
Let's analyze the actual implementation and any validation logic that might be in place. The previous grep pattern was too specific, looking only for literal string patterns. We need to understand the broader context of how CORS is handled.
CORS validation is properly implemented
Ohayo, sensei! After reviewing the codebase, I can confirm that the CORS domain validation is properly handled. Here's why:
- The implementation in
crates/katana/node/src/lib.rsshows proper CORS handling:- For wildcard (
*), it usesAllowOrigin::mirror_request()which is a secure pattern - For specific origins, they are properly parsed and validated through the tower-http CORS middleware
- For wildcard (
- The CLI accepts comma-separated domains via
--http.corsdomainflag with proper value delimiter configuration - The CORS configuration is integrated with a robust middleware stack that includes proper headers and method restrictions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the CORS domain contains unsafe wildcards.
# Test: Search for usage of `http_cors_domain` with unsafe patterns.
# Expect: No occurrences of '*' or 'null' as allowed origins.
fd --extension rs | xargs grep -n "http_cors_domain" | grep -E "'(\*|null)'"
Length of output: 82
Script:
#!/bin/bash
# Let's search for all occurrences of http_cors_domain to understand its usage
rg -n "http_cors_domain" -A 3 -B 3
# Also search for any CORS-related tests or validations
rg -n "cors" --type rust -A 3 -B 3
# Look for the struct definition containing http_cors_domain
ast-grep --pattern 'struct $_ {
$$$
http_cors_domain: $_,
$$$
}'
Length of output: 12070
| pub apis: HashSet<ApiKind>, | ||
| pub cors_domain: Option<Vec<String>>, |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! The field renaming needs to be synchronized across the codebase
The allowed_origins field is still being used in multiple locations:
bin/torii/src/main.rscrates/torii/server/src/proxy.rscrates/katana/node-bindings/src/lib.rs
This inconsistency could lead to confusion and potential bugs. The renaming should be applied consistently across all related components.
🔗 Analysis chain
Ohayo sensei! The field renaming improves semantic clarity.
The change from allowed_origins to cors_domain better reflects the field's purpose in handling Cross-Origin Resource Sharing (CORS) configurations.
Let's verify the impact of this rename:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to allowed_origins
# to ensure complete renaming across the codebase
echo "Checking for any remaining references to allowed_origins..."
rg "allowed_origins"
Length of output: 1405
| @@ -121,7 +121,7 @@ impl KatanaRunner { | |||
| .port(port) | |||
| .accounts(n_accounts) | |||
| .json_log(true) | |||
| .max_connections(10000) | |||
| .rpc_max_connections(10000) | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! We found an inconsistency in the API usage, sensei! 🔍
The method .max_connections() is still being used in crates/katana/node/src/lib.rs while it was renamed to .rpc_max_connections() in the runner. For consistency across the codebase, this should also be updated to use the new method name.
crates/katana/node/src/lib.rs: Update.max_connections(config.max_connections)to.rpc_max_connections(config.max_connections)
🔗 Analysis chain
Ohayo! The method rename improves API clarity, sensei! ✨
The change from .max_connections() to .rpc_max_connections() better specifies that this limit applies specifically to RPC connections, which aligns well with the PR's objective of improving configuration clarity.
Let's verify if there are any other occurrences of the old method name that need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old method name
rg "\.max_connections\(" --type rust
Length of output: 117
Script:
#!/bin/bash
# Let's check the context of the max_connections usage in the node lib
rg "max_connections" -B 2 -A 2 crates/katana/node/src/lib.rs
# Also check if there's a Config struct definition to understand the field
ast-grep --pattern 'struct Config {
$$$
max_connections: $_,
$$$
}'
Length of output: 320
| 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") | ||
| }) | ||
| .collect::<Vec<_>>(), | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Ohayo! Let's improve error handling in CORS configuration, sensei!
The current implementation has a few concerns:
- Double parsing: URI is parsed twice (lines 322 and 324)
- Use of
expect()could cause panics in production - Missing proper error handling for invalid URIs
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")
- })
+ .filter_map(|o| {
+ match o.parse::<Uri>() {
+ Ok(_) => o.parse().ok(),
+ Err(e) => {
+ tracing::warn!("Invalid URI in CORS configuration: {}", e);
+ None
+ }
+ }
+ })
.collect::<Vec<_>>(),
),
});This change:
- Eliminates double parsing
- Replaces
expect()with proper error handling - Logs invalid URIs instead of panicking
- Filters out invalid origins gracefully
📝 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_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") | |
| }) | |
| .collect::<Vec<_>>(), | |
| ), | |
| }); | |
| 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() | |
| .filter_map(|o| { | |
| match o.parse::<Uri>() { | |
| Ok(_) => o.parse().ok(), | |
| Err(e) => { | |
| tracing::warn!("Invalid URI in CORS configuration: {}", e); | |
| None | |
| } | |
| } | |
| }) | |
| .collect::<Vec<_>>(), | |
| ), | |
| }); |
bin/katana/src/cli/node.rs
Outdated
| /// Run in forking mode. | ||
| #[arg(long, requires = "l1_provider")] | ||
| pub fork: bool, | ||
|
|
||
| /// Fork the network at a specific block id, can either be a hash (0x-prefixed) or a block | ||
| /// number. | ||
| #[arg(long = "fork.block", value_name = "BLOCK", requires_all = ["fork", "l1_provider"])] | ||
| #[arg(value_parser = parse_block_hash_or_number)] | ||
| pub fork_block: Option<BlockHashOrNumber>, | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Found in L1 Provider Requirement
Ohayo, sensei! While the CLI arguments correctly enforce the l1_provider requirement through the requires attribute, there's a concerning comment in the code implementation suggesting that "L1 provider is optional for now." This creates an inconsistency between the CLI argument definition and the actual implementation's intent.
bin/katana/src/cli/node.rs: The comment "L1 provider is optional for now" contradicts the CLI'srequires = "l1_provider"constraint.- The code uses
expect("should be required by cli")which assumes the requirement, but the comment suggests otherwise.
🔗 Analysis chain
Ensure l1_provider Requirement for Forking
Ohayo, sensei! The fork flag correctly requires l1_provider in the ForkingOptions. This ensures that forking mode has the necessary L1 provider URL.
To confirm that l1_provider is consistently required when fork is true, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `l1_provider` is required when `fork` is enabled.
# Check CLI argument definitions
rg '#\[arg\(.*requires\s*=\s*"l1_provider"\)' -A 2 bin/katana/src/cli/node.rs
# Verify usage in code
rg 'if self\.forking\.fork' -A 5 bin/katana/src/cli/node.rs
Length of output: 572
bin/katana/src/cli/node.rs
Outdated
| /// The L1 ETH gas price. (denominated in wei) | ||
| #[arg(requires_all = ["l1_strk_gas_price"])] | ||
| #[arg(long = "gpo.l1-eth-gas-price", value_name = "WEI")] | ||
| pub l1_eth_gas_price: Option<u128>, | ||
|
|
||
| #[arg(long = "l1-strk-gas-price", value_name = "FRI")] | ||
| #[arg(requires = "l1_eth_data_gas_price")] | ||
| #[arg(help = "The L1 STRK gas price. (denominated in fri)")] | ||
| /// The L1 STRK gas price. (denominated in fri) | ||
| #[arg(requires_all = ["l1_eth_data_gas_price"])] | ||
| #[arg(long = "gpo.l1-strk-gas-price", value_name = "FRI")] | ||
| pub l1_strk_gas_price: Option<u128>, | ||
|
|
||
| #[arg(long = "l1-eth-data-gas-price", value_name = "WEI")] | ||
| #[arg(requires = "l1_strk_data_gas_price")] | ||
| #[arg(help = "The L1 ETH gas price. (denominated in wei)")] | ||
| /// The L1 ETH data gas price. (denominated in wei) | ||
| #[arg(requires_all = ["l1_strk_data_gas_price"])] | ||
| #[arg(long = "gpo.l1-eth-data-gas-price", value_name = "WEI")] | ||
| pub l1_eth_data_gas_price: Option<u128>, | ||
|
|
||
| #[arg(long = "l1-strk-data-gas-price", value_name = "FRI")] | ||
| #[arg(requires = "l1_eth_gas_price")] | ||
| #[arg(help = "The L1 STRK gas prick. (denominated in fri)")] | ||
| /// The L1 STRK data gas price. (denominated in fri) | ||
| #[arg(requires_all = ["l1_eth_gas_price"])] | ||
| #[arg(long = "gpo.l1-strk-data-gas-price", value_name = "FRI")] |
There was a problem hiding this comment.
Potential Inconsistencies in requires_all Attributes
Ohayo, sensei! There seem to be inconsistencies in the requires_all attributes within the GasPriceOracleOptions struct. The mutual dependencies might not reflect the intended configuration requirements.
To ensure all related gas price parameters are set together, consider adjusting the requires_all attributes:
#[derive(Debug, Args, Clone)]
#[command(next_help_heading = "Gas Price Oracle Options")]
pub struct GasPriceOracleOptions {
/// The L1 ETH gas price. (denominated in wei)
- #[arg(requires_all = ["l1_strk_gas_price"])]
+ #[arg(requires_all = ["l1_strk_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])]
#[arg(long = "gpo.l1-eth-gas-price", value_name = "WEI")]
pub l1_eth_gas_price: Option<u128>,
/// The L1 STRK gas price. (denominated in fri)
- #[arg(requires_all = ["l1_eth_data_gas_price"])]
+ #[arg(requires_all = ["l1_eth_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])]
#[arg(long = "gpo.l1-strk-gas-price", value_name = "FRI")]
pub l1_strk_gas_price: Option<u128>,
/// The L1 ETH data gas price. (denominated in wei)
- #[arg(requires_all = ["l1_strk_data_gas_price"])]
+ #[arg(requires_all = ["l1_eth_gas_price", "l1_strk_gas_price", "l1_strk_data_gas_price"])]
#[arg(long = "gpo.l1-eth-data-gas-price", value_name = "WEI")]
pub l1_eth_data_gas_price: Option<u128>,
/// The L1 STRK data gas price. (denominated in fri)
- #[arg(requires_all = ["l1_eth_gas_price"])]
+ #[arg(requires_all = ["l1_eth_gas_price", "l1_strk_gas_price", "l1_eth_data_gas_price"])]
#[arg(long = "gpo.l1-strk-data-gas-price", value_name = "FRI")]
pub l1_strk_data_gas_price: Option<u128>,
}📝 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.
| /// The L1 ETH gas price. (denominated in wei) | |
| #[arg(requires_all = ["l1_strk_gas_price"])] | |
| #[arg(long = "gpo.l1-eth-gas-price", value_name = "WEI")] | |
| pub l1_eth_gas_price: Option<u128>, | |
| #[arg(long = "l1-strk-gas-price", value_name = "FRI")] | |
| #[arg(requires = "l1_eth_data_gas_price")] | |
| #[arg(help = "The L1 STRK gas price. (denominated in fri)")] | |
| /// The L1 STRK gas price. (denominated in fri) | |
| #[arg(requires_all = ["l1_eth_data_gas_price"])] | |
| #[arg(long = "gpo.l1-strk-gas-price", value_name = "FRI")] | |
| pub l1_strk_gas_price: Option<u128>, | |
| #[arg(long = "l1-eth-data-gas-price", value_name = "WEI")] | |
| #[arg(requires = "l1_strk_data_gas_price")] | |
| #[arg(help = "The L1 ETH gas price. (denominated in wei)")] | |
| /// The L1 ETH data gas price. (denominated in wei) | |
| #[arg(requires_all = ["l1_strk_data_gas_price"])] | |
| #[arg(long = "gpo.l1-eth-data-gas-price", value_name = "WEI")] | |
| pub l1_eth_data_gas_price: Option<u128>, | |
| #[arg(long = "l1-strk-data-gas-price", value_name = "FRI")] | |
| #[arg(requires = "l1_eth_gas_price")] | |
| #[arg(help = "The L1 STRK gas prick. (denominated in fri)")] | |
| /// The L1 STRK data gas price. (denominated in fri) | |
| #[arg(requires_all = ["l1_eth_gas_price"])] | |
| #[arg(long = "gpo.l1-strk-data-gas-price", value_name = "FRI")] | |
| /// The L1 ETH gas price. (denominated in wei) | |
| #[arg(requires_all = ["l1_strk_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])] | |
| #[arg(long = "gpo.l1-eth-gas-price", value_name = "WEI")] | |
| pub l1_eth_gas_price: Option<u128>, | |
| /// The L1 STRK gas price. (denominated in fri) | |
| #[arg(requires_all = ["l1_eth_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])] | |
| #[arg(long = "gpo.l1-strk-gas-price", value_name = "FRI")] | |
| pub l1_strk_gas_price: Option<u128>, | |
| /// The L1 ETH data gas price. (denominated in wei) | |
| #[arg(requires_all = ["l1_eth_gas_price", "l1_strk_gas_price", "l1_strk_data_gas_price"])] | |
| #[arg(long = "gpo.l1-eth-data-gas-price", value_name = "WEI")] | |
| pub l1_eth_data_gas_price: Option<u128>, | |
| /// The L1 STRK data gas price. (denominated in fri) | |
| #[arg(requires_all = ["l1_eth_gas_price", "l1_strk_gas_price", "l1_eth_data_gas_price"])] | |
| #[arg(long = "gpo.l1-strk-data-gas-price", value_name = "FRI")] | |
| pub l1_strk_data_gas_price: Option<u128>, |
| if self.disable_validate { | ||
| if !is_dev { | ||
| cmd.arg("--dev"); | ||
| } | ||
|
|
||
| cmd.arg("--dev.no-account-validation"); |
There was a problem hiding this comment.
Ensure consistent setting of is_dev flag when adding --dev
Ohayo, sensei! In the disable_validate block, after adding --dev, the is_dev flag isn't set to true, unlike in other similar blocks. This could lead to --dev being added multiple times if multiple dev options are enabled. For consistency and to avoid potential issues, consider setting is_dev = true after adding --dev.
Apply this diff to fix the inconsistency:
if self.disable_validate {
if !is_dev {
cmd.arg("--dev");
+ is_dev = true;
}
cmd.arg("--dev.no-account-validation");
}📝 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.
| if self.disable_validate { | |
| if !is_dev { | |
| cmd.arg("--dev"); | |
| } | |
| cmd.arg("--dev.no-account-validation"); | |
| if self.disable_validate { | |
| if !is_dev { | |
| cmd.arg("--dev"); | |
| is_dev = true; | |
| } | |
| cmd.arg("--dev.no-account-validation"); |
| if let Some(addr) = self.metrics_addr { | ||
| if !metrics_enabled { | ||
| cmd.arg("--metrics"); | ||
| metrics_enabled = true; | ||
| } | ||
|
|
||
| cmd.arg("--metrics.addr").arg(addr.to_string()); |
There was a problem hiding this comment.
Avoid potential multiple --metrics flags
Ohayo, sensei! In the metrics_addr and metrics_port blocks, you're checking if !metrics_enabled before adding --metrics, but you don't set metrics_enabled = true after adding --metrics in the metrics_port block. This could lead to --metrics being added multiple times. For consistency, consider setting metrics_enabled = true after adding --metrics.
Apply this diff to fix the inconsistency:
if let Some(port) = self.metrics_port {
if !metrics_enabled {
cmd.arg("--metrics");
+ metrics_enabled = true;
}
cmd.arg("--metrics.port").arg(port.to_string());
}Committable suggestion skipped: line range outside the PR's diff.
kariy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
bin/katana/src/cli/node.rs (1)
459-464: Consider improving error handling for L1 provider URL.Ohayo, sensei! While the code uses
expect()for the L1 provider URL, it might be better to handle this more gracefully.Consider this alternative approach:
- let url = self.l1_provider.as_ref().expect("should be required by cli"); + let url = self.l1_provider.as_ref().ok_or_else(|| { + anyhow::anyhow!("L1 provider URL is required when forking is enabled") + })?;crates/katana/node-bindings/src/lib.rs (1)
428-431: Consider adding port validation.Ohayo, sensei! While port 0 is handled for auto-assignment, consider adding validation for other port numbers to ensure they're within valid range (1-65535).
Consider adding validation like this:
+ if let Some(port) = self.http_port { + if port > 0 && port < 65536 { + cmd.arg("--http.port").arg(port.to_string()); + } else if port != 0 { + return Err(Error::InvalidPort(port)); + } + } - let mut port = self.http_port.unwrap_or(0); - cmd.arg("--http.port").arg(port.to_string());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/ci.yml(1 hunks)bin/katana/src/cli/node.rs(13 hunks)crates/katana/node-bindings/src/lib.rs(6 hunks)
🔇 Additional comments (9)
bin/katana/src/cli/node.rs (6)
122-142: LGTM! Well-structured metrics configuration.
Ohayo, sensei! The new MetricsOptions struct is well-organized with clear documentation and proper CLI argument requirements. The default values are appropriately set using constants.
201-230: LGTM! Comprehensive development options.
Ohayo, sensei! The DevOptions struct provides a good set of development-specific configurations with proper documentation and dependencies between flags.
475-479: LGTM! Clean metrics configuration handling.
Ohayo, sensei! The metrics configuration is well-implemented with proper optional handling.
232-244: Verify L1 provider requirement consistency.
Ohayo, sensei! There's a potential inconsistency between the code comment and implementation regarding L1 provider requirement.
#!/bin/bash
# Description: Check L1 provider usage consistency
# Look for places where L1 provider is used
rg "l1_provider" -A 3329-344:
Fix potential dangling reference in logging configuration.
Ohayo, sensei! The logging configuration has a potential issue with a dangling reference when using a temporary string.
Apply this diff to fix the issue:
- 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()
+ };Likely invalid or redundant comment.
147-165: Consider adding validation for CORS domains.
Ohayo, sensei! While the HTTP server configuration looks good, the CORS domain list could benefit from validation to ensure proper formatting and security.
✅ Verification successful
CORS validation is already properly implemented, no changes needed
Ohayo, sensei! After reviewing the codebase, I can confirm that CORS validation is already well-implemented:
- The CORS domains are validated as valid URIs during server setup (
o.parse::<Uri>().expect("Invalid URI")) - Special handling for wildcard (
*) is implemented correctly - The implementation includes proper security measures:
- Specific allowed headers
- Limited HTTP methods (POST, GET only)
- Proper origin validation
The current implementation in katana/node/src/lib.rs follows security best practices and doesn't need additional validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in CORS configuration
# Look for any wildcard CORS settings that might be too permissive
rg -A 2 "http_cors_domain.*\*"
Length of output: 3986
Script:
#!/bin/bash
# Let's check for CORS validation and implementation
# First, let's look for CORS-related code
rg -A 5 "cors" --ignore-case
# Also check for any HTTP server setup code that might handle CORS
ast-grep --pattern 'fn $_($$cors$$) { $$$ }'
Length of output: 8767
crates/katana/node-bindings/src/lib.rs (3)
179-191: LGTM! Field restructuring improves clarity.
The restructuring of configuration fields into logical groups (L1, metrics, server options) enhances code organization and maintainability.
524-530: LGTM! Good error handling for RPC and CORS configurations.
The implementation properly handles optional RPC max connections and CORS domain settings.
484-489:
Fix inconsistent dev mode flag handling.
Ohayo, sensei! The disable_validate block doesn't set is_dev = true after adding --dev, unlike other similar blocks. This could lead to --dev being added multiple times.
Apply this diff to fix the inconsistency:
if self.disable_validate {
if !is_dev {
cmd.arg("--dev");
+ is_dev = true;
}
cmd.arg("--dev.no-account-validation");
}Likely invalid or redundant comment.
| if let Some(port) = self.metrics_port { | ||
| if !metrics_enabled { | ||
| cmd.arg("--metrics"); | ||
| } | ||
|
|
||
| cmd.arg("--metrics.port").arg(port.to_string()); |
There was a problem hiding this comment.
Fix potential duplicate metrics flag.
Ohayo, sensei! The metrics_port block doesn't set metrics_enabled = true after adding --metrics, which could lead to duplicate flags.
Apply this diff to fix the inconsistency:
if let Some(port) = self.metrics_port {
if !metrics_enabled {
cmd.arg("--metrics");
+ metrics_enabled = true;
}
cmd.arg("--metrics.port").arg(port.to_string());
}📝 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.
| if let Some(port) = self.metrics_port { | |
| if !metrics_enabled { | |
| cmd.arg("--metrics"); | |
| } | |
| cmd.arg("--metrics.port").arg(port.to_string()); | |
| if let Some(port) = self.metrics_port { | |
| if !metrics_enabled { | |
| cmd.arg("--metrics"); | |
| metrics_enabled = true; | |
| } | |
| cmd.arg("--metrics.port").arg(port.to_string()); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bin/katana/src/cli/node.rs (1)
454-457: Improve error message for fork provider URLOhayo, sensei! While the expect() is safe due to CLI validation, the error message could be more descriptive for debugging purposes.
Consider updating the error message:
- let url = self.forking.fork_provider.as_ref().expect("should be required by cli"); + let url = self.forking.fork_provider.as_ref().expect("fork provider URL is required when --fork is enabled");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)bin/katana/src/cli/node.rs(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
🔇 Additional comments (3)
bin/katana/src/cli/node.rs (3)
324-331: Previous review comment is still applicable
The dangling reference issue in the logging configuration needs to be addressed as mentioned in the previous review.
113-133: LGTM! Well-structured metrics configuration
Ohayo, sensei! The metrics configuration is well-organized with proper dependency handling between flags and clear documentation.
253-270:
Fix circular dependencies in gas price oracle options
Ohayo, sensei! The current requires_all configuration creates a circular chain of dependencies instead of ensuring all gas prices are set together. This could lead to unexpected behavior where not all required values are provided.
Apply this fix to ensure all gas price options are set together:
/// The L1 ETH gas price. (denominated in wei)
- #[arg(requires_all = ["l1_strk_gas_price"])]
+ #[arg(requires_all = ["l1_strk_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])]
#[arg(long = "gpo.l1-eth-gas-price", value_name = "WEI")]
pub l1_eth_gas_price: Option<u128>,
/// The L1 STRK gas price. (denominated in fri)
- #[arg(requires_all = ["l1_eth_data_gas_price"])]
+ #[arg(requires_all = ["l1_eth_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])]
#[arg(long = "gpo.l1-strk-gas-price", value_name = "FRI")]
pub l1_strk_gas_price: Option<u128>,
/// The L1 ETH data gas price. (denominated in wei)
- #[arg(requires_all = ["l1_strk_data_gas_price"])]
+ #[arg(requires_all = ["l1_eth_gas_price", "l1_strk_gas_price", "l1_strk_data_gas_price"])]
#[arg(long = "gpo.l1-eth-data-gas-price", value_name = "WEI")]
pub l1_eth_data_gas_price: Option<u128>,
/// The L1 STRK data gas price. (denominated in fri)
- #[arg(requires_all = ["l1_eth_gas_price"])]
+ #[arg(requires_all = ["l1_eth_gas_price", "l1_strk_gas_price", "l1_eth_data_gas_price"])]
#[arg(long = "gpo.l1-strk-data-gas-price", value_name = "FRI")]
pub l1_strk_data_gas_price: Option<u128>,Likely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2663 +/- ##
==========================================
- Coverage 57.49% 57.36% -0.13%
==========================================
Files 400 402 +2
Lines 50276 50497 +221
==========================================
+ Hits 28905 28970 +65
- Misses 21371 21527 +156 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/katana/node/src/config/dev.rs (1)
43-53: Consider adding documentation for the Default implementation, sensei!While the implementation is clean and well-structured, adding documentation would help users understand the default values being used.
Here's a suggested addition:
+/// Provides default gas prices for development environments. +/// Uses predefined constants for both ETH and STRK chains: +/// - Default gas prices: ETH_L1_GAS_PRICE and STRK_L1_GAS_PRICE +/// - Default data gas prices: ETH_L1_DATA_GAS_PRICE and STRK_L1_DATA_GAS_PRICE impl std::default::Default for FixedL1GasPriceConfig {bin/katana/src/cli/node.rs (1)
642-697: Consider adding edge case tests for gas prices.Ohayo, sensei! While the current test coverage is good, consider adding tests for:
- Zero gas prices
- Maximum possible gas prices
- Invalid gas price combinations
Here's a suggested test case:
#[test] fn test_gas_price_edge_cases() { // Test zero gas prices let config = NodeArgs::parse_from([ "katana", "--gpo.l1-eth-gas-price", "0", "--gpo.l1-strk-gas-price", "0" ]).config().unwrap(); assert_matches!(config.dev.fixed_gas_prices, Some(prices) => { assert_eq!(prices.gas_price.eth, 0); assert_eq!(prices.gas_price.strk, 0); }); // Test maximum gas prices let config = NodeArgs::parse_from([ "katana", "--gpo.l1-eth-gas-price", "340282366920938463463374607431768211455", // u128::MAX "--gpo.l1-strk-gas-price", "340282366920938463463374607431768211455" ]).config().unwrap(); assert_matches!(config.dev.fixed_gas_prices, Some(prices) => { assert_eq!(prices.gas_price.eth, u128::MAX); assert_eq!(prices.gas_price.strk, u128::MAX); }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/katana/src/cli/node.rs(12 hunks)crates/katana/node/src/config/dev.rs(2 hunks)
🔇 Additional comments (5)
crates/katana/node/src/config/dev.rs (2)
1-4: Ohayo! Clean and well-organized imports, sensei!
The imports clearly separate the different types of gas prices (ETH/STRK and regular/data), making the code's intent obvious.
43-53: Verify CLI integration of default values.
Let's ensure these default values are properly integrated with the new CLI structure.
✅ Verification successful
Ohayo sensei! Based on the search results, I can now provide a definitive response about the default gas price configuration integration.
Default gas prices are properly integrated in the CLI and node architecture
The verification shows that:
- Default values from
FixedL1GasPriceConfigare correctly used in the node's gas oracle setup (crates/katana/node/src/lib.rs) - The gas prices flow properly through the block production pipeline and executor
- The CLI supports both default values and custom gas price configuration through the dev config
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of FixedL1GasPriceConfig::default() in CLI code
rg -l "FixedL1GasPriceConfig::default" "crates/katana/bin"
# Search for CLI argument parsing related to gas prices
ast-grep --pattern 'fn $_(gas_price|data_gas_price)($$$) {
$$$
}'
Length of output: 184
Script:
#!/bin/bash
# Let's try searching in the entire crates directory for CLI-related files
fd -e rs . crates/katana
# Search for usage of FixedL1GasPriceConfig::default
rg "FixedL1GasPriceConfig::default" crates/katana
# Search for gas price related CLI arguments
rg -A 5 "gas.?price" crates/katana
Length of output: 68862
bin/katana/src/cli/node.rs (3)
319-326: Fix dangling reference in logging configuration.
Ohayo, sensei! The dangling reference issue in the logging configuration is still present.
467-471: LGTM: Clean metrics configuration implementation.
Ohayo, sensei! The metrics configuration implementation is clean and follows good separation of concerns.
454-459: Verify URL scheme in forking configuration.
Ohayo, sensei! Consider validating the URL scheme to ensure it's using a supported protocol (http/https).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
crates/katana/node/src/config/dev.rs (1)
Line range hint
8-41: Consider enhancing DevConfig documentation.The
DevConfigstruct's documentation could be improved by adding details about the new gas price configuration options, particularly explaining when and why one might want to usefixed_gas_prices.Here's a suggested documentation enhancement:
/// Development configuration. #[derive(Debug, Clone)] pub struct DevConfig { + /// Configuration options for development mode that modify the node's behavior + /// for testing and development purposes. + /// + /// # Gas Price Configuration + /// When `fixed_gas_prices` is set, the node will use these constant values + /// instead of querying the gas price oracle. This is particularly useful + /// for testing and development environments where predictable gas prices + /// are desired. // ... rest of the struct }bin/katana/src/cli/node.rs (2)
251-267: Consider adding mutual dependencies between gas price options.Ohayo, sensei! The gas price options are related and should typically be set together for consistency. Consider adding
requires_allattributes to ensure all gas prices are configured as a group.pub struct GasPriceOracleOptions { /// The L1 ETH gas price. (denominated in wei) + #[arg(requires_all = ["l1_strk_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])] #[arg(long = "gpo.l1-eth-gas-price", value_name = "WEI")] pub l1_eth_gas_price: Option<u128>, /// The L1 STRK gas price. (denominated in fri) + #[arg(requires_all = ["l1_eth_gas_price", "l1_eth_data_gas_price", "l1_strk_data_gas_price"])] #[arg(long = "gpo.l1-strk-gas-price", value_name = "FRI")] pub l1_strk_gas_price: Option<u128>,
Line range hint
113-471: Improve test coverage for new configuration options.Ohayo, sensei! Several new configuration options and code paths lack test coverage, particularly:
- Metrics configuration
- Logging format options
- Development mode options
- Forking configuration
Would you like me to help generate additional test cases to improve coverage?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-120: bin/katana/src/cli/node.rs#L120
Added line #L120 was not covered by tests
[warning] 126-126: bin/katana/src/cli/node.rs#L126
Added line #L126 was not covered by tests
[warning] 132-132: bin/katana/src/cli/node.rs#L132
Added line #L132 was not covered by tests
[warning] 141-141: bin/katana/src/cli/node.rs#L141
Added line #L141 was not covered by tests
[warning] 146-146: bin/katana/src/cli/node.rs#L146
Added line #L146 was not covered by tests
[warning] 151-151: bin/katana/src/cli/node.rs#L151
Added line #L151 was not covered by tests
[warning] 197-197: bin/katana/src/cli/node.rs#L197
Added line #L197 was not covered by tests
[warning] 202-202: bin/katana/src/cli/node.rs#L202
Added line #L202 was not covered by tests
[warning] 208-208: bin/katana/src/cli/node.rs#L208
Added line #L208 was not covered by tests
[warning] 213-213: bin/katana/src/cli/node.rs#L213
Added line #L213 was not covered by tests
[warning] 220-220: bin/katana/src/cli/node.rs#L220
Added line #L220 was not covered by tests
[warning] 246-246: bin/katana/src/cli/node.rs#L246
Added line #L246 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/katana/src/cli/node.rs(12 hunks)crates/katana/node/src/config/dev.rs(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
bin/katana/src/cli/node.rs
[warning] 120-120: bin/katana/src/cli/node.rs#L120
Added line #L120 was not covered by tests
[warning] 126-126: bin/katana/src/cli/node.rs#L126
Added line #L126 was not covered by tests
[warning] 132-132: bin/katana/src/cli/node.rs#L132
Added line #L132 was not covered by tests
[warning] 141-141: bin/katana/src/cli/node.rs#L141
Added line #L141 was not covered by tests
[warning] 146-146: bin/katana/src/cli/node.rs#L146
Added line #L146 was not covered by tests
[warning] 151-151: bin/katana/src/cli/node.rs#L151
Added line #L151 was not covered by tests
[warning] 197-197: bin/katana/src/cli/node.rs#L197
Added line #L197 was not covered by tests
[warning] 202-202: bin/katana/src/cli/node.rs#L202
Added line #L202 was not covered by tests
[warning] 208-208: bin/katana/src/cli/node.rs#L208
Added line #L208 was not covered by tests
[warning] 213-213: bin/katana/src/cli/node.rs#L213
Added line #L213 was not covered by tests
[warning] 220-220: bin/katana/src/cli/node.rs#L220
Added line #L220 was not covered by tests
[warning] 246-246: bin/katana/src/cli/node.rs#L246
Added line #L246 was not covered by tests
[warning] 322-323: bin/katana/src/cli/node.rs#L322-L323
Added lines #L322 - L323 were not covered by tests
[warning] 325-325: bin/katana/src/cli/node.rs#L325
Added line #L325 was not covered by tests
[warning] 333-334: bin/katana/src/cli/node.rs#L333-L334
Added lines #L333 - L334 were not covered by tests
[warning] 336-338: bin/katana/src/cli/node.rs#L336-L338
Added lines #L336 - L338 were not covered by tests
[warning] 455-456: bin/katana/src/cli/node.rs#L455-L456
Added lines #L455 - L456 were not covered by tests
[warning] 468-468: bin/katana/src/cli/node.rs#L468
Added line #L468 was not covered by tests
[warning] 478-478: bin/katana/src/cli/node.rs#L478
Added line #L478 was not covered by tests
[warning] 480-480: bin/katana/src/cli/node.rs#L480
Added line #L480 was not covered by tests
🔇 Additional comments (4)
crates/katana/node/src/config/dev.rs (2)
1-4: LGTM! Clean import organization, sensei!
The new imports for default gas prices are well-organized and follow a clear naming convention.
43-53: Ohayo! Clean Default implementation, but let's verify the constants.
The implementation is well-structured and follows Rust best practices. However, since this is part of a breaking change, we should verify the default gas price values.
Let's verify the default values across the codebase:
✅ Verification successful
Ohayo! Default gas price implementation is verified and safe, sensei!
The implementation correctly uses the constants defined in crates/katana/core/src/constants.rs:
- ETH and STRK L1 gas prices are both set to 20 Gwei (20 * 10^9)
- ETH and STRK L1 data gas prices are both set to 1 Mwei (10^6)
The values are consistently used across the codebase, with proper test coverage in bin/katana/src/cli/node.rs. No migration or upgrade files are impacted, and the only other gas price related constants are builder methods in node-bindings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of default gas price constants
# Search for any other usage or definition of these constants
echo "Checking usage of default gas price constants..."
rg -A 2 "DEFAULT_(ETH|STRK)_L1_(DATA_)?GAS_PRICE"
# Look for any tests that might need updating with these values
echo "Checking for related tests..."
rg -l "gas.?price" $(fd -t f "test.*\.rs$")
Length of output: 5757
Script:
#!/bin/bash
# Let's verify the actual values of these constants and their definitions
# First, let's get the constant definitions
echo "=== Checking constant definitions ==="
rg -B 2 -A 2 "pub const DEFAULT_(ETH|STRK)_L1_(DATA_)?GAS_PRICE" crates/katana/core/src/constants.rs
# Now let's check if there are any other gas price related constants that might need coordination
echo -e "\n=== Checking other gas price related constants ==="
rg "pub const.*gas.*price" -i
# Finally, let's check for any migration or upgrade related files that might need these values
echo -e "\n=== Checking migration/upgrade related files ==="
fd -t f "(migrat|upgrad).*\.(rs|toml)$" -x rg -l "gas.?price" {}
Length of output: 1695
bin/katana/src/cli/node.rs (2)
113-133: LGTM! Well-structured metrics configuration.
The MetricsOptions struct is well-documented and properly implements the requires attribute for dependent fields.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-120: bin/katana/src/cli/node.rs#L120
Added line #L120 was not covered by tests
[warning] 126-126: bin/katana/src/cli/node.rs#L126
Added line #L126 was not covered by tests
[warning] 132-132: bin/katana/src/cli/node.rs#L132
Added line #L132 was not covered by tests
408-428: LGTM! Well-implemented gas price configuration.
The gas price configuration is well-structured with proper handling of optional values and defaults. The implementation is thoroughly tested.
| http_addr: Option<SocketAddr>, | ||
| http_port: Option<u16>, | ||
| rpc_max_connections: Option<u64>, | ||
| http_cors_domain: Option<String>, |
There was a problem hiding this comment.
| http_cors_domain: Option<String>, | |
| http_cors_domains: Option<String>, |
can't the input contains several values separated by commas? If yes, wdyt about keeping it plural?
There was a problem hiding this comment.
I would go with cors_origins actually, wdyt?
There was a problem hiding this comment.
yeah definitely remove the http_ so that it's not confused with the http listen address/port
There was a problem hiding this comment.
At least on the CLI arg (ie --http.corsdomain), I prefixed it with http to indicate that the cors domains are only applied to the http server as we may introduce different transports (eg ws) and the idea is to allow configuring cors for each server.
Im a bit premature optimizing for this because its on the cli side to make sure we dont introduce these sorta changes too much. But in the code snippet above, i dont mind renaming it to what you guys suggested. Just want to make sure people are aware of my intention.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
bin/katana/src/cli/node.rs (1)
191-220: Consider adding validation for development seed.Ohayo, sensei! While the development options are well-structured, the seed string lacks validation. Consider adding a parser to ensure it's a valid seed format.
#[arg(requires = "dev")] #[arg(long = "dev.seed", default_value = "0")] +#[arg(value_parser = parse_dev_seed)] pub seed: String,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/katana/src/cli/node.rs(12 hunks)
🔇 Additional comments (4)
bin/katana/src/cli/node.rs (4)
112-132: LGTM! Well-structured metrics configuration.
Ohayo, sensei! The new metrics configuration is well-organized with clear separation of concerns. The conditional enabling of the metrics server is a good design choice.
134-156: LGTM! Clear server configuration structure.
The server options have been nicely restructured with descriptive field names and appropriate defaults. The separation of HTTP and RPC concerns is clean.
407-427: LGTM! Clean gas price configuration handling.
The gas price configuration is well-implemented with proper default handling and clear separation between ETH and STRK prices.
641-695: LGTM! Comprehensive test coverage for gas prices.
Excellent test coverage for the gas price configurations. The tests verify both individual and combined parameter settings.
new/rename katana cli args
Motivation
standardize the cli arguments names, categorized the arguments based on their scope, and makes it more flexible to add new arguments later.
Changes
Removed
--json-logand introduce a more generic argument,--log.format, with possible values: json, full. Doing--log.format jsonis the equivalent of--json-log.Currently,
--metricsis used to specify the address of the metrics server, but now we split the argument into 3 parts:--metrics: To enable metrics collection. Right now, whether you specify the flag or not, metrics will still be collected. This is a required flag if you want to start the metrics server.--metrics.addr: Metrics server address.--metrics.port: Metrics server port.--http.addr: RPC HTTP server address. Replacing--host.--http.port: RPC HTTP server port. Replacing--port.--http.corsdomain: RPC HTTP server allowed cors domain. Replacing--allowed-origins.--rpc.max-connections: Maximum number of concurrent connections allowed for all servers.--dev: Now this flag must be enabled to use any of the flags below.--dev.seed: Replacing--seed.--dev.accounts: Replacing--accounts.--dev.no-fee: Replacing--disable-fee.--dev.no-account-validation: Replacing--disable-validation.--gpo.l1-eth-gas-price: Replacing--l1-eth-gas-price.--gpo.l1-strk-gas-price: Replacing--l1-str-gas-price.--gpo.l1-eth-data-gas-price: Replacing--l1-eth-data-gas-price.--gpo.l1-strk-data-gas-price: Replacing--l1-strk-data-gas-price.--fork.provider: Replacing--rpc-url.Preview
Summary by CodeRabbit
Release Notes
New Features
LogFormatenum.Bug Fixes
cors_domainfor improved clarity.Chores
katanabinary.These changes enhance the overall functionality and clarity of the Katana CLI, improving user experience and configuration management.