refactor(katana): simplify genesis class #2948
Conversation
Pull Request AnalysisOhayo, sensei! Let's dive into the details of this intriguing code transformation! WalkthroughThis pull request introduces a significant refactoring of class handling across multiple Katana crates. The primary focus is on simplifying the representation of contract classes by removing the Changes
Possibly related PRs
The changes reflect a trend towards simplification and more direct data representation in the Katana project's codebase. 🪧 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: 0
🧹 Nitpick comments (3)
crates/katana/primitives/src/genesis/json.rs (2)
298-318: Ohayo sensei, fallback logic for contract parsing
The code attempts to parse a Sierra contract and, if that fails, parses a legacy contract. This dual parsing is effective but might induce a performance overhead. Consider optimizing detection if necessary.
458-460: Ohayo sensei, serializing class artifacts
Iterating over classes to serialize them ensures consistent storage of both Sierra and legacy contracts. The approach appears solid for typical use cases.crates/katana/chain-spec/src/lib.rs (1)
130-131: Ohayo sensei! Avoid multiple unwrap calls here.
Chained unwrap calls can lead to runtime panics if the class fails to compile. Consider handling errors more gracefully.Here’s a potential refactor:
- let casm_hash = class.as_ref().clone().compile().unwrap().class_hash().unwrap(); + let compiled = class.as_ref().clone().compile()?; + let casm_hash = compiled.class_hash()?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/katana/chain-spec/src/lib.rs(2 hunks)crates/katana/controller/src/lib.rs(1 hunks)crates/katana/primitives/src/genesis/json.rs(10 hunks)crates/katana/primitives/src/genesis/mod.rs(3 hunks)crates/katana/storage/provider/src/test_utils.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
🔇 Additional comments (22)
crates/katana/primitives/src/genesis/json.rs (12)
20-20: Ohayo sensei, nice import!
This enables better error handling for class hashing and JSON parsing. No concerns here.
27-27: Ohayo sensei, consistent usage of constants
ReferencingDEFAULT_ACCOUNT_CLASSandDEFAULT_ACCOUNT_CLASS_HASHensures uniformity across the code.
30-30: Ohayo sensei, good import for classes
Pulling inContractClassandSierraContractClassaligns with the refactored design.
32-32: Ohayo sensei, parse_deprecated_compiled_class
Including this utility cleanly supports legacy contract formats.
278-278: Ohayo sensei, simpler data structure for classes
UsingArc<ContractClass>directly streamlines class management by removing extra abstractions.
281-284: Ohayo sensei, conditional controller insertion
Conditionally inserting the controller class is orderly. Ensure to confirm no unexpected side effects if the feature is disabled.
287-287: Ohayo sensei, destructuring of GenesisClassJson
Neat approach that eliminates the now-unusedclass_hashfield. It’s more concise.
335-335: Ohayo sensei, final class insertion
Directly storing the computed class hash andArc<ContractClass>is clear and efficient.
366-366: Ohayo sensei, default account class insertion
Gracefully inserts the default account class when none is specified. Well-handled fallback.
754-758: Ohayo sensei, enumerating expected default classes in tests
Including fee token, universal deployer, and account class checks ensures robust coverage.
879-879: Ohayo sensei, verifying actual vs. expected classes
An assert like this is crucial for confirming correctness of the loaded classes. Good thoroughness.
933-935: Ohayo sensei, validating default/controller classes
Ensures that both the default account class and the controller class (if enabled) match the expected configuration.Also applies to: 972-972
crates/katana/storage/provider/src/test_utils.rs (2)
12-12: Ohayo sensei, streamlined imports
DroppingGenesisClassin favor of direct references toGenesisaligns with the simplified architecture.
53-53: Ohayo sensei, direct Arc usage
Wrapping the parsed class in anArcis consistent with the rest of the changes removingGenesisClass. Nicely done.crates/katana/primitives/src/genesis/mod.rs (4)
16-16: Ohayo sensei, importing multiple default constants
Centralizing these constants in a single location is beneficial for code clarity and maintainability.
20-20: Ohayo sensei, shifting from GenesisClass
UsingContractClassdirectly indicates a more cohesive design.
40-40: Ohayo sensei, adopting Arc
This approach cuts down on unnecessary wrappers and keeps the code simpler overall.
85-85: Ohayo sensei, default classes inlined
Inserting the default, legacy, and optional controller classes directly improves readability. This is a clean improvement.Also applies to: 87-87, 89-89, 91-91
crates/katana/controller/src/lib.rs (1)
118-119: Ohayo sensei! Removal of theclass_hashfield looks neat.
This aligns perfectly with storing classes directly instead of referencing the hash.crates/katana/chain-spec/src/lib.rs (3)
127-127: Ohayo sensei! Simplified check for legacy classes.
Switching fromclass.class.is_legacy()toclass.is_legacy()is more direct and clear.
134-134: Ohayo sensei! Direct insertion intostates.classesis concise.
This approach reflects the new architecture of storing classes directly inGenesis, which simplifies management.
327-331: Ohayo sensei! Using the new direct approach for default classes.
Replacing references toGenesisClasswith.into()to store classes directly keeps consistency across the codebase.
Simplify how we handle contract classes in the genesis. We removed the responsibility of computing the compiled class hash from the genesis and delegate it fully to the provider level. This will avoid having mismatched compiled class hash in the case where the genesis type is hand-crafted - essentially reducing places where invalid things could potentially happen :)