fix(katana): genesis deprecated declared classes in state updates #2893
Conversation
kariy
changed the title
Pull Request AnalysisOhayo, sensei! Here's a comprehensive breakdown of the changes: WalkthroughThe pull request introduces modifications to class management and state updates across multiple Rust files. The primary focus is on distinguishing between legacy and non-legacy classes, adding a new Changes
Sequence DiagramsequenceDiagram
participant ChainSpec
participant ContractClass
participant DbProvider
participant HistoricalStateProvider
ChainSpec->>ContractClass: Check class type (is_legacy/is_class)
ContractClass-->>ChainSpec: Return boolean
ChainSpec->>DbProvider: Insert block with state updates
DbProvider->>HistoricalStateProvider: Verify class declaration
HistoricalStateProvider-->>DbProvider: Confirmation of class status
Possibly Related PRs
Suggested Reviewers
Sensei, I hope this breakdown helps illuminate the intricacies of the pull request! Let me know if you need any further clarification. Ohayo! 🍵🥷 🪧 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 (1)
crates/katana/primitives/src/class.rs (1)
60-64: Consider adding migration guidance, sensei! 📚Since this change is part of fixing deprecated declared classes, it would be helpful to add a doc comment explaining the migration path from
as_legacy()tois_legacy()for downstream users.Example doc addition:
/// Checks if this contract class is a Cairo 0 legacy class. /// /// Returns `true` if the contract class is a legacy class, `false` otherwise. /// /// Note: This replaces the previous `as_legacy()` method as part of the deprecated classes cleanup. /// If you need to access the underlying legacy class, use pattern matching instead: /// ``` /// match contract_class { /// ContractClass::Legacy(legacy) => // handle legacy class /// ContractClass::Class(_) => // handle Sierra class /// } /// ```
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/katana/core/Cargo.toml(2 hunks)crates/katana/primitives/src/class.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
crates/katana/core/Cargo.toml (2)
39-39: Ohayo! Nice addition of the optional dependency, sensei!The addition of
starknet-cryptoas an optional dependency aligns well with the modular design principles. This approach ensures that the crypto functionality is available only when needed for handling legacy classes in state updates.
53-53: Clean feature definition, sensei!The
starknet-messagingfeature now correctly depends onstarknet-crypto, which makes sense as messaging operations might require cryptographic capabilities when dealing with state updates.Let's verify if any other features might need the crypto dependency:
✅ Verification successful
Ohayo! Your feature definition is spot-on, sensei!
After thorough verification, the
starknet-messagingfeature correctly depends onstarknet-crypto. Other features in the codebase either have direct crypto dependencies where needed or don't require crypto capabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential crypto-related functionality in other features rg -l "crypto|sign|verify|hash" --type rust | grep -v "messaging"Length of output: 11799
Script:
#!/bin/bash # Find all Cargo.toml files and search for starknet/crypto related features and dependencies fd Cargo.toml --type f --exec sh -c 'echo "=== {} ==="; grep -A 5 -B 5 "features\|starknet\|crypto" {} || true'Length of output: 40768
crates/katana/primitives/src/class.rs (1)
60-64: Ohayo! Clean implementation ofis_legacy, sensei! 🎋The change from
Option<&T>to a boolean simplifies the API while maintaining functionality. However, I notice that the counterpartis_classmethod mentioned in the summary is missing.Let's verify the impact of removing
as_legacyon existing callers:Could you also add the
is_classmethod for consistency? Here's a suggested implementation:/// Checks if this contract class is a Sierra class. /// /// Returns `true` if the contract class is a Sierra class, `false` otherwise. pub fn is_class(&self) -> bool { matches!(self, Self::Class(_)) }✅ Verification successful
Ohayo! The removal of
as_legacyis safe, sensei! 🎋The codebase shows no remaining usage of the removed method, and all legacy class handling is done through pattern matching and direct variants. The new
is_legacyimplementation is clean and idiomatic.Please add the suggested
is_classmethod for API consistency:pub fn is_class(&self) -> bool { matches!(self, Self::Class(_)) }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of as_legacy that need updating rg "as_legacy\(\)" --type rustLength of output: 32
Script:
#!/bin/bash # Search for any form of as_legacy usage rg "as_legacy" --type rust # Look for the old implementation pattern ast-grep --pattern 'fn as_legacy' # Also check for potential uses of LegacyContractClass rg "LegacyContractClass" --type rust -A 2Length of output: 14561
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2893 +/- ##
==========================================
+ Coverage 55.81% 55.83% +0.01%
==========================================
Files 449 449
Lines 57697 57700 +3
==========================================
+ Hits 32206 32219 +13
+ Misses 25491 25481 -10 ☔ View full report in Codecov by Sentry. |
Put legacy classes in the correct mappings in the
StateUpdatesstruct.Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
starknet-cryptoand updated feature requirements forstarknet-messaging.