feat(katana): starknet gas oracle placeholder #2874
Conversation
WalkthroughOhayo, sensei! This pull request introduces a significant refactoring of the Katana project's chain specification and gas oracle components. The changes primarily involve creating a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Node
participant ChainSpec
participant GasOracle
Client->>Node: Initialize with Chain Configuration
Node->>ChainSpec: Load Chain Specification
ChainSpec-->>Node: Return Configuration
Node->>GasOracle: Create Gas Oracle
alt Settlement Layer is Ethereum
GasOracle->>GasOracle: Sample Ethereum Gas Prices
else Settlement Layer is Starknet
GasOracle->>GasOracle: Use Fixed Gas Price
end
GasOracle-->>Node: Provide Gas Prices
Possibly related issues
Possibly related PRs
Sensei, this refactoring looks like a solid step towards more modular and configurable chain specifications in the Katana project! 🚀 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (28)
✅ Files skipped from review due to trivial changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (30)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Nitpick comments (4)
bin/katana/src/cli/init/mod.rs (1)
131-136: Consider removing or properly tracking commented-out codeOhayo, sensei! The code block for handling
fee_tokenis commented out. Leaving commented-out code can clutter the codebase and may cause confusion. If it's not needed soon, consider removing it or tracking the TODO in an issue.crates/katana/core/src/backend/mod.rs (1)
44-44: Consider documenting gas oracle behaviorSince this is a placeholder implementation, it would be helpful to add documentation about the expected behavior and future plans for the gas oracle.
Add a doc comment explaining the gas oracle's role:
+ /// Gas oracle responsible for providing gas price information. + /// Currently a placeholder implementation that will be enhanced with actual + /// Starknet gas price tracking in the future. pub gas_oracle: GasOracle,crates/katana/core/src/backend/gas_oracle.rs (1)
62-68: Add a TODO comment for future implementation.While the placeholder implementation is well-documented, it would be helpful to add a TODO comment indicating future plans for implementing actual Starknet gas price sampling.
/// This is just placeholder for now, as Starknet doesn't provide a way to get the L2 gas /// prices, we just return a fixed gas price values of 0. This is equivalent to calling /// [`GasOracle::fixed`] with 0 values for both gas and data prices. /// /// The result of this is the same as running the node with fee disabled. +/// TODO: Implement actual Starknet gas price sampling when the L2 gas price API becomes available. pub fn sampled_starknet() -> Self {crates/katana/cli/src/args.rs (1)
228-261: Consider enhancing error context in chain spec loading.While the implementation is solid, the error handling could be more informative for users.
Consider adding more context to the error handling:
- let mut chain_spec = ChainSpec::load(path).context("failed to load chain spec")?; + let mut chain_spec = ChainSpec::load(path).context(format!( + "failed to load chain spec from path: {}", + path.display() + ))?;
🛑 Comments failed to post (4)
crates/katana/node/src/lib.rs (1)
27-30: 🛠️ Refactor suggestion
Avoid importing constants from test utilities in production code
Ohayo, sensei! Importing
DEFAULT_ETH_L1_GAS_PRICEand related constants fromtest_utilsis not recommended for production code. Consider defining these constants in a production module to prevent dependencies on test code.crates/katana/primitives/src/genesis/json.rs (1)
27-27: 💡 Codebase verification
Ohayo sensei!
Feature flag inconsistency detected between crates
The change from "slot" to "controller" feature in
primitives/genesis/json.rscreates an inconsistency with the CLI crate, which still uses the "slot" feature for controller-related functionality. Affected files:
crates/katana/cli/src/args.rs: Uses "slot" feature to control controller account setupcrates/katana/cli/src/options.rs: Defines slot-specific optionscrates/katana/cli/src/utils.rs: Imports affected genesis types🔗 Analysis chain
Verify feature flag consistency across the codebase.
The feature flag change from "slot" to "controller" needs to be consistently applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to the old feature flag rg --type rust '#\[cfg\(feature\s*=\s*"slot"\)\]'Length of output: 235
Script:
#!/bin/bash # Check for controller feature in the affected files echo "=== Checking for controller feature ===" rg --type rust '#\[cfg\(feature\s*=\s*"controller"\)\]' crates/katana/cli/src/ # Check for imports and relationships echo -e "\n=== Checking file relationships ===" rg --type rust -l "use.*primitives.*genesis" crates/katana/cli/src/Length of output: 420
Script:
#!/bin/bash # Check the specific imports and context around feature flags echo "=== Checking genesis imports and feature contexts ===" for file in utils.rs options.rs args.rs; do echo -e "\n=== $file ===" rg -B 2 -A 2 "use.*primitives.*genesis|#\[cfg\(feature\s*=\s*\"slot\"\)\]" "crates/katana/cli/src/$file" doneLength of output: 2275
crates/katana/cli/Cargo.toml (1)
9-9: 💡 Codebase verification
Ohayo! We found a critical dependency issue, sensei!
The
wait_signalsfunctionality fromdojo-utilsis used across multiple components including the CLI, node, and Torii. Removing thedojo-utilsdependency without providing an alternative will break the signal handling functionality.
crates/katana/cli/Cargo.toml: Keep thedojo-utilsdependency until signal handling is migrated- Alternative: Move the signal handling to a common utility if you want to remove the
dojo-utilsdependency🔗 Analysis chain
Ohayo! Nice architectural refinement, sensei!
The shift from
katana-primitives/slottokatana-chain-spec/controllerin the slot feature shows good architectural organization. However, let's verify the removal ofdojo-utilsdependency.Let's confirm no functionality is lost with the removal of dojo-utils:
Also applies to: 39-39
Ohayo! We have a small oversight, sensei!
The code in
args.rsstill usesdojo_utils::signal::wait_signals(), but thedojo-utilsdependency has been removed fromCargo.toml. This will cause compilation errors.
crates/katana/cli/src/args.rs: Remove or replace thedojo_utils::signal::wait_signals()usage🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining dojo-utils usage rg "dojo_utils::" crates/katana/cli/src/Length of output: 126
Script:
#!/bin/bash # Check if similar signal handling exists in chain-spec rg "wait_signals" -A 2Length of output: 821
crates/katana/executor/Cargo.toml (1)
24-24: 💡 Codebase verification
Ohayo! The chain-spec dependency appears unused, sensei!
The dependency
katana-chain-specis not used in the executor crate's source code or tests. Consider removing it entirely instead of discussing its placement in dev-dependencies.🔗 Analysis chain
Verify the placement of katana-chain-spec as dev-dependency, sensei!
Given that chain-spec contains gas oracle functionality, should it be a runtime dependency instead of a dev-dependency? This might be needed for gas-related execution logic.
Let's check for any chain-spec usage in the main executor code:
Also applies to: 28-28, 29-29
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for chain-spec usage in executor source rg "katana_chain_spec::" crates/katana/executor/src/Length of output: 52
Script:
#!/bin/bash # Check for any chain-spec related imports or types rg -i "chain.?spec" crates/katana/executor/src/ # Check test directory specifically rg -i "chain.?spec" crates/katana/executor/src/ --type-add 'test:*.rs' --type test # Look for potential imports without namespace fd -e rs . crates/katana/executor/src/ -x ast-grep -p 'use $$$chain_spec$$$'Length of output: 213
Script:
#!/bin/bash # Check git history for this dependency git grep -l "katana-chain-spec" $(git rev-parse HEAD^) -- crates/katana/executor/Cargo.toml # Also check if it's used in any other files that might have been renamed/moved git log -p --full-history -S "katana-chain-spec" -- crates/katana/executor/Length of output: 209
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## katana/chainspec #2874 +/- ##
====================================================
- Coverage 55.74% 55.72% -0.03%
====================================================
Files 446 447 +1
Lines 57818 57821 +3
====================================================
- Hits 32233 32223 -10
- Misses 25585 25598 +13 ☔ View full report in Codecov by Sentry. |
Starknet doesn't yet provide a way to query the L2 gas prices from the current RPC specs (but soon will be in [RPC v0.80 ](https://github.com/starkware-libs/starknet-specs/blob/c94df2c5866e11c866abd3d234b0d5df681073c3/api/starknet_api_openrpc.json#L1603-L1607)), so this is merely a placeholder to make sure that we can run the node with Starknet as the settlement layer when specified in the `ChainSpec`. Reference: #2870
Starknet doesn't yet provide a way to query the L2 gas prices from the current RPC specs (but soon will be in RPC v0.80 ), so this is merely a placeholder to make sure that we can run the node with Starknet as the settlement layer when specified in the
ChainSpec. Reference: #2870