feat(katana): gas oracle skeleton #2643
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications to the gas price configuration within the Katana project. Key changes include making several gas price fields optional, implementing a new gas oracle structure, and updating the development configuration to accommodate fixed gas prices. The Changes
Possibly related issues
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: 2
🧹 Outside diff range and nitpick comments (11)
crates/katana/core/src/backend/gas_oracle.rs (3)
1-4: Ohayo! Consider enhancing the documentation for future implementation.While the TODO comments clearly indicate this is a temporary implementation, it would be helpful to add more detailed documentation about:
- The expected behavior of the "proper gas oracle function"
- The sampling mechanism to be implemented
- Any specific requirements or constraints for the future implementation
5-9: Consider adding more derive macros, sensei!The struct could benefit from additional common derive macros for better utility:
-#[derive(Debug)] +#[derive(Debug, Clone, PartialEq)] pub struct L1GasOracle {
1-25: Ohayo! The gas oracle skeleton looks solid!The implementation provides a clean foundation for the future gas oracle functionality. It successfully addresses the immediate need while maintaining flexibility for future enhancements. The structure aligns well with the PR objectives of implementing a gas oracle skeleton.
For the future implementation, consider:
- Adding events/hooks for gas price updates
- Implementing caching mechanisms for gas price sampling
- Adding configuration for sampling intervals
crates/katana/node/src/config/dev.rs (2)
26-29: Documentation enhancement suggestion, sensei!The documentation is good, but could be even more helpful with an example of when this configuration would be used.
Consider adding an example like this:
/// Fixed L1 gas prices for development. /// /// These are the prices that will be used for calculating the gas fee for transactions. +/// +/// For example, this can be used to override the default gas prices during development +/// or testing scenarios where specific gas price values are needed.
32-36: Consider adding field-level documentation, sensei!While the struct is well-designed, adding documentation for individual fields would improve clarity.
Consider adding field documentation:
/// Fixed gas prices for development. #[derive(Debug, Clone)] pub struct FixedL1GasPriceConfig { + /// The base gas price used for transaction execution pub gas_price: GasPrices, + /// The gas price used for data operations pub data_gas_price: GasPrices, }crates/dojo/test-utils/src/sequencer.rs (1)
116-116: Ohayo! The gas oracle skeleton implementation looks good, sensei!The addition of
fixed_gas_prices: Noneto DevConfig aligns well with the gas oracle implementation goals. This change provides the foundation for future gas price configurations while maintaining current behavior in tests.Consider adding a comment explaining the significance of
fixed_gas_prices:- let dev = DevConfig { fee: false, account_validation: true, fixed_gas_prices: None }; + // Set fixed_gas_prices to None in tests to use default gas price behavior + let dev = DevConfig { fee: false, account_validation: true, fixed_gas_prices: None };crates/katana/core/src/backend/mod.rs (2)
43-44: Add documentation for the gas_oracle field.Ohayo! Consider adding doc comments to explain:
- The purpose of the gas_oracle field
- How it interacts with the rest of the Backend struct
- Any important invariants or assumptions
Example:
/// The L1 gas oracle responsible for managing gas prices. /// This component provides current gas prices for block production /// and ensures proper gas price updates based on network conditions. pub gas_oracle: L1GasOracle,
111-114: Consider refactoring update_block_env for better separation of concerns.The method currently handles multiple responsibilities:
- Block number increment
- Timestamp management
- Gas price updates
Consider extracting the gas price update logic into a separate private method for better maintainability.
Example refactor:
impl<EF: ExecutorFactory> Backend<EF> { fn update_gas_prices(&self, block_env: &mut BlockEnv) { block_env.l1_gas_prices = self.gas_oracle.current_gas_prices(); block_env.l1_data_gas_prices = self.gas_oracle.current_data_gas_prices(); } pub fn update_block_env(&self, block_env: &mut BlockEnv) { // ... existing timestamp logic ... block_env.number += 1; block_env.timestamp = timestamp; self.update_gas_prices(block_env); } }crates/katana/node/src/lib.rs (1)
211-218: Consider these architectural points for the future gas oracle implementationThe TODO comment indicates this is a temporary solution. For the future implementation, consider:
- Implementing a trait for different oracle strategies (fixed, dynamic, external)
- Adding configuration for sampling intervals and data sources
- Including fallback mechanisms for failed price fetches
- Implementing caching to prevent excessive sampling
bin/katana/src/cli/node.rs (2)
213-213: Ohayo, sensei! Minor typo in help messageThere's a small typo in the help text for
l1_strk_data_gas_price. It says "gas prick" instead of "gas price".Here's the fix:
-#[arg(help = "The L1 STRK gas prick. (denominated in fri)")] +#[arg(help = "The L1 STRK gas price. (denominated in fri)")]
569-605: Ohayo, sensei! Consider enhancing test coverage for gas price optionsThe
custom_fixed_gas_pricestest checks the correct configuration when all gas price options are provided.It would be beneficial to add tests for scenarios where some gas price options are missing, to ensure that the application handles these cases gracefully and informs the user appropriately.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
bin/katana/src/cli/node.rs(6 hunks)crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/core/src/backend/gas_oracle.rs(1 hunks)crates/katana/core/src/backend/mod.rs(4 hunks)crates/katana/core/src/constants.rs(1 hunks)crates/katana/node/src/config/dev.rs(2 hunks)crates/katana/node/src/lib.rs(3 hunks)
🔇 Additional comments (13)
crates/katana/core/src/constants.rs (2)
9-12: Clear and well-documented constants, sensei!
The constants are well-documented with their respective units (Wei for ETH and STRK for STRK tokens) which helps prevent confusion and potential conversion errors.
9-11: Ohayo! Verify the ETH data gas price magnitude.
The ETH data gas price (10^6 Wei) is significantly lower than the STRK data gas price (10^9 STRK). While this might be intentional due to different token economics, we should verify this difference.
✅ Verification successful
Based on the search results, I can now generate a final response since I have enough context about the gas prices in the codebase:
Ohayo! The ETH and STRK data gas price difference is intentional and correct.
The difference in magnitudes between ETH (10^6 Wei) and STRK (10^9 STRK) data gas prices is intentional and consistent with the codebase's design:
- The codebase shows consistent handling of different units for ETH (Wei) and STRK (Fri/STRK) gas prices
- The default L1 gas prices (non-data) also follow similar patterns:
- ETH: 100 * 10^9 Wei
- STRK: 100 * 10^9 STRK
- The implementation in
blockifier/utils.rsand other core components properly handles these different price scales
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any other gas price configurations or documentation
# that might validate these values
# Look for gas price related configurations
rg -i "gas.*price|wei|strk" --type rust
# Look for related documentation
fd -e md -e txt | xargs rg -i "gas.*price|wei|strk"
Length of output: 37884
crates/katana/node/src/config/dev.rs (2)
1-1: Ohayo! Clean import addition, sensei!
The GasPrices import is well-placed and necessary for the new gas configuration functionality.
41-41: Default implementation looks good, but let's verify the behavior!
The default values align with the PR objectives, setting fixed_gas_prices to None by default.
Let's verify that this default behavior is consistent with other parts of the codebase:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to find any references to DevConfig and analyze its usage patterns across the codebase.
Let's examine how DevConfig is actually used in these files to ensure the default values are appropriate in all contexts.
Default implementation is correctly verified, sensei!
The default implementation of fixed_gas_prices: None is appropriate and consistent across the codebase:
- It's used correctly in test utilities (
test-utils/src/sequencer.rs) - The
L1GasOracleproperly handles the None case inkatana/node/src/lib.rs - CLI properly overrides this default when explicit gas prices are provided
- The implementation aligns with the documented behavior in the configuration structs
Ohayo! The verification shows that this default value works harmoniously with both development and production scenarios, allowing flexible gas price configuration when needed while maintaining sensible defaults. Keep coding with confidence, sensei! 🌸
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances where DevConfig is created with default values
rg -A 3 "DevConfig::default\(\)"
# Search for tests that might rely on these default values
rg -A 5 "test.*DevConfig"
Length of output: 62
Script:
#!/bin/bash
# Let's try a broader search for DevConfig usage and initialization
rg -l "DevConfig"
# Then let's look for test files that might contain DevConfig
fd "test.*\.rs$" | xargs rg "DevConfig"
# Also search for any struct definitions or implementations related to DevConfig
ast-grep --pattern 'struct DevConfig { $$$ }'
ast-grep --pattern 'impl $_ for DevConfig { $$$ }'
Length of output: 654
Script:
#!/bin/bash
# Let's check the actual content of files that use DevConfig
cat crates/dojo/test-utils/src/sequencer.rs
cat bin/katana/src/cli/node.rs
cat crates/katana/node/src/config/mod.rs
# Also search for any usage of fixed_gas_prices field
rg "fixed_gas_prices" -A 3 -B 3
Length of output: 30063
crates/katana/core/src/backend/mod.rs (2)
3-3: LGTM! Clean module organization, sensei!
The gas oracle module integration follows Rust's module system conventions nicely.
Also applies to: 24-24
111-114: Verify gas oracle integration with system tests.
Ohayo sensei! The gas oracle integration looks solid, but we should ensure it's properly tested. Consider adding system tests that verify:
- Gas prices are correctly propagated from the oracle to block environment
- Price updates are reflected in mined blocks
- Integration with the CLI and genesis configuration works as expected
crates/katana/node/src/lib.rs (3)
22-28: Ohayo sensei! The imports look good!
The new imports are well-organized and properly scoped for the gas oracle implementation.
Also applies to: 39-39
207-210: Nice implementation of the fixed gas price configuration!
The code elegantly handles user-specified fixed gas prices from the dev config, which aligns with the PR objective of respecting configuration values.
222-222: Backend struct update looks clean, sensei!
The gas_oracle field is properly integrated into the Backend struct, maintaining good structural organization.
bin/katana/src/cli/node.rs (4)
22-22: Ohayo, sensei! Import statements updated correctly
The new imports for DEFAULT_SEQUER_ADDRESS, FixedL1GasPriceConfig, and GasPrices are appropriate and necessary for the added functionality.
Also applies to: 25-25, 35-35
190-194: Ohayo, sensei! Making validate_max_steps and invoke_max_steps optional
Changing validate_max_steps and invoke_max_steps to Option<u32> allows for more flexible configurations and aligns with their usage in the code.
373-382: Ohayo, sensei! Changes to max steps handling look good
Using unwrap_or(DEFAULT_*) ensures default values are applied when invoke_max_steps and validate_max_steps are not specified, providing flexibility while maintaining defaults.
347-362: Ohayo, sensei! Verify safe unwrapping of gas price options
In the dev_config function, you assume that if l1_eth_gas_price is Some, all other gas price options are also Some, and you use .unwrap() on them.
Given potential issues with the CLI argument requirements, please verify that this assumption is safe.
You can run the following test to ensure that the gas price options are correctly enforced:
✅ Verification successful
Ohayo! The gas price options are safely chained with requires, sensei!
The CLI argument parsing is implemented with proper dependency chains using clap's requires attribute:
l1_eth_gas_pricerequiresl1_strk_gas_pricel1_strk_gas_pricerequiresl1_eth_data_gas_pricel1_eth_data_gas_pricerequiresl1_strk_data_gas_pricel1_strk_data_gas_pricerequiresl1_eth_gas_price
This circular dependency chain ensures that if any gas price option is provided, all of them must be provided. Therefore, the unwrap calls in the code are safe as the CLI parser will reject any incomplete gas price configuration before the code is executed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test that the application fails when not all gas price options are provided.
# Attempt to run the application with only one gas price option.
katana --l1-eth-gas-price 10
# Expected result: The application should display an error indicating that the other gas price options are required.
Length of output: 104
Script:
#!/bin/bash
# Let's check the CLI argument parsing logic in the codebase
# First, find the relevant CLI parsing code
rg -A 10 "l1-eth-gas-price" --type rust
# Then check for any argument validation or requirement checks
rg -A 10 "requires|required_if|required_with" --type rust
Length of output: 12072
Script:
#!/bin/bash
# Let's also check the struct definition and any related code
ast-grep --pattern 'struct $_ {
$$$
l1_eth_gas_price: $_,
$$$
}'
Length of output: 72
Script:
#!/bin/bash
# Check for any clap derive attributes that might enforce requirements
rg "#\[clap\(" --type rust -A 5
Length of output: 10362
| /// Returns the current gas prices. | ||
| pub fn current_gas_prices(&self) -> GasPrices { | ||
| self.gas_prices.clone() | ||
| } | ||
|
|
||
| /// Returns the current data gas prices. | ||
| pub fn current_data_gas_prices(&self) -> GasPrices { | ||
| self.data_gas_prices.clone() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing getter methods to avoid unnecessary cloning.
The current implementation clones the gas prices on every call. Consider returning references instead:
- pub fn current_gas_prices(&self) -> GasPrices {
- self.gas_prices.clone()
+ pub fn current_gas_prices(&self) -> &GasPrices {
+ &self.gas_prices
}
- pub fn current_data_gas_prices(&self) -> GasPrices {
- self.data_gas_prices.clone()
+ pub fn current_data_gas_prices(&self) -> &GasPrices {
+ &self.data_gas_prices
}This change would improve performance by avoiding unnecessary cloning, especially if these methods are called frequently during block updates.
📝 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.
| /// Returns the current gas prices. | |
| pub fn current_gas_prices(&self) -> GasPrices { | |
| self.gas_prices.clone() | |
| } | |
| /// Returns the current data gas prices. | |
| pub fn current_data_gas_prices(&self) -> GasPrices { | |
| self.data_gas_prices.clone() | |
| } | |
| /// Returns the current gas prices. | |
| pub fn current_gas_prices(&self) -> &GasPrices { | |
| &self.gas_prices | |
| } | |
| /// Returns the current data gas prices. | |
| pub fn current_data_gas_prices(&self) -> &GasPrices { | |
| &self.data_gas_prices | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
bin/katana/src/cli/node.rs (2)
347-362: Ohayo, sensei! Consider making the gas price handling more robust.While the implementation works, we can make it more idiomatic and safer:
- let fixed_gas_prices = if self.starknet.environment.l1_eth_gas_price.is_some() { - // It is safe to unwrap all of these here because the CLI parser ensures if one is set, - // all must be set. - - let eth_gas_price = self.starknet.environment.l1_eth_gas_price.unwrap(); - let strk_gas_price = self.starknet.environment.l1_strk_gas_price.unwrap(); - let eth_data_gas_price = self.starknet.environment.l1_eth_data_gas_price.unwrap(); - let strk_data_gas_price = self.starknet.environment.l1_strk_data_gas_price.unwrap(); - - let gas_price = GasPrices { eth: eth_gas_price, strk: strk_gas_price }; - let data_gas_price = GasPrices { eth: eth_data_gas_price, strk: strk_data_gas_price }; - - Some(FixedL1GasPriceConfig { gas_price, data_gas_price }) - } else { - None - }; + let fixed_gas_prices = self.starknet.environment.l1_eth_gas_price.map(|eth_gas_price| { + let env = &self.starknet.environment; + let gas_price = GasPrices { + eth: eth_gas_price, + strk: env.l1_strk_gas_price.expect("CLI ensures STRK gas price is set"), + }; + let data_gas_price = GasPrices { + eth: env.l1_eth_data_gas_price.expect("CLI ensures ETH data gas price is set"), + strk: env.l1_strk_data_gas_price.expect("CLI ensures STRK data gas price is set"), + }; + FixedL1GasPriceConfig { gas_price, data_gas_price } + });
Line range hint
562-598: Ohayo, sensei! Consider adding more test cases for gas prices.The current test case covers the happy path well, but we should add tests for edge cases:
- Test when only some gas prices are provided (should fail)
- Test with invalid combinations of gas prices
- Test with zero or negative gas prices
Example additional test:
#[test] fn test_incomplete_gas_prices() { let result = NodeArgs::try_parse_from([ "katana", "--l1-eth-gas-price", "10", // Missing other gas prices ]); assert!(result.is_err()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/katana/src/cli/node.rs(7 hunks)
🔇 Additional comments (1)
bin/katana/src/cli/node.rs (1)
196-214:
Ohayo, sensei! The circular dependency in gas price arguments needs attention.
The current implementation creates a circular chain of requirements between the gas price arguments:
l1_eth_gas_price → l1_strk_gas_price → l1_eth_data_gas_price → l1_strk_data_gas_price → l1_eth_gas_price
This circular dependency could cause issues with argument parsing. Consider grouping these options together using a custom validator that ensures all gas prices are provided if any one is specified.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
bin/katana/tests/test-data/genesis.json (1)
1-23: Consider adding documentation about CLI override behavior.Ohayo sensei! Given that this genesis file is part of the gas oracle implementation and there's a known behavior where CLI values override genesis values, it would be helpful to add a comment at the top of the file explaining this behavior and its implications for testing.
{ + // Note: These genesis gas prices may be overridden by CLI values. + // See issue #2558 for details about gas oracle implementation. "number": 0,bin/katana/src/cli/node.rs (1)
348-363: Ohayo, sensei! Consider using pattern matching instead of multiple unwraps.While the comment indicates it's safe to unwrap, using pattern matching would make the code more robust and clearer:
- let fixed_gas_prices = if self.starknet.environment.l1_eth_gas_price.is_some() { - // It is safe to unwrap all of these here because the CLI parser ensures if one is set, - // all must be set. - let eth_gas_price = self.starknet.environment.l1_eth_gas_price.unwrap(); - let strk_gas_price = self.starknet.environment.l1_strk_gas_price.unwrap(); - let eth_data_gas_price = self.starknet.environment.l1_eth_data_gas_price.unwrap(); - let strk_data_gas_price = self.starknet.environment.l1_strk_data_gas_price.unwrap(); + let fixed_gas_prices = match ( + &self.starknet.environment.l1_eth_gas_price, + &self.starknet.environment.l1_strk_gas_price, + &self.starknet.environment.l1_eth_data_gas_price, + &self.starknet.environment.l1_strk_data_gas_price, + ) { + (Some(eth), Some(strk), Some(eth_data), Some(strk_data)) => { + let gas_price = GasPrices { eth: *eth, strk: *strk }; + let data_gas_price = GasPrices { eth: *eth_data, strk: *strk_data }; + Some(FixedL1GasPriceConfig { gas_price, data_gas_price }) + } + _ => None, + };
📜 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 (3)
bin/katana/Cargo.toml(1 hunks)bin/katana/src/cli/node.rs(8 hunks)bin/katana/tests/test-data/genesis.json(1 hunks)
🔇 Additional comments (6)
bin/katana/tests/test-data/genesis.json (4)
2-6: LGTM! Block metadata looks good.
Ohayo! The block metadata fields contain valid test values that are suitable for testing purposes.
11-21: LGTM! Account configuration is properly structured.
The test account configuration looks good, sensei! It includes all necessary fields (publicKey, balance, nonce, storage) with appropriate test values.
22-23: Verify if empty contracts and classes are intentional.
While empty contracts and classes sections are valid, we should verify if this is intentional for the gas oracle testing scenario.
#!/bin/bash
# Description: Check if other test genesis files contain contract or class definitions
# to understand if this is a pattern or an oversight
echo "Searching for other genesis test files with contract or class definitions..."
fd -e json -x grep -l '"contracts"\s*:\s*[^{}]' {} \; 2>/dev/null
fd -e json -x grep -l '"classes"\s*:\s*[^\[\]]' {} \; 2>/dev/null7-10: Verify gas price values align with test requirements.
The updated gas prices (ETH: 9999, STRK: 8888) appear to be test values. However, given that this PR implements a gas oracle skeleton and addresses CLI override behavior, we should verify these values.
Consider documenting in a comment why these specific values were chosen for the test data, as it would help other developers understand if these numbers have any significance for testing edge cases.
bin/katana/Cargo.toml (1)
34-34: Ohayo! The starknet dev dependency addition looks good, sensei!
The addition aligns perfectly with the gas oracle implementation goals and follows the workspace versioning pattern consistently used throughout the project.
bin/katana/src/cli/node.rs (1)
Line range hint 565-634: Ohayo, sensei! The test coverage looks great!
The new tests thoroughly verify:
- Custom fixed gas prices configuration
- Genesis configuration with fixed gas prices
- Correct handling of gas price values
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
crates/katana/core/src/backend/mod.rs (2)
43-44: Add documentation for the gas oracle field, sensei!Consider adding documentation comments to explain:
- The purpose of the gas oracle
- How it interacts with the genesis configuration
- Its role in gas price management
Example:
pub executor_factory: Arc<EF>, +/// Gas oracle responsible for managing and providing gas prices. +/// When a custom genesis is provided, it uses the gas prices from the configuration. pub gas_oracle: L1GasOracle,
111-119: Consider adding error handling and validation for gas price updates.The gas price update logic is clean and well-structured, but could benefit from additional safety measures:
- Error handling for potential gas oracle failures
- Validation of returned gas prices (e.g., ensure they're within acceptable ranges)
Example enhancement:
pub fn update_block_gas_prices(&self, block_env: &mut BlockEnv) { - block_env.l1_gas_prices = self.gas_oracle.current_gas_prices(); - block_env.l1_data_gas_prices = self.gas_oracle.current_data_gas_prices(); + let gas_prices = self.gas_oracle.current_gas_prices(); + let data_gas_prices = self.gas_oracle.current_data_gas_prices(); + + // Validate gas prices are within acceptable ranges + if !self.validate_gas_prices(&gas_prices, &data_gas_prices) { + tracing::warn!( + target: LOG_TARGET, + "Invalid gas prices received from oracle, using previous values" + ); + return; + } + + block_env.l1_gas_prices = gas_prices; + block_env.l1_data_gas_prices = data_gas_prices; } + +fn validate_gas_prices(&self, gas_prices: &GasPrices, data_gas_prices: &GasPrices) -> bool { + // Add validation logic here + true +}crates/katana/rpc/rpc/tests/starknet.rs (2)
210-210: Ohayo sensei! Consider documenting the magic number.The amount
0x1ba32524a3000appears to be a magic number. While the change is valid, it would be helpful to document why this specific amount was chosen.- let amount = Uint256 { low: felt!("0x1ba32524a3000"), high: Felt::ZERO }; + // Amount set to ~2 ETH to cover deployment costs and initial transactions + let amount = Uint256 { low: felt!("0x1ba32524a3000"), high: Felt::ZERO };
Line range hint
574-574: Ohayo sensei! Let's address the TODO comment.The TODO comment indicates that more elaborate tests for get_events are needed. I can help implement additional test cases to improve coverage.
Would you like me to help generate additional test cases for the get_events functionality? I can create tests that cover:
- Different filter combinations
- Edge cases for continuation tokens
- Various chunk sizes
- Error scenarios
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
Line range hint
569-571: Consider Makingevents_innerAsynchronous to Avoid BlockingOhayo, sensei! Using
futures::executor::block_onwithin a synchronous function can block the thread and negatively impact performance. Sinceclient.get_eventsis asynchronous, consider refactoringevents_innerto be asynchronous as well. This will allow you to use.awaitand improve concurrency.Apply this diff to refactor the code:
-fn events_inner( +async fn events_inner( &self, from_block: BlockIdOrTag, to_block: BlockIdOrTag, address: Option<ContractAddress>, keys: Option<Vec<Vec<Felt>>>, continuation_token: Option<MaybeForkedContinuationToken>, chunk_size: u64, ) -> StarknetApiResult<EventsPage> { // ... - let forked_result = futures::executor::block_on( - client.get_events(from, to, address, keys, token, chunk_size), - )?; + let forked_result = client.get_events(from, to, address, keys, token, chunk_size).await?;You'll also need to update all the calls to
events_innerto await its result and adjust any related code.
Line range hint
729-733: Use a More Specific Error Variant for ClarityOhayo, sensei! Returning
StarknetApiError::UnexpectedErrormay not clearly convey the nature of the issue. Consider defining and using a more specific error variant likeInvalidBlockRangeto accurately represent this condition.Apply this diff to introduce a new error variant:
+// In StarknetApiError enum definition +InvalidBlockRange { + reason: String, +},And update the error handling:
Err(StarknetApiError::UnexpectedError { reason: "Invalid block range; `from` block must be lower than `to`".to_string(), }) +Err(StarknetApiError::InvalidBlockRange { + reason: "Invalid block range; `from` block must be lower than `to`".to_string(), +})
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/katana/core/src/backend/mod.rs(4 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(1 hunks)crates/katana/rpc/rpc/tests/starknet.rs(1 hunks)
🔇 Additional comments (5)
crates/katana/core/src/backend/mod.rs (1)
3-3: Ohayo sensei! The gas oracle module integration looks good!
The module declaration and import are properly structured, making the gas oracle functionality accessible as part of the public API.
Also applies to: 24-24
crates/katana/rpc/rpc/tests/starknet.rs (1)
Line range hint 1-1017: LGTM! Well-structured test suite with comprehensive coverage.
The test suite demonstrates excellent practices:
- Comprehensive coverage of happy paths and error cases
- Good use of test frameworks and utilities
- Clear test organization and naming
- Proper async/await handling
- Thorough assertions and error checks
crates/katana/rpc/rpc/src/starknet/mod.rs (3)
203-213: Handling of Pending Block Environment is Correct
Ohayo, sensei! The logic for handling the pending block environment is implemented appropriately. When there's a pending block, it correctly retrieves the block environment from the pending executor. Otherwise, it creates a new block environment and updates it to reflect the current state.
Line range hint 659-679: Duplicate Concern Regarding Blocking with block_on
Ohayo, sensei! The use of futures::executor::block_on here mirrors the pattern in the previous code segment. Please refer to my earlier comment about refactoring events_inner to be asynchronous to avoid blocking the thread.
Line range hint 755-759: Verification of Block Number Increment for Pending Blocks
Ohayo, sensei! The logic for handling pending blocks by considering latest + 1 as the block number seems appropriate. This ensures that pending blocks are assigned a unique block number higher than the latest confirmed block.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
crates/katana/core/src/backend/mod.rs (2)
43-44: Add documentation for the gas_oracle field.Ohayo sensei! While the field addition looks good, please consider adding documentation to explain the purpose and usage of the gas_oracle field, especially since it's a public field.
pub executor_factory: Arc<EF>, +/// The gas oracle responsible for managing gas prices. pub gas_oracle: L1GasOracle,
111-114: Add a comment explaining the update sequence.Consider adding a comment to explain why gas prices are updated after the timestamp:
block_env.timestamp = timestamp; +// Update gas prices after timestamp to ensure consistent block environment // update the gas prices self.update_block_gas_prices(block_env);crates/katana/node/src/lib.rs (2)
205-218: Consider adding more documentation about the gas oracle implementation.While the TODO comment indicates this is a temporary solution, it would be helpful to:
- Document the expected behavior of the future gas oracle implementation
- Explain how the sampling mechanism will work
- Specify what factors will influence gas price calculations
This documentation will help future contributors understand the intended design.
205-225: Consider future extensibility of the gas oracle system.The current implementation tightly couples the gas oracle with the Backend struct. For better extensibility, consider:
- Creating a trait for gas oracle implementations
- Using dependency injection for the gas oracle
- Implementing a proper observer pattern for gas price updates
This would make it easier to:
- Add new gas oracle implementations
- Test different strategies
- Mock the gas oracle in tests
bin/katana/src/cli/node.rs (1)
Line range hint
565-634: Ohayo, sensei! Consider adding a negative test case.The test coverage for gas prices is good, but consider adding a test case that verifies the CLI parser correctly rejects partial gas price configurations (e.g., when only some of the gas price arguments are provided).
Example test case:
#[test] #[should_panic] fn partial_gas_prices_should_fail() { NodeArgs::parse_from([ "katana", "--l1-eth-gas-price", "100", "--l1-strk-gas-price", "200", // Missing eth and strk data gas prices ]); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (12)
bin/katana/Cargo.toml(1 hunks)bin/katana/src/cli/node.rs(8 hunks)bin/katana/tests/test-data/genesis.json(1 hunks)crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/core/src/backend/gas_oracle.rs(1 hunks)crates/katana/core/src/backend/mod.rs(4 hunks)crates/katana/core/src/backend/storage.rs(1 hunks)crates/katana/core/src/constants.rs(1 hunks)crates/katana/node/src/config/dev.rs(2 hunks)crates/katana/node/src/lib.rs(3 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(1 hunks)crates/katana/rpc/rpc/tests/starknet.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- bin/katana/Cargo.toml
- bin/katana/tests/test-data/genesis.json
- crates/dojo/test-utils/src/sequencer.rs
- crates/katana/core/src/backend/gas_oracle.rs
- crates/katana/core/src/backend/storage.rs
- crates/katana/core/src/constants.rs
- crates/katana/node/src/config/dev.rs
- crates/katana/rpc/rpc/src/starknet/mod.rs
🔇 Additional comments (10)
crates/katana/core/src/backend/mod.rs (3)
3-3: LGTM! Clean module organization.
The gas oracle module declaration and import are well-organized and properly scoped.
Also applies to: 24-24
116-119: LGTM! Clean and focused implementation.
The method is well-documented and correctly encapsulates the gas price update logic.
116-119: Verify gas price updates in all block creation paths.
Let's ensure that gas prices are consistently updated across all block creation scenarios.
✅ Verification successful
Let me search for more specific information about block gas price updates.
Ohayo sensei! After analyzing the codebase, I can now provide a definitive response about the gas price updates.
Gas price updates are properly implemented across block creation paths ✅
The gas price update mechanism is correctly integrated through:
-
The
update_block_gas_pricesmethod inBackendthat updates both:l1_gas_pricesfrom gas oraclel1_data_gas_pricesfrom gas oracle
-
This method is called in the critical path during block creation:
- Inside
update_block_envwhich is invoked before any block creation - Used by both regular and empty block creation flows
- Inside
-
The gas oracle implementation (
L1GasOracle) provides consistent gas prices through:current_gas_prices()current_data_gas_prices()
The gas price updates are properly synchronized with block creation, ensuring all blocks have up-to-date gas prices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all methods that create or modify blocks to verify gas price updates
# Search for methods that might need gas price updates
echo "Searching for block creation/modification methods..."
rg -A 2 'fn.*block.*|block_env' --type rust
# Search for existing gas price update calls
echo "Checking current gas price update usage..."
rg 'update_block_gas_prices|l1_gas_prices|l1_data_gas_prices' --type rust
Length of output: 106276
Script:
#!/bin/bash
# Search for all places where block gas prices are modified or used in execution
echo "Searching for block gas price modifications..."
rg -A 5 'fn.*gas.*price|gas_oracle' --type rust
# Search for gas oracle implementation
echo "Searching for gas oracle implementation..."
ast-grep --pattern 'trait GasOracle {
$$$
}'
Length of output: 6549
crates/katana/node/src/lib.rs (3)
22-22: Ohayo sensei! Clean and well-organized imports.
The new imports clearly indicate the introduction of gas oracle functionality and its associated constants.
Also applies to: 25-28
220-225: Verify field initialization dependencies.
The gas_oracle field is correctly added to the Backend struct. However, please ensure that there are no implicit dependencies between the gas_oracle and other fields that might require a specific initialization order.
✅ Verification successful
No field initialization dependencies found in Backend struct
Looking at the Backend struct definition in crates/katana/core/src/backend/mod.rs, the fields are independent and don't have any implicit ordering requirements. The gas_oracle field is a standalone component that can be initialized in any order relative to other fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any field dependencies in Backend struct initialization
# Search for Backend struct initialization patterns
ast-grep --pattern 'Backend {
$$$
gas_oracle: $_,
$$$
}'
Length of output: 159
Script:
#!/bin/bash
# Let's check the Backend struct definition and its usage patterns
rg -U "struct Backend" -A 10
# Check for any Backend initialization patterns
rg -U "Backend \{" -A 10
# Look for any field dependencies or ordering requirements in tests or documentation
rg -U "Backend::new|impl Backend" -A 15
Length of output: 12456
207-210: Verify fixed gas price configuration handling.
The code correctly handles fixed gas prices from dev config, but let's ensure all configuration paths are properly tested.
✅ Verification successful
Ohayo sensei! Based on the search results, I can now provide a definitive response about the gas price configuration handling:
Fixed gas price configuration is properly implemented and tested
The codebase shows comprehensive handling of fixed gas prices:
- Default gas prices are used when no fixed prices are specified
- Fixed gas prices from dev config are properly passed to
L1GasOracle::fixed() - Test coverage exists for both custom fixed gas prices and genesis gas price configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing tests related to gas price configuration
# Search for test files and functions related to gas price configuration
rg -t rust "test.*gas.*price" --glob "**/*test*.rs"
# Search for gas price configuration handling in the codebase
ast-grep --pattern 'fn $_(fixed_gas_prices: $_) {
$$$
}'
Length of output: 159
Script:
#!/bin/bash
# Let's search more comprehensively for gas price related tests and configurations
# Search for test modules and functions related to gas price
rg -t rust "mod\s+test.*|#\[test\].*gas.*price" -A 5
# Search for gas price configuration structures and implementations
rg -t rust "struct.*Gas|impl.*Gas|fixed_gas_prices" -A 3
# Look for L1GasOracle usage and implementations
rg -t rust "L1GasOracle" -A 3
# Search for configuration handling related to gas prices
rg -t rust "config.*gas.*price" -A 3
Length of output: 52181
bin/katana/src/cli/node.rs (2)
196-214: Ohayo, sensei! The gas price arguments still have a circular dependency.
The circular dependency issue in the gas price arguments that was previously identified still exists.
348-363: LGTM! Safe handling of gas price configuration.
The implementation safely handles the gas price configuration. The unwrap calls are protected by the CLI parser's requirements and the initial existence check.
crates/katana/rpc/rpc/tests/starknet.rs (2)
Line range hint 1-1018: Ohayo sensei! The test coverage looks excellent!
The test suite demonstrates comprehensive coverage of StarkNet RPC functionality, including:
- Transaction handling (success and failure cases)
- Event processing and pagination
- Transaction tracing
- Gas price handling
- Error scenarios
The test structure is well-organized with clear setup, execution, and verification steps.
210-210: Ohayo sensei! Please verify the updated transfer amount.
The amount for account deployment funding has been increased significantly. Let's ensure this new value (0x1ba32524a3000) aligns with the current minimum balance requirements.
✅ Verification successful
Let me gather more context about this specific test case to understand the purpose of this amount.
Let me check if there are any related gas or fee configurations that might explain this specific amount.
Let me check the test context to understand the purpose of this specific amount.
Let me try a different approach to find the test context.
Ohayo sensei! The updated amount is perfectly valid.
The amount 0x1ba32524a3000 is used in the deploy_account test to fund a new account with enough tokens to cover deployment costs. The test is specifically designed to verify account deployment functionality with both fee-enabled and fee-disabled modes, and the amount is sufficient for this purpose. The test's structure shows that this is an intentional change, as it includes proper validation of the deployment process and even tests deployment with empty balance in fee-disabled mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other occurrences of similar transfer amounts to verify consistency
rg -A 2 "0x1ba32524a3000|0x100000000000" --type rust
Length of output: 2936
Script:
#!/bin/bash
# Get the context of the deploy_account test where this amount is used
rg -B 10 -A 10 "0x1ba32524a3000" --type rust
Length of output: 2039
Script:
#!/bin/bash
# Search for gas/fee related constants and configurations
rg -B 2 -A 2 "fee|gas.*constant|min.*balance" --type rust --ignore-case crates/katana/
Length of output: 194508
Script:
#!/bin/bash
# Get more context around the deploy_account test
rg -B 20 -A 10 "deploy_account.*async fn" crates/katana/rpc/rpc/tests/starknet.rs
Length of output: 83
Script:
#!/bin/bash
# Get the full context of the deploy_account test function
cat crates/katana/rpc/rpc/tests/starknet.rs | grep -A 50 -B 50 "0x1ba32524a3000"
Length of output: 4951
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2643 +/- ##
==========================================
+ Coverage 57.17% 57.25% +0.07%
==========================================
Files 399 400 +1
Lines 49731 49826 +95
==========================================
+ Hits 28433 28526 +93
- Misses 21298 21300 +2 ☔ View full report in Codecov by Sentry. |
implements a skeleton for the gas oracle type #2558.
currently, when a custom genesis is provided, it is expected that katana will use the gas prices as specified in the config file (yes, those values are not supposed to be used as the actual gas values for subsequent blocks). but in the cli stage, we overwrite the genesis values with the cli values instead.
dojo/bin/katana/src/cli/node.rs
Lines 333 to 334 in 24964ce
and because the cli has default values set, doesn't matter what gas prices you set in the genesis file, the actual values that will be used will always be the ones from the cli. even if the args arent explicitly given by the user (due to the default values).
dojo/bin/katana/src/cli/node.rs
Lines 200 to 210 in 24964ce
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These changes aim to improve user experience by allowing more customizable and clear gas price settings.