refactor(katana): remove starknet version from chainspec#2875
refactor(katana): remove starknet version from chainspec#2875kariy merged 1 commit intokatana/chainspecfrom
Conversation
Pull Request Analysis 🚀Ohayo, sensei! Let's dive into this epic refactoring adventure! 🌟 WalkthroughThis pull request introduces a significant reorganization of the chain specification management in the Katana project. The primary focus is on creating a new Changes
Possibly Related PRs
Key Observations
The changes represent a significant architectural refactoring, centralizing chain specification management and improving the overall modularity of the Katana project. Sensei would be proud of this clean, strategic code restructuring! 🥷🏼🔧 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (31)
✅ Files skipped from review due to trivial changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (41)
🪧 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: 5
🧹 Nitpick comments (10)
crates/katana/primitives/src/genesis/mod.rs (1)
126-135: Ohayo, sensei! Handling potential compilation errorsUsing
expectwith descriptive messages is acceptable here since the controller account class is known and expected to compile without errors. However, to enhance robustness, consider handling possible errors gracefully without causing a panic.Here's a suggested refactor:
compiled_class_hash: { let compiled_class = CONTROLLER_ACCOUNT_CLASS .clone() .compile() - .expect("failed to compile"); + .unwrap_or_else(|e| { + // Handle the compilation error + log::error!("Compilation failed: {:?}", e); + // Provide a default or abort initialization as necessary + }); compiled_class .class_hash() - .expect("failed to compute class hash") + .unwrap_or_else(|e| { + // Handle the class hash computation error + log::error!("Class hash computation failed: {:?}", e); + // Provide a default or abort initialization as necessary + }) },bin/katana/src/cli/init/mod.rs (2)
131-136: Ohayo, sensei! Noted the TODO for fee token implementationThe commented-out code for the fee token indicates future implementation plans. If you'd like assistance in implementing this feature, I'm happy to help. Would you like me to generate the necessary code or open a GitHub issue to track this task?
308-316: Ohayo, sensei! Consider usingonce_celloverlazy_staticReplacing
lazy_staticwithonce_cell::sync::Lazyis recommended for initializing static data due to its simplicity and efficiency.Here's how you might refactor the code:
-use lazy_static::lazy_static; +use once_cell::sync::Lazy; -lazy_static! { - static ref GENESIS: Genesis = { +static GENESIS: Lazy<Genesis> = Lazy::new(|| { // master account let accounts = DevAllocationsGenerator::new(1).generate(); let mut genesis = Genesis::default(); genesis.extend_allocations(accounts.into_iter().map(|(k, v)| (k, v.into()))); genesis - }; -} +});crates/katana/chain-spec/src/lib.rs (2)
102-114: Ensure robust error handling inloadmethod.The
loadmethod reads from external files and deserializes content, which can be error-prone. Consider enhancing error messages with context to aid in debugging, and ensure that all possible errors are gracefully handled.
203-210: Ohayo sensei! Consider adding#[serde(default)]to optional fields inChainSpecFile.Adding
#[serde(default)]to fields likesettlementensures default values are used when fields are missing during deserialization, enhancing robustness.crates/katana/primitives/src/genesis/json.rs (2)
806-816: Ohayo sensei! Ensure error handling in test class compilation.In the tests, compiling
CONTROLLER_ACCOUNT_CLASScould potentially fail. It's important to handle any compilation errors to prevent test flakiness.
944-958: Consider enhancing round-trip conversion tests.The
genesis_conversion_rttest is valuable. Expanding it to include various genesis configurations will strengthen confidence in the serialization and deserialization processes.crates/katana/primitives/src/contract.rs (1)
25-28: Ohayo! Nice addition of convenience constants, sensei! 🎭The new constants provide a clean way to access commonly used addresses. However, consider adding documentation to explain their intended usage.
Add documentation like this:
+ /// Represents the zero address (0x0) pub const ZERO: Self = Self(Felt::ZERO); + /// Represents the one address (0x1) pub const ONE: Self = Self(Felt::ONE); + /// Represents the two address (0x2) pub const TWO: Self = Self(Felt::TWO); + /// Represents the three address (0x3) pub const THREE: Self = Self(Felt::THREE);crates/katana/core/src/service/block_producer.rs (1)
582-582: Clean version handling update, sensei!Replacing
backend.chain_spec.versionwithCURRENT_STARKNET_VERSIONcentralizes version management and removes the dependency on chain-specific versions.Consider documenting the version management strategy in the project's architecture documentation to help maintainers understand this centralization decision.
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
19-19: Ohayo sensei! Nice work on standardizing the Starknet version handling.The change to use
CURRENT_STARKNET_VERSIONinstead of fetching from chain_spec makes the version handling more consistent across all block types. This is a good architectural decision that reduces configuration complexity.Also applies to: 617-617, 683-683, 747-747
🛑 Comments failed to post (5)
crates/katana/node/src/lib.rs (1)
27-30: 🛠️ Refactor suggestion
Ohayo, sensei! Avoid importing from test utilities in production code
Importing constants from
test_utilsis not recommended for production code. Consider movingDEFAULT_ETH_L1_GAS_PRICE,DEFAULT_ETH_L1_DATA_GAS_PRICE,DEFAULT_STRK_L1_GAS_PRICE, andDEFAULT_STRK_L1_DATA_GAS_PRICEto a production module or constants file.Here's how you might adjust the imports:
-use katana_executor::implementation::blockifier::blockifier::test_utils::{ +use katana_executor::implementation::blockifier::constants::{ DEFAULT_ETH_L1_DATA_GAS_PRICE, DEFAULT_ETH_L1_GAS_PRICE, DEFAULT_STRK_L1_DATA_GAS_PRICE, DEFAULT_STRK_L1_GAS_PRICE, };Committable suggestion skipped: line range outside the PR's diff.
bin/katana/src/cli/init/mod.rs (1)
274-276: 🛠️ Refactor suggestion
Ohayo, sensei! Change in configuration file format
The configuration file extension has been changed from
.tomlto.json. Ensure that all documentation, scripts, and tools that interact with the configuration file are updated to reflect this change.crates/katana/chain-spec/src/lib.rs (1)
473-473: 🛠️ Refactor suggestion
Ohayo sensei! Expand test coverage for
settlementvariants.Currently, tests primarily cover scenarios where
settlementisNone. Including tests with actualSettlementLayervalues will ensure that all functionality is properly verified.crates/katana/primitives/src/genesis/json.rs (1)
318-339:
⚠️ Potential issueHandle class deserialization errors gracefully.
When attempting to deserialize contract classes, if both
SierraContractClassandLegacyContractClassfail, consider providing informative error messages to aid in troubleshooting.crates/katana/primitives/src/lib.rs (1)
9-9:
⚠️ Potential issueOhayo sensei! Confirm removal of
chain_specmodule is accounted for.The
chain_specmodule has been removed and replaced withkatana-chain-speccrate. Ensure all references tokatana_primitives::chain_specare updated to prevent import errors.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## katana/chainspec #2875 +/- ##
====================================================
+ Coverage 55.74% 55.85% +0.10%
====================================================
Files 446 447 +1
Lines 57818 57904 +86
====================================================
+ Hits 32233 32342 +109
+ Misses 25585 25562 -23 ☔ View full report in Codecov by Sentry. |
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
1 participant
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.