Merge katana/chainspec
#2947
Merge katana/chainspec
#2947
Conversation
Make the flow more coherent by including information of the settlement layer stuff in the ChainSpec. For example, the messaging component has its own config struct to specify the core contract and RPC url of the underlying layer. These informations are also required by other component (mainly gas oracle) so we want to centralize where we put these information. Having it in the ChainSpec struct make sense because these information define the rollup chain itself.
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
To allow specifying the genesis file in the chain spec file in its JSON format. Basically, in the same format as if when the genesis file is passed using `--genesis`. The JSON repr is a more user facing format compared to its internal counterpart (ie `Genesis`).
The protocol version isn't exactly being used (for what it's meant to be) except for putting it into the block header when building blocks. So, semantically it does nothing, and also it doesn't make sense to include it in the chain spec anyway. For now, we're removing it and replace all of its references with a constant.
Pull Request AnalysisOhayo, sensei! 🍵 Let's dive into this comprehensive refactoring of the Katana project's chain specification management! WalkthroughThis pull request introduces a significant architectural change by creating a new Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant ChainSpec as katana-chain-spec
participant Backend as Katana Backend
participant Node as Katana Node
Dev->>ChainSpec: Define Chain Configuration
ChainSpec->>Backend: Provide Chain Specification
Backend->>Node: Configure Node Parameters
Node-->>Dev: Launch Configured Node
Possibly Related PRs
Suggested ReviewersWould you like me to elaborate on any specific aspect of this pull request, sensei? 🍜 🪧 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
🧹 Nitpick comments (15)
bin/katana/src/cli/init/mod.rs (4)
26-26: Ohayo sensei! Consider removing or expandingInitArgsstructSince
InitArgsis currently an empty struct, consider removing it if it's unnecessary, or keep it if you plan to add fields in the future to handle initialization arguments.
128-133: Ohayo sensei! Address the TODO regarding fee tokenThere's a TODO comment to uncomment the fee token prompt once it's in use. Would you like assistance in implementing this functionality, or should we open a new issue to track it?
163-178: Ohayo sensei! Consistent documentation commentsConsider using
///for all field comments inPromptOutcometo generate documentation and maintain consistency across the codebase.Apply this diff to update the comments:
}
181-188: Ohayo sensei! Consider replacinglazy_static!withonce_cell::sync::LazySince Rust 1.70, the standard library provides
std::sync::OnceLockandOnceCell, which can be used instead oflazy_static!. This reduces dependencies and leverages stabilized features.Apply this diff to update the code:
- use lazy_static::lazy_static; + use once_cell::sync::Lazy;crates/katana/chain-spec/src/file.rs (2)
155-156: Ohayo sensei! Fix typo in commentIn the comment on line 156, "testes" should be "tests".
Apply this diff to correct the typo:
147-149: Ohayo sensei! Consider replacing deprecateddirscrateThe
dirscrate is deprecated. Consider using thedirs-nextordirectoriescrate instead to determine the local configuration directory.Apply this diff to update the code:
- Ok(dirs::config_local_dir().ok_or(Error::UnsupportedOS)?.join(KATANA_LOCAL_DIR)) + Ok(dirs_next::config_local_dir().ok_or(Error::UnsupportedOS)?.join(KATANA_LOCAL_DIR))Also, update your
Cargo.tomlto replacedirswithdirs-next:- dirs = "3.0" + dirs-next = "2.0"crates/katana/chain-spec/src/lib.rs (2)
60-91: Ohayo sensei! Add documentation comments forSettlementLayerenumConsider adding documentation comments to the
SettlementLayerenum variants and their fields to enhance code readability and maintainability.
Line range hint
161-196: Ohayo sensei! Replacelazy_static!withonce_cell::sync::LazySince Rust 1.70,
std::sync::OnceLockandOnceCellare available in the standard library. Switching fromlazy_static!reduces dependencies and utilizes stable features.Apply this diff to update the code:
- use lazy_static::lazy_static; + use once_cell::sync::Lazy; - lazy_static! { + pub static DEV: Lazy<ChainSpec> = Lazy::new(|| { /// The default chain specification in dev mode. - pub static ref DEV: ChainSpec = { let mut chain_spec = DEV_UNALLOCATED.clone(); // ... - }; + }); + pub static DEV_UNALLOCATED: Lazy<ChainSpec> = Lazy::new(|| { // ... + });Also, update your
Cargo.tomlto removelazy_staticif it's no longer needed.crates/katana/primitives/src/genesis/json.rs (3)
288-304: Ohayo sensei, consider refactoring the temporary fix for controller class insertion.The current implementation within the
#[cfg(feature = "controller")]block is marked as a "band aid fix". To enhance maintainability and scalability, please plan to refactor this section to provide a more robust solution for initializing the controller class in the genesis configuration.
806-816: Ohayo sensei, replaceunwrap()withexpect()in tests for clearer error messages.Using
unwrap()in tests can cause panics without informative messages upon failure. Consider replacingunwrap()withexpect()and provide meaningful error messages to improve test diagnostics and debugging.Apply this diff to enhance error handling:
compiled_class_hash: CONTROLLER_ACCOUNT_CLASS .clone() .compile() - .unwrap() + .expect("Failed to compile CONTROLLER_ACCOUNT_CLASS") .class_hash() - .unwrap(), + .expect("Failed to get class hash of CONTROLLER_ACCOUNT_CLASS"),
1006-1011: Ohayo sensei, enhance test reliability by handling potential errors explicitly.In this test block, replacing
unwrap()withexpect()and adding descriptive error messages will improve the robustness of the tests and make debugging easier if failures occur.Apply this diff:
compiled_class_hash: CONTROLLER_ACCOUNT_CLASS .clone() .compile() - .unwrap() + .expect("Failed to compile CONTROLLER_ACCOUNT_CLASS") .class_hash() - .unwrap(), + .expect("Failed to get class hash of CONTROLLER_ACCOUNT_CLASS"),crates/katana/core/src/backend/gas_oracle.rs (2)
20-22: Ohayo! Nice refactoring of the GasOracle enum, sensei!The renaming from L1GasOracle to GasOracle makes the type more generic and better suited for supporting multiple settlement layers.
Consider adding doc comments to explain each variant's purpose and when to use them.
300-304: Consider adding test coverage for Starknet gas oracle.While the Ethereum gas oracle tests are updated correctly, we should add test cases for the Starknet implementation, even if it's currently a placeholder.
crates/katana/cli/src/args.rs (1)
241-277: Chain spec handling implementation looks solid!The new implementation properly handles:
- Reading chain spec from file when
chainis provided- Generating development chain spec with proper overrides
- Setting sequencer address correctly
However, there's a potential improvement:
Consider extracting the development chain spec generation into a separate method for better readability:
fn chain_spec(&self) -> Result<Arc<ChainSpec>> { if let Some(id) = &self.chain { use katana_chain_spec::file; let mut chain_spec = file::read(id).context("failed to load chain spec")?; chain_spec.genesis.sequencer_address = *DEFAULT_SEQUENCER_ADDRESS; Ok(Arc::new(chain_spec)) } else { - let mut chain_spec = katana_chain_spec::DEV_UNALLOCATED.clone(); - if let Some(id) = self.starknet.environment.chain_id { - chain_spec.id = id; - } - // ... rest of development chain spec generation + self.generate_dev_chain_spec() } } + +fn generate_dev_chain_spec(&self) -> Result<Arc<ChainSpec>> { + let mut chain_spec = katana_chain_spec::DEV_UNALLOCATED.clone(); + if let Some(id) = self.starknet.environment.chain_id { + chain_spec.id = id; + } + // ... rest of development chain spec generation + Ok(Arc::new(chain_spec)) }crates/katana/node/Cargo.toml (1)
9-9: Sugoi implementation, sensei! 🎯The addition of both
katana-chain-specandtomlsuggests that chain specifications will be loaded from TOML configuration files, which is a common and clean approach for managing chain configurations.Consider documenting the expected TOML schema for chain specifications to help users create custom configurations.
Also applies to: 30-30
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
Cargo.toml(2 hunks)bin/katana/Cargo.toml(2 hunks)bin/katana/src/cli/init/mod.rs(6 hunks)crates/dojo/test-utils/Cargo.toml(1 hunks)crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/chain-spec/Cargo.toml(1 hunks)crates/katana/chain-spec/src/file.rs(1 hunks)crates/katana/chain-spec/src/lib.rs(7 hunks)crates/katana/cli/Cargo.toml(3 hunks)crates/katana/cli/src/args.rs(5 hunks)crates/katana/cli/src/options.rs(2 hunks)crates/katana/cli/src/utils.rs(1 hunks)crates/katana/contracts/piltover(1 hunks)crates/katana/core/Cargo.toml(1 hunks)crates/katana/core/src/backend/gas_oracle.rs(6 hunks)crates/katana/core/src/backend/mod.rs(3 hunks)crates/katana/core/src/backend/storage.rs(5 hunks)crates/katana/core/src/service/block_producer.rs(2 hunks)crates/katana/executor/Cargo.toml(1 hunks)crates/katana/executor/tests/fixtures/mod.rs(2 hunks)crates/katana/executor/tests/fixtures/transaction.rs(1 hunks)crates/katana/node/Cargo.toml(2 hunks)crates/katana/node/src/config/mod.rs(1 hunks)crates/katana/node/src/lib.rs(3 hunks)crates/katana/primitives/src/class.rs(1 hunks)crates/katana/primitives/src/contract.rs(1 hunks)crates/katana/primitives/src/eth.rs(1 hunks)crates/katana/primitives/src/genesis/json.rs(8 hunks)crates/katana/primitives/src/genesis/mod.rs(2 hunks)crates/katana/primitives/src/lib.rs(1 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(4 hunks)crates/katana/storage/provider/Cargo.toml(1 hunks)crates/katana/storage/provider/src/test_utils.rs(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- crates/katana/primitives/src/eth.rs
- crates/katana/primitives/src/class.rs
- crates/katana/executor/tests/fixtures/transaction.rs
- crates/katana/contracts/piltover
- crates/katana/chain-spec/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (36)
bin/katana/src/cli/init/mod.rs (7)
8-14: Ohayo sensei! New imports are appropriately addedThe added imports correspond to the new functionalities introduced and are necessary for the code to compile correctly.
36-40: Ohayo sensei! Settlement layer initialized correctlyThe
SettlementLayer::Starknetconfiguration is properly populated with the user input parameters, ensuring correct setup of the settlement layer.
43-46: Ohayo sensei! Chain specification set up appropriatelyThe
chain_specis correctly initialized with the genesis state, chain ID, and settlement layer, preparing the chain for operation.
48-48: Ohayo sensei! Chain spec file written with proper error handlingThe
writefunction is called onchain_specwith error context added. Errors during file operations will be propagated with additional context, aiding in debugging.
53-65: Ohayo sensei! User input validation implemented correctlyThe
promptmethod validates the chain ID to ensure it contains only ASCII characters, enhancing input reliability and preventing invalid entries.
84-84: Ohayo sensei! Settlement chain selection prompt addedThe user is prompted to select the settlement chain from the provided options, improving user experience and guiding correct configuration.
153-158: Ohayo sensei!PromptOutcomeconstructed successfullyThe
PromptOutcomestruct is populated with the gathered inputs, and potential errors are correctly handled with the?operator.crates/katana/primitives/src/lib.rs (1)
9-9: Ohayo sensei, verify the public exposure of theethmodule.Adding the
ethmodule to the public exports may impact the library's API. Please ensure that it is intended to be publicly accessible and that this change aligns with the project's module visibility strategy.crates/katana/node/src/config/mod.rs (2)
14-14: Ohayo sensei, confirm the updated import path forChainSpec.The import has been updated to
katana_chain_spec::ChainSpec. Please verify that all references toChainSpecthroughout the codebase are updated accordingly to prevent any inconsistencies or build errors.
Line range hint
25-25: Ohayo sensei, assess the impact of removingl1_provider_url.The
l1_provider_urlfield has been removed from theConfigstruct. Ensure that any functionality relying on this configuration is appropriately updated, and that its removal does not introduce regressions or affect features dependent on L1 provider interactions.crates/katana/storage/provider/src/test_utils.rs (1)
4-4: Ohayo! The import changes and chain spec usage look good, sensei!The migration of chain specification to its own crate improves modularity.
Also applies to: 44-44
crates/katana/primitives/src/contract.rs (1)
25-28: Ohayo! Nice addition of convenience constants, sensei!These predefined constants will help reduce magic numbers in the codebase and improve readability.
crates/dojo/test-utils/src/sequencer.rs (1)
3-3: Ohayo! The import update aligns with the new architecture, sensei!The change consistently reflects the migration of chain specification to its dedicated crate.
crates/katana/primitives/src/genesis/mod.rs (1)
10-11: Ohayo! The feature flag update looks good, sensei!The change from 'slot' to 'controller' feature better reflects its purpose.
crates/katana/core/src/backend/gas_oracle.rs (2)
62-68: Verify the placeholder implementation for Starknet gas prices.The zero gas price implementation is well-documented as a placeholder. However, we should track this for future implementation.
Would you like me to create an issue to track the implementation of proper Starknet gas price sampling?
74-75: Clean implementation of the GasOracle methods, sensei!The implementation maintains existing functionality while adapting to the new enum structure.
Also applies to: 82-83, 91-92
crates/katana/node/src/lib.rs (1)
205-212: Excellent settlement layer integration, sensei!The gas oracle initialization properly handles different settlement layers with a clean match expression.
crates/katana/executor/tests/fixtures/mod.rs (1)
4-4: Clean import updates, sensei!The changes correctly update the imports and constant usage to the new module structure.
Also applies to: 53-53
crates/katana/cli/src/utils.rs (1)
8-8: Import path updated correctly, sensei!The ChainSpec import has been properly updated to use the new module.
crates/katana/core/src/backend/mod.rs (3)
3-4: Ohayo! Import organization looks good, sensei!The imports have been properly reorganized to use the new
katana_chain_speccrate andCURRENT_STARKNET_VERSION.Also applies to: 15-15
46-46: Type change fromL1GasOracletoGasOraclelooks good!The simplified type name better represents its purpose without the L1 prefix.
178-178: Verify the protocol version change impact.The protocol version is now sourced from
CURRENT_STARKNET_VERSIONinstead ofchain_spec.version. This change makes version handling more consistent but could affect compatibility with different chain specifications.✅ Verification successful
Ohayo sensei!
Protocol version change is safe and beneficial ✨
The change to use
CURRENT_STARKNET_VERSION(0.13.1.1) improves code consistency by establishing a single source of truth for the protocol version. No compatibility issues were found as the oldchain_spec.versionis not referenced anywhere else in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any code relies on chain_spec.version # Search for any remaining references to chain_spec.version rg -l "chain_spec.*version" --type rust # Search for CURRENT_STARKNET_VERSION usage rg -l "CURRENT_STARKNET_VERSION" --type rustLength of output: 336
Script:
#!/bin/bash # Get definition and usage context of CURRENT_STARKNET_VERSION echo "=== CURRENT_STARKNET_VERSION definition and usage ===" rg "CURRENT_STARKNET_VERSION" -B 2 -A 2 --type rust echo -e "\n=== chain_spec version usage patterns ===" rg "chain_spec.*version" -B 2 -A 2 --type rust echo -e "\n=== Any version-related structs or comparisons ===" ast-grep --pattern 'struct $_ { $$$ version: $_, $$$ }'Length of output: 7547
crates/katana/cli/src/options.rs (1)
152-152: Ohayo! Nice argument constraint additions, sensei!The added constraints prevent conflicting chain specifications by ensuring that:
genesiscannot be used withchainchain_idcannot be used withchainThis helps avoid ambiguous configurations.
Also applies to: 176-176
crates/katana/core/src/backend/storage.rs (1)
4-4: Ohayo! Import path update looks good, sensei!The
ChainSpecimport has been properly updated to use the newkatana_chain_speccrate.crates/katana/cli/src/args.rs (2)
46-49: Ohayo! New chain option looks good, sensei!The
chainfield is properly hidden and uses the appropriate parser.
189-190: Verify the removal of l1_provider_url.The
l1_provider_urlhas been removed from the config construction. Please ensure this doesn't affect any L1-related functionality.crates/katana/core/src/service/block_producer.rs (1)
582-582: Ohayo! Nice simplification of version management, sensei!Using
CURRENT_STARKNET_VERSIONconstant instead of getting it from chain spec makes the code cleaner and ensures version consistency.crates/katana/rpc/rpc/src/starknet/mod.rs (1)
627-627: Consistent version management across RPC handlers, nice work sensei!The changes align with the block producer implementation by using
CURRENT_STARKNET_VERSIONconsistently in all block header constructions.Also applies to: 693-693, 757-757
crates/dojo/test-utils/Cargo.toml (1)
9-19: Ohayo! Clean dependency organization, sensei!The addition of
katana-chain-specand reordering of dependencies improves the maintainability of the package.crates/katana/cli/Cargo.toml (1)
9-9: Nice architectural alignment, sensei!The changes properly integrate the new chain-spec crate:
- Added as a direct dependency
- Updated slot feature to use the controller from chain-spec instead of primitives
Also applies to: 39-39
bin/katana/Cargo.toml (1)
11-11: Ohayo! Dependencies look good, sensei! 🎋The addition of
katana-chain-specaligns with the PR objectives of incorporating settlement layer information. The removal ofdirsandserdein favor of workspace-level dependencies helps maintain a cleaner dependency tree.Also applies to: 26-26
crates/katana/storage/provider/Cargo.toml (1)
10-10: Looking sharp, sensei! 🗡️The addition of
katana-chain-specto the provider crate makes sense as the storage provider likely needs access to chain specification information.crates/katana/executor/Cargo.toml (1)
24-29: Ohayo! Nice organization, sensei! 📚Adding
katana-chain-specto dev-dependencies suggests it's being used for testing scenarios. The reordering of dependencies maintains alphabetical order, which is a good practice.crates/katana/core/Cargo.toml (1)
10-10: Excellent addition, sensei! 🌟The core module requiring
katana-chain-specis crucial as it likely needs to reference chain specifications for its core operations.Cargo.toml (2)
19-19: Ohayo! The new workspace member path looks good, sensei! ٩(◕‿◕｡)۶The path
crates/katana/chain-specfollows the established workspace structure pattern.
97-97: The workspace dependency declaration is properly configured! (｀・ω・´)The path dependency
katana-chain-spec = { path = "crates/katana/chain-spec" }correctly matches the workspace member path.However, let's verify if all necessary dependencies for the chain-spec crate are available in the workspace:
✅ Verification successful
Ohayo sensei! All dependencies are properly configured! (｀・ω・´)
The chain-spec crate's dependencies are all correctly declared in the workspace, with only one direct dependency (
dirs = "6.0.0") which is perfectly fine to keep outside workspace management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all dependencies required by chain-spec are declared in workspace # Expected: All dependencies should be found in workspace.dependencies section # Get chain-spec's Cargo.toml content cat crates/katana/chain-spec/Cargo.toml | grep '^\w.*=' || echo "Chain spec Cargo.toml not found" # Compare with workspace dependencies echo "Checking against workspace dependencies:" cat Cargo.toml | grep -A 200 '^\[workspace\.dependencies\]' | grep '=' || echo "No workspace dependencies found"Length of output: 8154
| ConfigReadError(#[from] toml::ser::Error), | ||
|
|
||
| #[error("failed to write config file: {0}")] | ||
| ConfigWriteError(#[from] toml::de::Error), |
There was a problem hiding this comment.
Ohayo sensei! Correct error types in Error enum variants
The ConfigReadError and ConfigWriteError variants have their error types swapped. ConfigReadError should use toml::de::Error for deserialization errors, and ConfigWriteError should use toml::ser::Error for serialization errors.
Apply this diff to correct the error types:
Committable suggestion skipped: line range outside the PR's diff.
| #[cfg(feature = "controller")] | ||
| ( | ||
| CONTROLLER_CLASS_HASH, | ||
| GenesisClass { | ||
| compiled_class_hash: CONTROLLER_CLASS_HASH, | ||
| compiled_class_hash: CONTROLLER_ACCOUNT_CLASS | ||
| .clone() | ||
| .compile() | ||
| .expect("failed to compile") | ||
| .class_hash() | ||
| .expect("failed to compute class hash"), |
There was a problem hiding this comment.
Consider handling compilation errors gracefully, sensei!
The current implementation uses expect which could cause panics at runtime. Consider handling these errors explicitly:
- Class compilation failure
- Class hash computation failure
Here's a suggested approach:
- compiled_class_hash: CONTROLLER_ACCOUNT_CLASS
- .clone()
- .compile()
- .expect("failed to compile")
- .class_hash()
- .expect("failed to compute class hash"),
+ compiled_class_hash: {
+ let compiled = CONTROLLER_ACCOUNT_CLASS
+ .clone()
+ .compile()
+ .map_err(|e| format!("Failed to compile controller class: {}", e))?;
+ compiled
+ .class_hash()
+ .map_err(|e| format!("Failed to compute controller class hash: {}", e))?
+ },📝 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.
| #[cfg(feature = "controller")] | |
| ( | |
| CONTROLLER_CLASS_HASH, | |
| GenesisClass { | |
| compiled_class_hash: CONTROLLER_CLASS_HASH, | |
| compiled_class_hash: CONTROLLER_ACCOUNT_CLASS | |
| .clone() | |
| .compile() | |
| .expect("failed to compile") | |
| .class_hash() | |
| .expect("failed to compute class hash"), | |
| #[cfg(feature = "controller")] | |
| ( | |
| CONTROLLER_CLASS_HASH, | |
| GenesisClass { | |
| compiled_class_hash: { | |
| let compiled = CONTROLLER_ACCOUNT_CLASS | |
| .clone() | |
| .compile() | |
| .map_err(|e| format!("Failed to compile controller class: {}", e))?; | |
| compiled | |
| .class_hash() | |
| .map_err(|e| format!("Failed to compute controller class hash: {}", e))? | |
| }, |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2947 +/- ##
==========================================
+ Coverage 55.76% 55.90% +0.13%
==========================================
Files 445 446 +1
Lines 57657 57814 +157
==========================================
+ Hits 32153 32321 +168
+ Misses 25504 25493 -11 ☔ View full report in Codecov by Sentry. |
this includes bunch of improvement to the
ChainSpecstruct, to include information about the settlement layerSummary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
New Features
katana-chain-specpackage for managing chain specificationsImprovements
Dependencies
katana-chain-specBreaking Changes
l1_provider_urlconfiguration optionChainSpecand related types