feat(katana): simplify genesis class #2948
Conversation
WalkthroughOhayo, sensei! The pull request introduces significant refactoring in the Katana project's class and genesis management. The changes primarily focus on simplifying class representation by removing the Changes
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: 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.
kariy
force-pushed
the
katana/genesis-class
branch
from
22cba42 to
d50e44f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/katana/contracts/legacy/universal_deployer.cairo (1)
24-33: Ohayo sensei! Nice multiline function signature.
Splitting out the arguments onto separate lines enhances readability. Consider adding a docstring to clarify each parameter’s usage and constraints.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/katana/chain-spec/src/lib.rs(3 hunks)crates/katana/contracts/legacy/universal_deployer.cairo(3 hunks)crates/katana/primitives/src/genesis/json.rs(18 hunks)crates/katana/primitives/src/genesis/test-genesis-with-class.json(3 hunks)crates/katana/primitives/src/genesis/test-genesis.json(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/katana/primitives/src/genesis/json.rs
[error] 979-987: Code formatting issue: Incorrect formatting of assert! macro call
[error] 1026-1031: Code formatting issue: Incorrect formatting of assert! macro call
🔇 Additional comments (15)
crates/katana/primitives/src/genesis/test-genesis.json (3)
16-16: Ohayo! Nice improvement in class reference readability sensei!Changing from a hash value to a descriptive name makes the configuration more maintainable and less error-prone.
49-49: Maintaining consistency in class naming, excellent work sensei!The change to use "Foo" here maintains consistency with the account configuration above.
62-62: Clean simplification of class definition sensei!Removing the explicit class hash and using just the name aligns well with the PR's goal of delegating hash computation to the provider level.
crates/katana/chain-spec/src/lib.rs (4)
127-134: Clean refactoring of class handling logic sensei!The simplified approach to class hash computation is more direct and removes unnecessary abstraction layers. The code is now more maintainable and easier to understand.
327-331: Nice test adaptation sensei!The test setup has been properly updated to use the new class representation with Arc.
501-501: Test assertion properly updated sensei!The assertion now correctly reflects the new approach to class hash computation.
Line range hint
881-881: Clean test assertion sensei!The simplified class comparison makes the test more readable and maintainable.
crates/katana/primitives/src/genesis/json.rs (1)
Line range hint
278-335: Excellent refactoring of class handling logic sensei!The simplified approach with direct use of Arc improves code clarity while maintaining robust error handling for duplicate class names.
crates/katana/primitives/src/genesis/test-genesis-with-class.json (4)
16-16: Ohayo! Nice improvement in readability sensei!Replacing the hex value
0x80085with the descriptive nameFoomakes the configuration more maintainable and easier to understand.
44-44: Clean refactoring sensei!The changes from "classHash" to "name" with descriptive values improve the configuration's clarity in two ways:
- The field name better represents its purpose
- The values are human-readable
Also applies to: 48-48
Line range hint
51-4639: Sierra program looks good sensei!The bytecode section is appropriately left unchanged as it needs to remain in its machine-readable format.
Line range hint
32-39: Should we also update the class references here sensei?While we've updated class references to use descriptive names in the accounts section, the contracts section still uses hex values (
0x8). Consider whether we should maintain consistency across the configuration.crates/katana/contracts/legacy/universal_deployer.cairo (3)
19-19: Ohayo sensei! Including the salt parameter in the event is a neat improvement.
This change ensures that relevant deployment details are fully captured and logged.
68-68: Ohayo sensei! Looks consistent with the event definition.
Emitting thesaltparameter here helps maintain a uniform event schema.
2-2: Ohayo sensei! Double-check the version rollback.
Downgrading from v0.6.1 to v0.5.1 might introduce unexpected side effects, so please verify all references and dependencies pointing to OpenZeppelin Cairo contracts.Kindly run the following script to search for references to v0.6.1 mentions across the codebase:
| assert!(Genesis::try_from(json) | ||
| .unwrap_err() | ||
| .to_string() | ||
| .contains("Unresolved class artifact path")); |
There was a problem hiding this comment.
Let's fix the formatting issue sensei!
The multi-line assert! macro needs proper formatting according to the pipeline failure.
Apply this formatting fix:
- assert!(Genesis::try_from(json)
- .unwrap_err()
- .to_string()
- .contains("Unresolved class artifact path"));
+ assert!(
+ Genesis::try_from(json)
+ .unwrap_err()
+ .to_string()
+ .contains("Unresolved class artifact path")
+ );📝 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.
| assert!(Genesis::try_from(json) | |
| .unwrap_err() | |
| .to_string() | |
| .contains("Unresolved class artifact path")); | |
| assert!( | |
| Genesis::try_from(json) | |
| .unwrap_err() | |
| .to_string() | |
| .contains("Unresolved class artifact path") | |
| ); |
| assert!(res | ||
| .unwrap_err() | ||
| .to_string() | ||
| .contains(&format!("Class name '{name}' already exists"))) |
There was a problem hiding this comment.
One more formatting fix needed sensei!
Similar formatting issue with the assert! macro.
Apply this formatting fix:
- assert!(res
- .unwrap_err()
- .to_string()
- .contains(&format!("Class name '{name}' already exists")))
+ assert!(
+ res.unwrap_err()
+ .to_string()
+ .contains(&format!("Class name '{name}' already exists"))
+ );📝 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.
| assert!(res | |
| .unwrap_err() | |
| .to_string() | |
| .contains(&format!("Class name '{name}' already exists"))) | |
| assert!( | |
| res.unwrap_err() | |
| .to_string() | |
| .contains(&format!("Class name '{name}' already exists")) | |
| ); |
kariy
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2948 +/- ##
==========================================
- Coverage 55.85% 55.81% -0.05%
==========================================
Files 447 447
Lines 57859 57735 -124
==========================================
- Hits 32319 32226 -93
+ Misses 25540 25509 -31 ☔ View full report in Codecov by Sentry. |
kariy
force-pushed
the
katana/genesis-class
branch
from
1ad19e0 to
d575423
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/chain-spec/src/lib.rs (1)
127-134: Consider improving error handling sensei!While the class handling logic is cleaner, the multiple
unwrap()calls on lines 130-131 could be replaced with proper error handling to make the code more robust.- let casm_hash = class.as_ref().clone().compile().unwrap().class_hash().unwrap(); - states.state_updates.declared_classes.insert(class_hash, casm_hash); + match class.as_ref().clone().compile().and_then(|c| c.class_hash()) { + Ok(casm_hash) => { + states.state_updates.declared_classes.insert(class_hash, casm_hash); + } + Err(e) => { + log::error!("Failed to compile class {}: {}", class_hash, e); + // Handle error appropriately + } + }crates/katana/primitives/src/genesis/json.rs (1)
283-286: Consider documenting the temporary solution better sensei!The comment indicates this is a "band aid fix", but it would be helpful to:
- Document the specific k8s ConfigMap limitation (1MiB)
- Create a tracking issue for implementing a permanent solution
- Add more context about why this approach was chosen
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/katana/chain-spec/src/lib.rs(3 hunks)crates/katana/controller/src/lib.rs(1 hunks)crates/katana/primitives/src/class.rs(2 hunks)crates/katana/primitives/src/genesis/json.rs(18 hunks)crates/katana/primitives/src/genesis/mod.rs(3 hunks)crates/katana/primitives/src/genesis/test-genesis.json(3 hunks)crates/katana/storage/provider/src/test_utils.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/katana/controller/src/lib.rs
- crates/katana/storage/provider/src/test_utils.rs
- crates/katana/primitives/src/class.rs
- crates/katana/primitives/src/genesis/test-genesis.json
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ensure-wasm
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (5)
crates/katana/primitives/src/genesis/mod.rs (2)
40-40: Ohayo sensei! Nice simplification of the classes field!The change to directly map
ClassHashtoArc<ContractClass>improves code clarity by removing the intermediateGenesisClassstruct while maintaining thread-safety withArc.
85-91: Clean default implementation sensei!The default implementation is now more straightforward with direct
Arcwrapping of contract classes.crates/katana/chain-spec/src/lib.rs (1)
327-331: Well-maintained test coverage sensei!The test setup has been properly updated to reflect the new class handling structure while maintaining good coverage.
crates/katana/primitives/src/genesis/json.rs (2)
Line range hint
289-321: Excellent class parsing implementation sensei!The code cleanly handles both Sierra and Legacy contract classes with proper error handling and clear separation of concerns.
720-739: Great test coverage sensei!The tests thoroughly verify the new class handling with good coverage of different scenarios and properly formatted assertions.
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 :)
Summary by CodeRabbit
Release Notes
Refactor
GenesisClassstructChanges
Code Structure