refactor(katana): abstract compiled class thru the provider level #2709
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications across various files, primarily focusing on restructuring workspace members and dependencies in Changes
Possibly related PRs
🪧 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: 19
🧹 Outside diff range and nitpick comments (50)
crates/katana/primitives/src/utils/class.rs (2)
16-20: Consider optimizing the parsing fallback mechanismThe current implementation attempts to parse as v1 first, then falls back to deprecated format. While this provides good compatibility, it could lead to unnecessary parsing attempts for legacy classes.
Given that the PR mentions an expected regression during declare transactions, this might be another area where performance could be optimized in the future.
Consider adding a format identifier check before parsing to avoid the double parsing attempt:
pub fn parse_compiled_class(artifact: Value) -> Result<CompiledClass> { + // Check for format identifier first + if let Some(format_type) = artifact.get("format_type") { + match format_type.as_str() { + Some("v1") => return Ok(CompiledClass::Class(parse_compiled_class_v1(artifact)?)), + _ => return Ok(CompiledClass::Legacy(parse_deprecated_compiled_class(artifact)?)), + } + } if let Ok(casm) = parse_compiled_class_v1(artifact.clone()) { Ok(CompiledClass::Class(casm)) } else { Ok(CompiledClass::Legacy(parse_deprecated_compiled_class(artifact)?)) } }
23-25: Document the use of usize::MAX parameterThe use of
usize::MAXas the third parameter tofrom_contract_classshould be documented to explain why we're not using a specific size limit (like the commented outMAX_BYTECODE_SIZE).Consider adding a doc comment:
+/// Parses a contract class from its JSON representation into a CasmContractClass. +/// Uses usize::MAX as size limit to effectively disable size validation, as size +/// constraints are handled at the provider level. pub fn parse_compiled_class_v1(class: Value) -> Result<CasmContractClass> {crates/saya/provider/src/provider.rs (1)
31-31: Document the known performance regression, sensei!The PR introduces a known performance regression for declare transactions (class needs to be compiled twice). This should be documented in the method's documentation to inform implementors.
Add a note in the documentation:
/// Fetches the state updates related to a given block. /// Returns the [`StateUpdatesWithDeclaredClasses`] and the serialiazed /// state update for data availability layer. /// /// # Arguments /// /// * `block_number` - The block to fetch. + /// + /// # Note + /// There is a known performance impact for declare transactions where classes + /// need to be compiled twice: once during execution and again when storing + /// the state update in the database. async fn fetch_state_updates(crates/katana/storage/provider/src/traits/contract.rs (1)
8-12: Excellent architectural separation, sensei!The split between
class()andcompiled_class()methods provides a clear separation of concerns:
class()now returns the contract class in its base formcompiled_class()specifically handles the compiled representationThis aligns perfectly with the PR's goal of deferring compilation operations to the provider level.
crates/katana/storage/provider/src/test_utils.rs (2)
54-56: Ohayo! Acknowledging performance trade-off, sensei!The new class creation flow aligns with the architectural changes but introduces the expected double compilation during declare transactions. As mentioned in the PR objectives, this is an intentional trade-off for code simplification, with optimization planned for the future.
Consider adding a comment documenting this known performance implication for future reference.
53-53: Ohayo! Consider implementing the genesis builder, sensei!The TODO comment suggests building a genesis builder to simplify this setup process.
Would you like me to help create a GitHub issue to track the implementation of a genesis builder? This could help streamline the test setup process and make it more maintainable.
crates/saya/core/src/blockchain/mod.rs (1)
123-123: Update method documentation, sensei!The parameter type change looks good, but the documentation comment above the method still references
StateUpdatesWithDeclaredClasses. Please update it to match the new type.-/// and the associated [`StateUpdatesWithDeclaredClasses`]. +/// and the associated [`StateUpdatesWithClasses`].crates/katana/executor/src/implementation/noop.rs (1)
130-146: Ohayo! Implementation aligns well with the abstraction goals!The changes to
ContractClassProviderimplementation nicely reflect the PR's objective of deferring class operations to the provider level:
class()now returnsContractClassinstead ofCompiledClass, providing a more abstract representationcompiled_class()separates the compilation concerncompiled_class_hash_of_class_hash()maintains hash lookup capabilityThe no-op implementations correctly preserve the pattern of returning
Nonefor all operations.While this no-op implementation looks good, keep in mind that real implementations will need to handle the double compilation scenario mentioned in the PR objectives. Consider adding a TODO comment about potential future optimizations to avoid this overhead.
crates/katana/storage/provider/src/error.rs (1)
99-101: The new error variant looks sharp, sensei!The
ContractClassCompilationvariant with#[error(transparent)]is well-implemented. Consider adding a brief doc comment to explain when this error occurs, similar to other variants in the enum.+ /// Error when compiling a contract class fails. #[error(transparent)] ContractClassCompilation(#[from] ContractClassCompilationError),crates/katana/primitives/src/genesis/mod.rs (1)
33-39: Consider enhancing Debug output for better troubleshootingThe current Debug implementation prints "..." for the class field, which might make debugging more difficult. Consider including some identifying information from the ContractClass, such as its hash or size.
- .field("class", &"...") + .field("class_hash", &self.class.class_hash())crates/katana/storage/provider/src/traits/block.rs (1)
149-152: Heads up about the performance trade-off, sensei!The parameter type change from
StateUpdatesWithDeclaredClassestoStateUpdatesWithClassesis aligned with our architectural goals. However, as mentioned in the PR objectives, this will cause classes to be compiled twice during declare transactions: once during execution and once during storage.While this is an accepted trade-off for now to achieve cleaner code structure, we should consider adding a TODO comment or creating an issue to track the future optimization of this double compilation scenario.
crates/katana/storage/provider/src/providers/in_memory/cache.rs (1)
68-71: Consider performance optimization and error handling improvements.The compilation during insertion could impact performance, especially for large classes. While this aligns with the PR's acknowledged regression, there are two areas for future improvement:
- Performance: Consider adding a compilation cache to prevent recompilation of previously seen classes
- Error Handling: The
expectcall could be replaced with proper error propagationConsider this approach for better error handling:
- let compiled = class.compile().expect("failed to compiled class"); + let compiled = class.compile().map_err(|e| { + format!("Failed to compile class {}: {}", hash, e) + })?;crates/katana/storage/provider/tests/class.rs (2)
22-33: Enhance error handling in class compilation, sensei!While the simplified assertion logic looks good, the error handling in the compilation step could be improved. The current
.expect("fail to compile")might not provide enough context for debugging test failures.Consider this more descriptive error handling:
- let expected_compiled = expected_class.clone().compile().expect("fail to compile"); + let expected_compiled = expected_class.clone().compile().map_err(|e| { + anyhow::anyhow!("Failed to compile class {}: {}", class_hash, e) + })?;
56-58: Comprehensive test coverage, sensei!The test cases effectively validate both the latest and historical states, with good coverage of different scenarios. The progression from empty states to fully populated classes is well structured.
Consider adding a brief comment explaining the test progression through different blocks, like:
+ // Test progression: + // Block 0: No classes declared + // Block 1: ERC20 class declared + // Block 4: UDC class added + // Block 5: World contract addedAlso applies to: 110-112, 118-120, 126-128, 134-136
crates/katana/executor/src/abstraction/mod.rs (1)
171-177: Ohayo! Clean separation of class handling concerns.The changes to
ContractClassProvidernicely implement the architectural shift:
class()now returns the baseContractClass- New
compiled_class()method provides compiled form when needed- Removed direct sierra class access simplifies the interface
Consider adding documentation to explain when to use
class()vscompiled_class()to guide future developers.Add documentation like this:
+ /// Returns the contract class in its base form. fn class(&self, hash: ClassHash) -> ProviderResult<Option<ContractClass>> { self.0.class(hash) } + /// Returns the contract class in its compiled form, computing it if necessary. fn compiled_class(&self, hash: ClassHash) -> ProviderResult<Option<CompiledClass>> { self.0.compiled_class(hash) }crates/katana/storage/provider/tests/fixtures.rs (2)
Line range hint
86-142: Consider enhancing test coverage with different class variants, sensei!While the mock data correctly implements the new structure, we could improve test coverage by including different variants of
ContractClassacross the state updates. Currently, we only testContractClass::Classwith Sierra class, but we could also include cases for other variants to ensure robust testing.Consider adding more variants in the mock data:
let state_update_1 = StateUpdatesWithClasses { // ... - classes: BTreeMap::from([(class_hash_1, DEFAULT_LEGACY_ERC20_CLASS.clone())]), + classes: BTreeMap::from([ + (class_hash_1, ContractClass::Legacy(DEFAULT_LEGACY_ERC20_CLASS.clone())), + ]), };
Line range hint
73-165: Ohayo! The architectural changes look solid, sensei!The refactoring successfully moves class compilation responsibility to the provider level while maintaining comprehensive test coverage. The acknowledged performance regression (double compilation during declare transactions) is an acceptable trade-off for the improved architecture, as it can be optimized in future iterations.
Consider documenting the intentional performance regression in the test file's module-level documentation to help future maintainers understand this design decision.
crates/saya/provider/src/rpc/mod.rs (1)
131-131: Performance consideration regarding class compilation.The simplified state updates structure aligns with the PR objectives. However, as mentioned in the PR description, this change introduces an expected regression where classes will need to be compiled twice: once during execution and again when storing the state update in the database.
While this is an accepted trade-off for code simplification, we should track this for future optimization.
Would you like me to create an issue to track this performance optimization opportunity?
Also applies to: 148-148
Cargo.toml (1)
69-71: The profiling profile configuration aligns with performance considerations!The profile inherits from release while enabling debug symbols, which will be valuable for analyzing the acknowledged performance regression in declare transactions.
Consider using this profile to measure and optimize the double compilation impact mentioned in the PR objectives.
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
Line range hint
177-199: Ohayo! The architectural changes look solid, sensei!The refactoring successfully achieves the goal of deferring class compilation to the provider level while maintaining proper state management and error handling. The changes are well-contained and don't disrupt the existing transaction execution flow.
A few architectural points to consider:
- The separation of concerns is improved by moving compilation responsibility to the provider
- The data structure simplification makes the code more maintainable
- The temporary performance regression is a reasonable trade-off for better architecture
crates/katana/rpc/rpc/tests/torii.rs (1)
197-200: Ohayo! Nice improvement to transaction handling, sensei!The destructuring of
DeclareTransactionResultimproves code clarity, and the addition ofTransactionWaiterensures proper transaction completion. However, we could enhance error handling.Consider adding error context:
- let DeclareTransactionResult { transaction_hash, class_hash } = - account.declare_v2(contract.clone(), compiled_class_hash).send().await.unwrap(); + let DeclareTransactionResult { transaction_hash, class_hash } = + account.declare_v2(contract.clone(), compiled_class_hash) + .send() + .await + .expect("Failed to declare contract");crates/katana/storage/provider/src/lib.rs (2)
272-278: Excellent separation of concerns in class handlingThe split between
class()andcompiled_class()methods elegantly implements the provider-level class abstraction. This change:
- Allows lazy compilation of classes
- Provides clear separation between contract definition and its compiled form
- Aligns with the PR's goal of deferring compilation to the provider level
326-326: Writer implementation maintains clean abstraction with noted trade-offsThe separation of
set_class()andset_compiled_class()completes the provider-level abstraction nicely. However, as noted in the PR objectives, this will cause a temporary performance regression during declare transactions due to double compilation.Consider adding a TODO comment to track future optimization opportunities.
+ // TODO: Optimize to prevent double compilation during declare transactions fn set_compiled_class(&self, hash: ClassHash, class: CompiledClass) -> ProviderResult<()> { self.provider.set_compiled_class(hash, class) }Also applies to: 338-339
crates/katana/primitives/src/conversion/rpc.rs (2)
149-149: Consider enhancing error handling, sensei!The simplified implementation is cleaner, but consider adding detailed error context for compilation failures, as they might be harder to debug when deferred to the provider level.
- let class = crate::class::CompiledClass::Class(casm); + let class = crate::class::CompiledClass::Class(casm) + .map_err(|e| anyhow::anyhow!("Failed to create compiled class: {}", e))?;
270-291: Enhance test coverage for error scenarios, sensei!While the happy path is well tested, consider adding test cases for:
- Invalid class formats
- Compilation failures
- Memory/resource constraints during compilation
Example test structure:
#[test] fn test_legacy_rpc_to_class_invalid_format() { let invalid_json = json!({ "abi": [], "program": vec![0u8; 10], // Invalid program data "entry_points_by_type": {} }); let result = legacy_rpc_to_class(&invalid_json); assert!(result.is_err()); }crates/katana/storage/provider/src/providers/fork/state.rs (2)
191-203: Consider optimizing the ForkedSnapshot implementation, sensei!While the implementation is correct, we could potentially optimize the double read pattern in the
classandcompiled_classmethods. Currently, we checkcompiled_class_hashesand then read fromclasses/compiled_classes.Consider combining these operations to reduce the number of reads:
- fn class(&self, hash: ClassHash) -> ProviderResult<Option<ContractClass>> { - if self.inner.compiled_class_hashes.contains_key(&hash) { - Ok(self.classes.classes.read().get(&hash).cloned()) - } else { - ContractClassProvider::class(&self.inner.db, hash) - } - } + fn class(&self, hash: ClassHash) -> ProviderResult<Option<ContractClass>> { + match (self.inner.compiled_class_hashes.contains_key(&hash), + self.classes.classes.read().get(&hash)) { + (true, Some(class)) => Ok(Some(class.clone())), + _ => ContractClassProvider::class(&self.inner.db, hash) + } + }
223-223: Consider adding test cases for class handling, sensei!While the state update tests are well-structured, we should add specific test cases for the new class handling functionality to ensure the abstraction works as intended.
Would you like me to help generate additional test cases for:
- Class retrieval through different providers
- Compiled class caching behavior
- Error cases in class compilation
Also applies to: 258-258, 279-279, 290-290, 311-311, 335-335
crates/katana/core/src/backend/storage.rs (1)
338-338: Consider removing unnecessary.clone()calls, sensei!The
.clone()calls onDEFAULT_LEGACY_*_CLASSconstants are unnecessary in assertions since we're doing equality comparisons. The compiler can handle this without explicit cloning.-assert_eq!(actual_udc_class, DEFAULT_LEGACY_UDC_CLASS.clone()); +assert_eq!(actual_udc_class, DEFAULT_LEGACY_UDC_CLASS); -assert_eq!(actual_fee_token_class, DEFAULT_LEGACY_ERC20_CLASS.clone()); +assert_eq!(actual_fee_token_class, DEFAULT_LEGACY_ERC20_CLASS);Also applies to: 341-341, 364-364, 367-367
crates/katana/storage/db/src/tables.rs (1)
Line range hint
374-390: Ohayo sensei! Consider enhancing the test coverageThe test cases could be improved in two ways:
- Consider using a test data helper function to generate realistic compiled class test data.
- The table type assertions could be made more maintainable by using a loop over
Tables::ALL.Here's a suggested improvement for the table type assertions:
- assert_eq!(Tables::Headers.table_type(), TableType::Table); - assert_eq!(Tables::BlockHashes.table_type(), TableType::Table); - // ... more individual assertions ... + for table in Tables::ALL { + let expected_type = match table { + Tables::ContractStorage => TableType::DupSort, + Tables::ClassDeclarations => TableType::DupSort, + Tables::NonceChangeHistory => TableType::DupSort, + Tables::ClassChangeHistory => TableType::DupSort, + Tables::StorageChangeHistory => TableType::DupSort, + _ => TableType::Table, + }; + assert_eq!( + table.table_type(), + expected_type, + "Table {} has unexpected type", + table + ); + }crates/katana/storage/provider/src/providers/fork/mod.rs (1)
523-529: LGTM! Clean separation of contract and compiled class handling.Ohayo, sensei! The implementation properly separates the concerns:
set_classfor storing the contract classset_compiled_classfor storing the compiled versionNote: As mentioned in the PR objectives, this change introduces an expected regression where classes need to be compiled twice during declare transactions. This is an acceptable trade-off for cleaner abstraction.
Consider adding a cache for compiled classes in a future optimization to address the double compilation issue.
crates/katana/primitives/src/transaction.rs (1)
139-140: LGTM! Simplified class representationOhayo! The consolidation of class representation into a single
Arc<ContractClass>field aligns well with the PR objectives of simplifying data structures. The use ofArcis appropriate for shared ownership.This change contributes to better maintainability by:
- Reducing the number of fields to maintain
- Providing a unified interface for class operations
- Enabling efficient sharing of class data through
Arccrates/saya/core/src/lib.rs (1)
Line range hint
316-324: Note about performance regressionOhayo sensei! As mentioned in the PR objectives, there's an expected performance regression where classes will need to be compiled twice during declare transactions. While this is acceptable for now as we prioritize code simplification, we should track this for future optimization.
Would you like me to create a GitHub issue to track the future optimization of class compilation during declare transactions?
crates/katana/primitives/src/chain_spec.rs (2)
145-150: Ohayo sensei! Consider optimizing fee token class initialization.The current implementation creates a new clone for each fee token. Consider using a reference or lazy initialization pattern.
- .or_insert_with(|| DEFAULT_LEGACY_ERC20_CLASS.clone()); + .or_insert_with(|| Arc::clone(&DEFAULT_LEGACY_ERC20_CLASS));
224-229: Similar optimization needed for UDC class initialization.The UDC class initialization follows the same pattern as fee tokens and could benefit from the same optimization.
- .or_insert_with(|| DEFAULT_LEGACY_UDC_CLASS.clone()); + .or_insert_with(|| Arc::clone(&DEFAULT_LEGACY_UDC_CLASS));crates/katana/primitives/src/class.rs (1)
32-43: Efficient implementation of thecompilemethod.The
compilemethod correctly handles both variants ofContractClass. However, consider adding logging for traceability.Apply this diff to include logging:
+use log::debug; impl ContractClass { /// Compiles the contract class into CASM. pub fn compile(self) -> Result<CompiledClass, ContractClassCompilationError> { match self { Self::Legacy(class) => { + debug!("Compiling legacy contract class."); Ok(CompiledClass::Legacy(class)) } Self::Class(class) => { + debug!("Compiling FlattenedSierraClass."); let class = rpc_to_cairo_contract_class(&class) .map_err(ContractClassCompilationError::from)?; let casm = CasmContractClass::from_contract_class(class, true, usize::MAX)?; Ok(CompiledClass::Class(casm)) } } }crates/katana/storage/provider/src/providers/db/state.rs (2)
73-82: Consider unifying class storage to simplify retrieval.Ohayo, sensei! In the
set_classmethod, you're storingContractClass::Classinstances inSierraClassesandContractClass::Legacyinstances inCompiledClasses. While this reflects the different class types, it introduces complexity during retrieval, as theclassmethod needs to check multiple tables. Consider unifying the storage into a single table or refactoring the data structure to simplify the retrieval logic.
194-203: Reduce code duplication by abstracting common retrieval logic.Ohayo, sensei! The
classmethod inHistoricalStateProvidermirrors the logic inLatestStateProvider. To improve maintainability, consider abstracting the shared retrieval logic into a helper function or trait. This will reduce code duplication and make future updates more manageable.crates/katana/executor/src/implementation/blockifier/state.rs (4)
95-95: Consider removing commented-out code for clarity.The line
// type DeclaredClass = (CompiledClass, ContractClass);is commented out. If it's no longer needed, removing it would keep the codebase clean.
322-324: Simplify nested method calls for readability.Consider assigning the compiled class to a variable before conversion for clarity.
Example:
let compiled_class = DEFAULT_LEGACY_ERC20_CLASS.clone().compile().expect("can compile"); let actual_legacy_class = utils::to_class(compiled_class).unwrap().contract_class();
352-352: Consider removing commented-out code in tests for clarity.The commented line
// let actual_new_sierra_class = sp.sierra_class(new_class_hash)?;may be unnecessary if not in use.
364-364: Remove unused commented code for cleaner tests.The commented assertion can be removed if it's no longer relevant.
crates/katana/storage/provider/src/providers/fork/backend.rs (3)
18-18: Typo in function name within import statement.There's a typo in the imported function name
compiled_class_hash_from_flattened_sierra_class. It should becompiled_class_hash_from_contract_classto match the updated function.Apply this diff to correct the function name:
use katana_primitives::conversion::rpc::{ - compiled_class_hash_from_flattened_sierra_class, legacy_rpc_to_class, + compiled_class_hash_from_contract_class, legacy_rpc_to_class, };
497-500: Correct error logging for consistency.Consider adjusting the error logging to maintain consistency and provide clearer error messages.
Apply this diff to improve the error log:
let (_, class) = legacy_rpc_to_class(&class).map_err(|error| { error!(target: LOG_TARGET, hash = %format!("{hash:#x}"), %error, "Parsing legacy class."); - ProviderError::ParsingError(error.to_string()) + ProviderError::ParsingError(format!("Failed to parse legacy class: {}", error)) })?;
520-522: Fix typo and grammatical error in comments.There's a typo and a grammatical error in the comments. Correcting them will enhance readability.
Apply this diff to correct the comments:
// The Starknet RPC specs don't provide an endpoint for fetching the compiled class. // If the uncompiled version doesn't yet exist locally, we fetch it from the forked -// provider (from the `starknet_getClsas` method) and compile it. +// provider (using the `starknet_getClass` method) and compile it.crates/katana/executor/src/implementation/blockifier/utils.rs (1)
524-534: Ohayo, sensei! Offer assistance on handling contract class conversion errorsI noticed the TODO comment about uncertainty in variant usage and whether to change the return type to handle error cases in contract class conversions. I'm happy to assist in implementing proper error handling or discussing potential solutions.
Would you like me to propose changes to improve error handling in this function?
crates/katana/primitives/src/genesis/json.rs (4)
Line range hint
315-338: Consolidate Error Handling in Class Parsing LogicOhayo, sensei! In the match block starting at line 315, the error handling for parsing Sierra and legacy contracts is duplicated. Consider refactoring the error handling into a helper function or method to reduce code duplication and improve maintainability.
713-739: Reduce Code Duplication in Test CasesOhayo, sensei! Between lines 713-739, the construction of
GenesisClassinstances involves repeated patterns, especially with theflatten().unwrap()calls. To enhance code readability and maintainability, consider abstracting this repetitive code into a helper function within your test module.
863-863: Ensure Consistent Assertion Styles in TestsOhayo, sensei! On line 863, the assertion
assert_eq!(class.1.class, expected_class.class);is used. Please ensure that assertion styles are consistent throughout your test suite for better readability. For example, consistently use eitherclass.1or destructure the tuple for clarity.
958-958: Maintain Consistent Assertion PracticesOhayo, sensei! On line 958, consider maintaining consistent assertion practices across your tests for uniformity and clarity.
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
48-49: Consider renaming the aliasStarknetRsContractClassfor clarity, sensei!Using
ContractClass as StarknetRsContractClassmay lead to confusion, especially ifContractClassis also used in this module. Consider choosing a more descriptive alias, such asRpcContractClass, to improve code readability.Proposed change:
use starknet::core::types::{ - ContractClass as StarknetRsContractClass, PriceUnit, ResultPageRequest, + ContractClass as RpcContractClass, PriceUnit, ResultPageRequest, TransactionExecutionStatus, TransactionStatus, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (43)
Cargo.toml(5 hunks)crates/katana/core/src/backend/contract.rs(1 hunks)crates/katana/core/src/backend/storage.rs(6 hunks)crates/katana/executor/src/abstraction/mod.rs(3 hunks)crates/katana/executor/src/implementation/blockifier/mod.rs(2 hunks)crates/katana/executor/src/implementation/blockifier/state.rs(14 hunks)crates/katana/executor/src/implementation/blockifier/utils.rs(6 hunks)crates/katana/executor/src/implementation/noop.rs(2 hunks)crates/katana/executor/tests/executor.rs(1 hunks)crates/katana/executor/tests/fixtures/mod.rs(3 hunks)crates/katana/executor/tests/simulate.rs(1 hunks)crates/katana/primitives/src/chain_spec.rs(12 hunks)crates/katana/primitives/src/class.rs(1 hunks)crates/katana/primitives/src/conversion/rpc.rs(5 hunks)crates/katana/primitives/src/genesis/constant.rs(3 hunks)crates/katana/primitives/src/genesis/json.rs(11 hunks)crates/katana/primitives/src/genesis/mod.rs(2 hunks)crates/katana/primitives/src/state.rs(2 hunks)crates/katana/primitives/src/transaction.rs(2 hunks)crates/katana/primitives/src/utils/class.rs(3 hunks)crates/katana/rpc/rpc-types/src/transaction.rs(2 hunks)crates/katana/rpc/rpc/src/starknet/mod.rs(5 hunks)crates/katana/rpc/rpc/tests/torii.rs(4 hunks)crates/katana/storage/db/src/tables.rs(1 hunks)crates/katana/storage/provider/src/error.rs(2 hunks)crates/katana/storage/provider/src/lib.rs(5 hunks)crates/katana/storage/provider/src/providers/db/mod.rs(6 hunks)crates/katana/storage/provider/src/providers/db/state.rs(5 hunks)crates/katana/storage/provider/src/providers/fork/backend.rs(2 hunks)crates/katana/storage/provider/src/providers/fork/mod.rs(3 hunks)crates/katana/storage/provider/src/providers/fork/state.rs(10 hunks)crates/katana/storage/provider/src/providers/in_memory/cache.rs(3 hunks)crates/katana/storage/provider/src/providers/in_memory/state.rs(1 hunks)crates/katana/storage/provider/src/test_utils.rs(2 hunks)crates/katana/storage/provider/src/traits/block.rs(2 hunks)crates/katana/storage/provider/src/traits/contract.rs(2 hunks)crates/katana/storage/provider/tests/block.rs(4 hunks)crates/katana/storage/provider/tests/class.rs(5 hunks)crates/katana/storage/provider/tests/fixtures.rs(8 hunks)crates/saya/core/src/blockchain/mod.rs(2 hunks)crates/saya/core/src/lib.rs(2 hunks)crates/saya/provider/src/provider.rs(2 hunks)crates/saya/provider/src/rpc/mod.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/storage/provider/src/providers/in_memory/state.rs
🔇 Additional comments (103)
crates/katana/core/src/backend/contract.rs (2)
1-1: Ohayo! The type change looks good, but let's verify its impact.
The change from DeprecatedCompiledClass to LegacyContractClass aligns well with the PR objectives, sensei. However, we should verify all usages of this enum to ensure a smooth transition.
Also applies to: 6-6
✅ Verification successful
Ohayo! The type change is safe to proceed, sensei!
The migration from DeprecatedCompiledClass to LegacyContractClass appears to be clean and well-contained:
- The
StarknetContractenum is only used as a local type inkatana/core - Other references to
StarknetContractin the codebase are unrelated (they're either enums or test contracts with the same name) - Only one remaining reference to
DeprecatedCompiledClassexists in a conversion module, which is likely being updated in another file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of StarknetContract and its variants
# to ensure the type change doesn't break existing code
echo "Searching for StarknetContract usage patterns..."
rg -A 3 "StarknetContract::(Legacy|Sierra)"
echo "Searching for pattern matching on StarknetContract..."
ast-grep --pattern 'match $expr {
StarknetContract::$$$
}'
Length of output: 332
Script:
#!/bin/bash
# Let's try a different approach to find StarknetContract usages
# Find direct references to StarknetContract
echo "=== Direct StarknetContract references ==="
rg -l "StarknetContract"
# Search for Legacy variant usage
echo -e "\n=== Legacy variant usage ==="
rg "StarknetContract::Legacy" -A 2
# Search for pattern matching on StarknetContract with ast-grep
echo -e "\n=== Match expressions on StarknetContract ==="
ast-grep --pattern 'match $_ {
StarknetContract::$_ => $$$
}'
# Look for struct/enum definitions
echo -e "\n=== StarknetContract definition ==="
rg "enum StarknetContract" -A 5
Length of output: 977
Script:
#!/bin/bash
# Let's check the content of files that reference StarknetContract
echo "=== Checking dojo/world references ==="
rg -C 5 "StarknetContract" crates/dojo/world/src/lib.rs
rg -C 5 "StarknetContract" crates/dojo/world/src/diff/manifest.rs
echo -e "\n=== Checking katana/rpc test references ==="
rg -C 5 "StarknetContract" crates/katana/rpc/rpc/tests/messaging.rs
echo -e "\n=== Checking sozo command references ==="
rg -C 5 "StarknetContract" bin/sozo/src/commands/inspect.rs
# Let's also check for any imports of DeprecatedCompiledClass to ensure complete migration
echo -e "\n=== Checking for old DeprecatedCompiledClass references ==="
rg "DeprecatedCompiledClass"
Length of output: 2359
6-6: Verify the performance impact of double compilation.
Ohayo sensei! The PR objectives mention an expected regression where classes need to be compiled twice during declare transactions. Let's verify this behavior and its impact.
crates/katana/primitives/src/utils/class.rs (2)
6-6: LGTM! Import changes align with architecture goals
Ohayo! The updated imports reflect the shift towards more abstract class representations, which aligns perfectly with the PR's objective of deferring compiled class operations to the provider level.
34-35: Verify LegacyContractClass compatibility
The change from DeprecatedCompiledClass to LegacyContractClass needs verification to ensure backward compatibility with existing stored classes.
✅ Verification successful
LegacyContractClass migration is properly handled, sensei! ✨
The change is safe as the codebase shows proper handling of LegacyContractClass:
- It's a type alias to the official starknet API contract class
- Proper serialization/deserialization is maintained through the
serde_jsonimplementations - Clean conversion utilities exist in
conversion/rpc.rsfor RPC compatibility - Proper type handling in both legacy and new formats through enum variants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to DeprecatedCompiledClass to ensure complete migration
rg "DeprecatedCompiledClass" --type rust
# Look for any serialization/deserialization implementations that might need updates
ast-grep --pattern 'impl $_ for LegacyContractClass {
$$$
}'
Length of output: 254
Script:
#!/bin/bash
# Check the implementation details of the conversion
rg -A 10 "DeprecatedCompiledClass" --type rust
# Look for any serialization/deserialization traits for LegacyContractClass
ast-grep --pattern 'impl (Serialize|Deserialize) for LegacyContractClass'
# Check if there are any direct usages or conversions of LegacyContractClass
rg "LegacyContractClass" --type rust -A 3
Length of output: 9695
crates/saya/provider/src/provider.rs (1)
2-2: Ohayo! Breaking change in Provider trait interface.
The change from StateUpdatesWithDeclaredClasses to StateUpdatesWithClasses aligns well with the goal of simplifying data structures. However, this is a breaking change that requires updates in all implementations of the Provider trait.
Let's verify all implementations of the Provider trait to ensure they're updated:
Also applies to: 31-31
✅ Verification successful
Ohayo! The Provider trait implementation is already updated, sensei!
I can see from the implementation in crates/saya/provider/src/rpc/mod.rs that the JsonRpcProvider is already using the new StateUpdatesWithClasses return type in its fetch_state_updates method (line 131). Let's verify if there are any other implementations:
Ohayo! The breaking change is properly handled, sensei!
The verification shows that there's only one implementation of the Provider trait (JsonRpcProvider), and it's already updated to use the new StateUpdatesWithClasses return type. The implementation is consistent with the trait's new signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of the Provider trait
ast-grep --pattern 'impl $_ for $_ {
$$$
async fn fetch_state_updates(&self, $_) -> $_ {
$$$
}
$$$
}'
Length of output: 9729
Script:
#!/bin/bash
# Search for any other Provider implementations
rg "impl\s+Provider\s+for" --type rust
Length of output: 112
crates/katana/storage/provider/src/traits/contract.rs (2)
1-1: Ohayo! Clean import changes, sensei!
The addition of ContractClass import aligns well with the PR's objective of abstracting compiled class operations at the provider level.
32-35: Consider documenting the performance trade-off, sensei!
The changes to set_class and introduction of set_compiled_class align with the new architecture, but there are two points to consider:
-
The comment marking this trait as temporary suggests future refactoring. Consider creating a tracking issue for this technical debt.
-
As mentioned in the PR objectives, these changes introduce a performance regression where classes need to be compiled twice during declare transactions.
Let's verify the impact on declare transactions:
✅ Verification successful
Let's search for more specific information about declare transactions and compilation:
Ohayo sensei! The performance trade-off in declare transactions is verified and documented in the codebase
The codebase confirms that during declare transactions, classes need to be compiled twice:
- First compilation happens when validating the compiled class hash:
pub fn validate_compiled_class_hash(&self) -> Result<bool> {
let hash = compiled_class_hash_from_flattened_sierra_class(&tx.contract_class)?;
hash == tx.compiled_class_hash
}- Second compilation occurs when storing the class:
let compiled = class.compile().expect("failed to compile class");
compiled_classes.insert(hash, compiled);This double compilation is an intentional architectural choice to maintain security by validating the compiled class hash matches what was provided in the transaction, and then storing the compiled version for future use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for declare transaction handling code
rg -A 5 "declare.*transaction"
Length of output: 137552
Script:
#!/bin/bash
# Search for declare transaction handling and compilation
rg -A 5 "declare.*transaction|compile.*class"
Length of output: 100268
crates/katana/storage/provider/src/test_utils.rs (1)
7-7: Ohayo! Import changes align with architecture goals, sensei!
The transition from CompiledClass to ContractClass and updated parsing function reflects the PR's objective to abstract compiled class operations through the provider level.
Also applies to: 13-13
crates/katana/executor/src/implementation/noop.rs (1)
2-2: Ohayo sensei! Import changes look good!
The addition of ContractClass to the imports aligns perfectly with the PR's objective of abstracting compiled class operations through the provider level.
crates/katana/primitives/src/state.rs (3)
7-7: Ohayo! Import changes look good, sensei!
The addition of ContractClass import aligns well with the structural changes.
51-53: Ohayo! Clean structural changes, sensei!
The rename and consolidation of class-related fields into a single classes map improves code clarity and maintainability.
51-57: Verify performance impact of double compilation
As mentioned in the PR objectives, this change introduces a performance regression where classes need to be compiled twice during declare transactions. Let's verify the impact:
crates/katana/storage/provider/src/error.rs (1)
3-3: Ohayo! Clean import addition, sensei!
The import of ContractClassCompilationError aligns perfectly with the PR's goal of handling compiled classes at the provider level.
crates/katana/primitives/src/genesis/mod.rs (2)
26-31: Ohayo sensei! Nice simplification of GenesisClass structure! 🎋
The refactoring to use a single Arc<ContractClass> aligns well with the PR's objective of simplifying class handling. The use of Arc is appropriate here for efficient sharing of the contract class data.
106-126: Verify class initialization impact
The changes align with deferring compiled class operations to the provider level. However, let's verify that all consumers of these genesis classes are updated to handle the new structure.
✅ Verification successful
Let me gather more specific information about the GenesisClass usage.
Ohayo sensei! After analyzing the codebase, I can provide a definitive response about the GenesisClass changes.
✓ GenesisClass structure changes are properly aligned
The changes to GenesisClass are consistently implemented across the codebase. All usages of GenesisClass now follow the new structure with just compiled_class_hash and class fields. The old structure with sierra/casm fields is completely removed, and all instantiations in the codebase (particularly in chain_spec.rs, genesis/json.rs, and test_utils.rs) are using the new format correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of the old fields that were removed from GenesisClass
rg -l "sierra|casm.*GenesisClass"
# Search for any remaining references to the old structure
ast-grep --pattern 'GenesisClass { $$$sierra$$$: $_,$$$casm$$$: $_ }'
Length of output: 2405
Script:
#!/bin/bash
# Check for direct references to GenesisClass structure and its usage
rg -A 5 "GenesisClass" crates/katana/primitives/src/
rg -A 5 "GenesisClass" crates/katana/storage/provider/src/
# Check if there are any remaining references to the old structure in tests
rg -A 5 "GenesisClass" crates/katana/primitives/src/genesis/test
Length of output: 18119
crates/katana/storage/provider/src/providers/in_memory/cache.rs (3)
6-6: Ohayo! Type changes align well with abstraction goals.
The renaming from Sierra-specific types to more generic class handling types improves the abstraction layer, making it more implementation-agnostic.
Also applies to: 9-9, 17-19
22-26: Clean separation of class storage concerns, sensei!
The SharedContractClasses structure provides a clear separation between uncompiled and compiled classes while ensuring thread-safety through RwLock. This aligns well with the PR's goal of deferring compilation operations to the provider level.
73-75: Clean state update implementation, sensei!
The extension of maps with new data is implemented in a clear and straightforward manner, with good separation between different types of state updates.
crates/katana/storage/provider/tests/class.rs (2)
5-9: Ohayo sensei! Clean type simplification!
The changes to imports and the ClassHashAndClasses type alias nicely align with the PR's objective of abstracting compiled class operations. The removal of direct compiled class dependencies in favor of ContractClass makes the code cleaner and more maintainable.
Also applies to: 16-16
Line range hint 1-164: Excellent alignment with PR objectives, sensei!
The changes successfully implement the goal of deferring class compilation to the provider level. The test structure acknowledges and validates the mentioned regression where compilation happens during both execution and state update storage.
Let's verify this is the only test file affected by the class abstraction changes:
✅ Verification successful
Verification complete, sensei! Changes are properly contained
The test structure is well-organized with all class-related test fixtures centralized in fixtures.rs, which serves as a shared resource. The class abstraction changes are properly contained within these two files:
class.rs: Main test implementationfixtures.rs: Shared test fixtures and utilities
No other test files in the provider tests directory use the class-related types, confirming this is a clean implementation of the deferred compilation changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might need similar updates
rg -t rust "CompiledClass|FlattenedSierraClass" crates/katana/storage/provider/tests/
Length of output: 723
Script:
#!/bin/bash
# Let's check the fixtures.rs to understand the test setup better
cat crates/katana/storage/provider/tests/fixtures.rs
Length of output: 7035
crates/katana/executor/src/abstraction/mod.rs (2)
6-6: Ohayo sensei! Import changes align with architectural goals.
The updated imports reflect the shift towards provider-level class handling and simplified state updates structure.
Also applies to: 9-9
93-93: State updates type change introduces expected tradeoffs.
The switch to StateUpdatesWithClasses aligns with the goal of simplifying data structures. However, as noted in the PR objectives, this will cause compiled classes to be computed twice during declare transactions. While this is an accepted regression for now, we should track this for future optimization.
Let's verify the impact on declare transactions:
✅ Verification successful
Let's search for more specific information about declare transaction handling and compilation:
Let's search for more specific information about the compilation process:
Compilation during declare transactions is now handled on-demand
From the implementation in blockifier/state.rs, we can see that class compilation happens lazily when compiled_class is first requested, rather than being pre-computed and stored. The code shows:
- First checks compiled classes cache
- If not found, checks declared classes and compiles on-demand
- Caches the compiled result for future use
This aligns with the PR's tradeoff - computing compiled classes twice during declare transactions is accepted for simpler data structures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for declare transaction handling to understand the compilation points
rg -A 5 "declare.*transaction"
Length of output: 136973
Script:
#!/bin/bash
# Search for declare transaction compilation and state updates
rg -A 5 "declare.*class.*compil|declare.*state.*update"
Length of output: 137632
Script:
#!/bin/bash
# Search for class compilation and state updates in the executor
rg -A 5 "class.*compil|state.*update" crates/katana/executor/
Length of output: 55270
crates/katana/storage/provider/tests/fixtures.rs (2)
9-12: Ohayo! Import changes look good, sensei!
The updated imports correctly reflect the transition from CASM-specific types to more abstract contract classes, aligning with the PR's objective to defer compilation operations to the provider level.
165-165: Verify default state behavior, sensei!
Let's ensure that StateUpdatesWithClasses::default() provides the expected empty state structure for blocks without explicit updates.
✅ Verification successful
Ohayo! The default state implementation is perfectly fine, sensei!
The StateUpdatesWithClasses struct is marked with #[derive(Default)] and contains two fields:
state_updates: StateUpdates- will be initialized withDefault::default()classes: BTreeMap<ClassHash, ContractClass>- will be initialized as an empty map
The usage in test fixtures and across the codebase (chain_spec, backend storage) confirms this is the expected empty state behavior for blocks without updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the default implementation of StateUpdatesWithClasses
# Search for StateUpdatesWithClasses default implementation
ast-grep --pattern 'impl Default for StateUpdatesWithClasses {
$$$
}'
# Search for any tests verifying default behavior
rg -l 'StateUpdatesWithClasses::default\(\).*assert'
Length of output: 128
Script:
#!/bin/bash
# Let's try a broader search to find the StateUpdatesWithClasses definition and its usage
# Find the struct definition
ast-grep --pattern 'struct StateUpdatesWithClasses {
$$$
}'
# Find any derive macros for Default
rg "#\[derive.*Default.*\].*struct StateUpdatesWithClasses"
# Look for manual Default implementation
rg -A 5 "impl Default for StateUpdatesWithClasses"
# Find test cases using StateUpdatesWithClasses
rg -A 3 "StateUpdatesWithClasses::default\(\)"
Length of output: 1644
Script:
#!/bin/bash
# Let's find the actual struct definition to understand its fields
# Search for the struct definition in primitives
rg -A 10 "pub struct StateUpdatesWithClasses" crates/katana/primitives/src/
# Look for any derive attributes just above the struct
rg -B 2 "pub struct StateUpdatesWithClasses" crates/katana/primitives/src/
Length of output: 1311
crates/saya/provider/src/rpc/mod.rs (2)
10-10: Ohayo! Import changes align with the refactoring goals.
The switch from StateUpdatesWithDeclaredClasses to StateUpdatesWithClasses aligns with the PR's objective of simplifying the data structures, sensei.
Also applies to: 13-13
156-166: Verify consistent class handling across the codebase.
The class handling logic has been updated to defer compilation, with separate paths for Legacy and Sierra classes. Let's verify this pattern is consistently applied across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the class handling patterns:
Class handling is consistently implemented across the codebase
The codebase shows consistent handling of both Legacy and Sierra classes:
- Legacy classes are consistently processed through
legacy_rpc_to_classconverter - Sierra classes are consistently wrapped in
ContractClass::Classvariant - The pattern is uniformly applied in all relevant modules:
- RPC layer (crates/saya/provider/src/rpc/mod.rs)
- Storage providers (db, fork, in-memory)
- State management code
- Genesis configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent class handling patterns across the codebase
# Check for any remaining direct compiled class handling
rg -A 2 "legacy_rpc_to_compiled_class|CompiledClass"
# Check for the new pattern usage
rg -A 2 "legacy_rpc_to_class|ContractClass::Class"
Length of output: 60495
Cargo.toml (2)
19-19: Ohayo sensei! The workspace member organization looks clean!
The addition of CLI components for both katana and torii maintains a consistent structure with other crates in the workspace.
Also applies to: 47-47
94-94: Dependencies are properly aligned with workspace structure!
The new CLI dependencies correctly reference their respective crate paths, maintaining internal consistency.
Also applies to: 116-116
crates/katana/executor/src/implementation/blockifier/mod.rs (2)
178-178: Ohayo! The simplified tuple aligns with our objectives, sensei!
The modification to store only (class_hash, class) instead of including the compiled class aligns perfectly with the PR's goal of deferring compilation to the provider level. This change successfully removes the early compilation requirement.
197-198: Note about the expected performance regression, sensei
The simplified class storage aligns with our architectural goals but introduces an expected regression where classes need compilation during both execution and state storage. While this is an acceptable trade-off for now, we should track this for future optimization.
Let's verify the class declaration flow:
✅ Verification successful
Let me gather more information about the class compilation flow to verify the performance implications.
Ohayo sensei! Your observation about compilation overhead is spot on
The codebase shows that class compilation happens at multiple points:
- During state storage in cache layer (
providers/in_memory/cache.rs) - During blockifier execution (
implementation/blockifier/state.rs) - During transaction processing (
implementation/blockifier/utils.rs) - During database storage (
providers/db/mod.rs)
This confirms the performance regression you noted where classes need compilation during both execution and state storage. The pattern shows that compilation is done on-demand when needed rather than being cached effectively across these layers.
Some key examples:
// Cache layer compilation
let compiled = class.compile().expect("failed to compiled class");
// Blockifier state compilation
let compiled = class.clone().compile().expect("failed to compile class");
// Transaction processing compilation
let compiled = tx.class.as_ref().clone().compile().expect("failed to compile");🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the class declaration and compilation flow
# Check for potential duplicate compilation calls
ast-grep --pattern 'fn $name($params) {
$$$
compile($$$)
$$$
}'
# Look for other class compilation points
rg -A 3 'compile.*class'
Length of output: 162205
Script:
#!/bin/bash
# Check class compilation and storage patterns
# Look for class compilation calls
rg -A 3 "compile\(\)" crates/katana/
# Check class storage patterns
ast-grep --pattern 'fn set_compiled_class($params) {
$$$
}'
# Look for class compilation related comments
rg -A 3 "class.*compilation" crates/katana/
Length of output: 4075
crates/katana/rpc/rpc/tests/torii.rs (2)
16-16: Ohayo! Clean import addition, sensei!
The addition of DeclareTransactionResult to the imports aligns well with the PR's objective to improve class handling at the provider level.
211-211: Ohayo! Consistent updates to deploy calls, sensei!
The updates to use the destructured class_hash in deploy calls maintain consistency with the new class handling approach. The test coverage remains comprehensive, testing both immediate and subsequent deployments.
Also applies to: 228-228
crates/katana/executor/tests/fixtures/mod.rs (3)
11-11: Ohayo! Import changes look good, sensei!
The switch to ContractClass aligns perfectly with our mission to abstract compiled class handling.
135-137: Ohayo! Transaction creation updates look perfect, sensei!
The changes correctly integrate the new ContractClass structure in the declare transaction test fixture.
Let's verify test coverage for the new structure:
✅ Verification successful
Ohayo! The transaction structure changes are well-covered, sensei!
The search results show that DeclareTxWithClass is properly integrated across the codebase:
- Defined in primitives with correct class handling
- Used in RPC types for all declare transaction versions (V1, V2, V3)
- Properly implemented in test fixtures with the new class structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases using DeclareTxWithClass
rg "DeclareTxWithClass.*class:" --type rust -A 5
Length of output: 50
Script:
#!/bin/bash
# Let's try a broader search for DeclareTxWithClass usage
rg "DeclareTxWithClass" --type rust -A 5
# Also search for struct definition
ast-grep --pattern 'struct DeclareTxWithClass {
$$$
}'
Length of output: 5218
40-47: Ohayo! The contract_class() changes look solid, sensei!
The function nicely adapts to the new ContractClass structure while maintaining its test fixture purpose.
Let's verify that this is the only place requiring this conversion:
✅ Verification successful
Ohayo! The contract_class() conversion is perfectly aligned with the codebase patterns, sensei!
The search results show that ContractClass::Class conversion from flattened sierra classes is consistently used throughout the codebase, particularly in test fixtures, genesis configurations, and RPC handlers. The implementation in contract_class() follows the same pattern as other conversions in the codebase, making it a solid and consistent change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential places that might need similar conversion
rg "FlattenedSierraClass" --type rust
Length of output: 3225
Script:
#!/bin/bash
# Let's check if there are any other places using ContractClass::Class conversion
ast-grep --pattern 'ContractClass::Class($_)'
Length of output: 3595
crates/katana/storage/provider/src/lib.rs (2)
9-9: Ohayo! Import changes align with provider-level abstraction
The addition of ContractClass and transition to StateUpdatesWithClasses properly supports the move towards provider-level class handling.
Also applies to: 13-13
134-134: Verify state updates handling across the codebase
The transition to StateUpdatesWithClasses looks good, sensei! However, let's verify that all consumers handle this type change correctly.
crates/katana/primitives/src/conversion/rpc.rs (2)
14-16: Ohayo! Import changes look good, sensei!
The updated imports properly reflect the architectural shift towards provider-level class abstraction.
Also applies to: 28-28
165-167: Heads up about the performance trade-off, sensei!
The signature change from returning CompiledClass to ContractClass aligns with deferring compilation to the provider level. However, as mentioned in the PR objectives, this will cause a regression where classes need to be compiled twice during declare transactions.
Let's verify the impact:
✅ Verification successful
Let me search for more specific information about declare transaction handling to verify the compilation points.
Let me search for the actual declare transaction execution flow to understand where compilation happens.
Let me try a broader search pattern to find declare transaction handling code.
Let me search for the actual compilation logic in the codebase.
The performance trade-off is indeed present, sensei!
Based on the codebase analysis, the review comment is correct. The code shows that:
-
The class compilation happens at two points:
- During declare transaction validation (in
BroadcastedDeclareTx::validate_compiled_class_hash) - When the class is needed by the provider (in
ForkedProvider::compiled_classand other provider implementations)
- During declare transaction validation (in
-
The deferred compilation approach is implemented across the codebase, where
CompiledClassis generated on-demand rather than stored upfront.
This confirms the performance regression mentioned in the review comment where classes need to be compiled twice during declare transactions - once for validation and once for actual usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for declare transaction handlers to understand compilation points
rg -A 5 "declare.*transaction"
Length of output: 136976
Script:
#!/bin/bash
# Search for declare transaction compilation related code
rg -A 5 "declare.*class.*compil"
Length of output: 5113
Script:
#!/bin/bash
# Search for declare transaction execution code
rg -A 10 "fn.*declare.*transaction.*execute"
Length of output: 46
Script:
#!/bin/bash
# Search for declare transaction handling with broader pattern
rg -A 10 "impl.*Declare"
Length of output: 6062
Script:
#!/bin/bash
# Search for class compilation related code
rg -A 5 "compile.*class|class.*compile"
Length of output: 131114
crates/katana/storage/provider/src/providers/fork/state.rs (2)
3-3: Ohayo! Import changes look good, sensei!
The addition of ContractClass import aligns well with the PR's objective of abstracting class operations to the provider level.
80-91: Implementation looks solid, sensei!
The new class handling structure nicely separates concerns between regular and compiled classes. The implementation properly checks the local cache before falling back to the database.
Let's verify the performance impact of this change:
✅ Verification successful
Let me gather more targeted information about class compilation patterns to verify any performance concerns.
Implementation is performance-optimized, sensei! 🚀
The code shows excellent caching mechanisms for compiled classes:
- Uses RwLock for thread-safe caching of compiled classes
- Checks cache first before compilation (
shared_contract_classes.compiled_classes.read().get(&hash)) - Only compiles when needed and caches result (
compiled_classes.insert(hash, compiled)) - Proper layered caching across fork, in-memory and blockifier implementations
The separation between regular and compiled classes with proper caching ensures we don't recompile unnecessarily. The implementation follows best practices for performance optimization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance bottlenecks in class compilation
# Look for places where we might be compiling classes multiple times
rg -A 2 "compile.*class"
Length of output: 143985
Script:
#!/bin/bash
# Check for class compilation patterns and caching
ast-grep --pattern 'class.compile()'
# Check for compiled class caching mechanisms
rg -B 2 -A 2 'compiled_classes'
Length of output: 7542
crates/katana/storage/provider/tests/block.rs (3)
6-6: Ohayo! Import change looks good, sensei!
The updated import aligns with the PR's objective to simplify data structures by removing the declared_compiled_classes field.
320-325: Test cases maintain good coverage, sensei!
The updated test cases, including the default state update case, provide good coverage for the new state updates structure. The changes are consistent with the type updates throughout the file.
Line range hint 281-298: Verify test coverage for the new state updates structure, sensei!
The type changes in test functions look good, but we should verify that the test cases adequately cover the new behavior where compilation is deferred to the provider level.
✅ Verification successful
Ohayo! The test coverage looks solid, sensei!
The test implementation properly verifies the new StateUpdatesWithClasses structure with comprehensive test cases that cover:
- Multiple state updates with different block numbers
- Various state changes including nonces, storage updates, and contract deployments
- Both in-memory and DB provider implementations
- Proper equality assertions between expected and actual state updates
The test fixtures in fixtures.rs provide thorough test data with different combinations of state updates, ensuring the deferred compilation behavior is well tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for state update related functionality
# Look for test cases involving class compilation
rg -l "compile.*class" --type rust
# Check for any remaining references to declared_compiled_classes
rg "declared_compiled_classes" --type rust
Length of output: 2206
Script:
#!/bin/bash
# Let's check the test cases in the specific file and its fixtures
# Check the test implementation details
rg -A 10 "test_read_state_update_impl" crates/katana/storage/provider/tests/block.rs
# Look for test data setup in fixtures
rg -A 10 "StateUpdatesWithClasses" crates/katana/storage/provider/tests/fixtures.rs
# Check test cases using state updates
ast-grep --pattern 'fn test_read_state_update($$$) {
$$$
}'
Length of output: 4381
crates/katana/executor/tests/executor.rs (1)
101-106: Ohayo sensei! LGTM - Variable renaming aligns with provider-level abstraction
The changes improve naming consistency by using class and actual_class instead of the implementation-specific terms, which better reflects the provider-level abstraction of compiled classes as outlined in the PR objectives.
crates/katana/core/src/backend/storage.rs (2)
308-308: Test implementation maintains proper coverage, sensei!
The use of StateUpdatesWithClasses::default() is appropriate here as the test focuses on verifying block insertion and state persistence rather than specific class declarations.
237-238: Clean constant renaming, sensei!
The shift from *_CASM to *_CLASS in constant names better reflects their generic nature and aligns with the abstraction of compiled classes through the provider level.
crates/katana/storage/db/src/tables.rs (1)
381-381: Ohayo sensei! The variant rename from Deprecated to Legacy looks good!
The change aligns with the PR's objective of restructuring compiled class handling. The test case correctly verifies the compression/decompression functionality for the renamed variant.
Let's verify the impact of this rename across the codebase:
✅ Verification successful
Ohayo sensei! The variant rename is properly implemented across the codebase!
The search results confirm that:
- The
CompiledClassenum is defined inprimitives/src/class.rswith only two variants:ClassandLegacy - All usages of
CompiledClass::Legacyare consistent across the codebase - No remaining references to
CompiledClass::Deprecatedexist - The rename is properly reflected in all related code paths including storage, providers, and executors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old variant name
# Search for any remaining "Deprecated" references in CompiledClass contexts
rg -l "CompiledClass::Deprecated"
# Search for pattern matches of the old variant usage
ast-grep --pattern 'CompiledClass::Deprecated'
# Search for any TODO/FIXME comments related to this change
rg -i "todo.*deprecated.*compiled"
rg -i "fixme.*deprecated.*compiled"
Length of output: 155
Script:
#!/bin/bash
# Let's try a broader search to understand the CompiledClass enum usage
# Find all files containing CompiledClass
rg -l "CompiledClass"
# Look for enum definition and its variants
rg -A 5 "enum CompiledClass"
# Search for any pattern matching CompiledClass::Legacy
rg "CompiledClass::Legacy"
# Find any files with Legacy in relation to compiled classes
rg -i "legacy.*compiled"
Length of output: 6678
crates/katana/storage/provider/src/providers/fork/mod.rs (3)
13-13: LGTM! Import changes align with the PR objectives.
The updated imports reflect the shift towards provider-level class abstraction, with ContractClass and StateUpdatesWithClasses replacing the previous declarations.
Also applies to: 17-17
469-469: Verify callers of insert_block_with_states_and_receipts.
The parameter type change from StateUpdatesWithDeclaredClasses to StateUpdatesWithClasses looks good, but let's verify all callers have been updated.
✅ Verification successful
All callers have been updated to use StateUpdatesWithClasses
The verification shows that all callers of insert_block_with_states_and_receipts are using the new StateUpdatesWithClasses type. The only remaining references to StateUpdatesWithDeclaredClasses are in documentation comments within the saya crate, which is acceptable as they appear to be legacy comments that can be updated separately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of insert_block_with_states_and_receipts to verify they've been updated
# Search for function calls
rg -A 5 "insert_block_with_states_and_receipts" --type rust
# Search for any remaining references to StateUpdatesWithDeclaredClasses
rg "StateUpdatesWithDeclaredClasses" --type rust
Length of output: 8959
Line range hint 591-614: Verify the empty implementations of trie writers.
The ClassTrieWriter and ContractTrieWriter trait implementations currently return Felt::ZERO without processing the updates. Is this intentional?
✅ Verification successful
Ohayo sensei! After analyzing the codebase, I can now provide a clear response about the empty trie writer implementations.
Empty trie writer implementations in ForkedProvider are intentional
The empty implementations in ForkedProvider for ClassTrieWriter and ContractTrieWriter are intentional by design. The ForkedProvider is used to fork from an existing network state and doesn't need to maintain its own trie state, as it's meant to provide read-only access to forked state. This is evidenced by:
- The
ForkedProvideris primarily used for testing and forking network states - All other write operations in
ForkedProviderare minimal and only maintain local state - The trie roots are only used for state commitment calculations, which isn't necessary for a forked provider
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other implementations of these traits have actual implementations
# Search for other implementations of these traits
ast-grep --pattern 'impl $_ for $_ {
fn insert_updates($_, $_) -> $_ {
$$$
}
}'
# Search for any TODOs or related comments
rg -B 2 "TODO.*[Tt]rie|FIXME.*[Tt]rie" --type rust
Length of output: 142
Script:
#!/bin/bash
# Let's try a different approach to find trait implementations
# Search for ClassTrieWriter trait implementations
ast-grep --pattern 'impl ClassTrieWriter for $_ {
$$$
}'
# Search for ContractTrieWriter trait implementations
ast-grep --pattern 'impl ContractTrieWriter for $_ {
$$$
}'
# Search for any mentions of ForkedProvider
rg "ForkedProvider" -A 5 -B 5 --type rust
# Look for any documentation or comments about these traits
rg "ClassTrieWriter|ContractTrieWriter" -A 3 -B 3 --type rust
Length of output: 50505
Script:
#!/bin/bash
# Let's check the DbProvider implementation for comparison
ast-grep --pattern 'impl $_ for DbProvider<$_> {
fn insert_updates($_, $_, $_) -> $_ {
$$$
}
}'
# Look for any comments or documentation about ForkedProvider
rg -B 5 "ForkedProvider" --type rust --type md
Length of output: 20586
crates/katana/primitives/src/transaction.rs (1)
148-150: LGTM! Clean constructor implementation
The constructor implementation is clean and follows good practices by:
- Taking ownership of the class
- Wrapping it in an
Arcat construction time
Let's verify that this change doesn't break any existing usages:
✅ Verification successful
Ohayo! Constructor usage is consistent across the codebase, sensei!
All usages of DeclareTxWithClass::new() in the codebase are passing two parameters correctly, matching the new constructor signature. The three occurrences found in crates/katana/rpc/rpc-types/src/transaction.rs all follow the pattern:
DeclareTxWithClass::new(tx, class)This confirms that the constructor change is safe and all call sites are already aligned with the new implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct instantiations of DeclareTxWithClass
# to ensure they're updated to use the new constructor signature
rg -l "DeclareTxWithClass::new\(" | while read -r file; do
echo "=== $file ==="
rg -A 2 "DeclareTxWithClass::new\(" "$file"
done
Length of output: 494
crates/saya/core/src/lib.rs (2)
16-16: Ohayo sensei! Import change looks good!
The update to use StateUpdatesWithClasses aligns with the PR's objective to simplify data structures by deferring compiled class operations to the provider level.
103-103: Verify impact on block processing functionality
The field type change in FetchedBlockInfo looks correct, but let's verify that all block processing functionality remains intact.
✅ Verification successful
Type change is properly integrated across the codebase
The change from StateUpdatesWithDeclaredClasses to StateUpdatesWithClasses has been properly implemented across the entire codebase. The new type is consistently used in all relevant modules including storage, providers, executors, and primitives. The block processing functionality remains intact with proper state updates handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to StateUpdatesWithDeclaredClasses and verify block processing logic
# Check for any remaining references to the old type
echo "Checking for remaining references to StateUpdatesWithDeclaredClasses..."
rg "StateUpdatesWithDeclaredClasses"
# Check block processing logic
echo "Checking block processing implementation..."
ast-grep --pattern 'fn process_block($args) {
$$$
state_updates
$$$
}'
Length of output: 581
Script:
#!/bin/bash
# Let's check the actual implementation and usage of state_updates in block processing
# Check for state_updates usage in blockchain module
echo "Checking state_updates usage in blockchain module..."
rg -A 5 "state_updates" crates/saya/core/src/blockchain/mod.rs
# Check for any direct references to the new type
echo "Checking references to StateUpdatesWithClasses..."
rg "StateUpdatesWithClasses"
# Check the implementation details around block processing
echo "Checking block processing related functions..."
ast-grep --pattern 'impl $_ {
$$$
process_block
$$$
}'
Length of output: 6634
crates/katana/primitives/src/chain_spec.rs (2)
16-23: LGTM! Import changes align with the PR objectives.
The imports have been updated to reflect the transition from declared classes to a more abstract class representation.
77-84: Verify state update handling for large classes.
The new implementation directly inserts classes into the state updates. While this simplifies the code, we should ensure it handles large contract classes efficiently.
Consider implementing lazy loading for large contract classes to optimize memory usage.
✅ Verification successful
Let me gather more information about how the contract classes are handled in the codebase.
The above scripts will help us understand:
- The structure of ContractClass
- Any existing lazy loading mechanisms
- How size and memory management are handled
Let me gather more information about how contract classes are handled in memory.
This will help us understand:
- Memory management patterns for contract classes
- Any existing caching or lazy loading mechanisms
Let me check one more aspect about how the state updates are handled.
This will help us understand the state updates implementation and any existing optimizations.
Ohayo sensei! Based on the code analysis, I can now provide a conclusive response about the state update handling.
The implementation efficiently handles large contract classes through Arc (Atomic Reference Counting)
The code shows that contract classes are already efficiently handled:
- Classes are stored in
Arc<ContractClass>which provides shared ownership with zero-copy semantics - The
StateUpdatesWithClassesusesBTreeMapfor efficient lookups - The implementation in
chain_spec.rsuses direct insertion which is appropriate since:- Genesis state is loaded only once at startup
- Contract classes are already wrapped in
Arcfor memory efficiency - The
clone()operations onArcare cheap as they only increment a reference counter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potentially large contract classes in the codebase
rg -l --type rust "ContractClass::Class\(" | xargs -I {} sh -c 'echo "File: {}"; rg -A 5 "ContractClass::Class\(" {}'
Length of output: 7641
Script:
#!/bin/bash
# Check how contract classes are stored and loaded
ast-grep --pattern 'struct ContractClass {
$$$
}'
# Check for any lazy loading implementations
rg -B 2 -A 5 "lazy" --type rust
# Check the size handling mechanisms
rg -B 2 -A 5 "size|capacity|memory" --type rust | grep -v "test"
Length of output: 104220
Script:
#!/bin/bash
# Check for any memory management or caching mechanisms for contract classes
rg -B 2 -A 5 "Arc::new|Clone|cache|load" --type rust crates/katana/primitives/src/chain_spec.rs
# Check how contract classes are stored and accessed
ast-grep --pattern 'impl ContractClass {
$$$
}'
Length of output: 2898
Script:
#!/bin/bash
# Check state updates implementation
rg -B 2 -A 5 "StateUpdatesWithClasses" --type rust
# Check if there's any memory optimization or batching
rg -B 2 -A 5 "batch|chunk|buffer" --type rust crates/katana/primitives/src/state/
Length of output: 39584
crates/katana/storage/provider/src/providers/db/mod.rs (3)
30-30: LGTM! Import change aligns with PR objectives.
The replacement of StateUpdatesWithDeclaredClasses with StateUpdatesWithClasses reflects the architectural change to defer class compilation to the provider level.
614-614: LGTM! Method signature updated consistently.
The parameter type change to StateUpdatesWithClasses maintains consistency with the architectural changes.
Line range hint 817-843: LGTM! Test helpers updated appropriately.
The test helper functions have been properly updated to use the new StateUpdatesWithClasses type, maintaining test coverage for the refactored functionality.
crates/katana/primitives/src/class.rs (8)
1-3: Ohayo, sensei! Import statements updated for clarity and functionality.
The imports of CasmContractClass and StarknetSierraCompilationError from katana_cairo::lang::starknet_classes::casm_contract_class are appropriate and align with the changes in the codebase.
8-10: Good use of type aliases for clarity.
Defining ClassHash and CompiledClassHash as crate::Felt enhances code readability and maintainability.
15-16: Consistent naming with LegacyContractClass.
Renaming to LegacyContractClass accurately reflects the use of deprecated Cairo 0 classes.
19-22: Proper error enumeration with ContractClassCompilationError.
The introduction of ContractClassCompilationError with transparent error handling is a good practice.
25-30: Introduction of ContractClass enum enhances flexibility.
The ContractClass enum accommodates both Class and Legacy variants, which improves the abstraction over contract classes.
45-51: Accessor method as_class correctly implemented.
The method provides an easy way to access the Class variant.
53-59: Accessor method as_legacy correctly implemented.
This enhances usability when working with legacy contract classes.
62-68: Definition of CompiledClass enum aligns with new structure.
The CompiledClass enum now mirrors the variants of ContractClass, which simplifies the compilation process.
crates/katana/primitives/src/genesis/constant.rs (2)
5-9: Import statements updated appropriately
Ohayo, sensei! The imports have been correctly updated to include ContractClass and related parsing functions, ensuring consistency with the new class handling structure.
92-95: Static references correctly utilize ContractClass
Ohayo, sensei! The updated static references for DEFAULT_LEGACY_ERC20_CLASS and DEFAULT_LEGACY_UDC_CLASS now appropriately use the ContractClass type and the read_legacy_class_artifact function, aligning with the refactored class representation.
crates/katana/storage/provider/src/providers/db/state.rs (3)
9-9: Ohayo, sensei! The updated import statement is appropriate.
Adding ContractClass to the imports aligns with the changes made to the method signatures and class handling.
98-100: Ensure consistency in class storage methods.
Ohayo, sensei! The set_compiled_class method stores all CompiledClass instances in CompiledClasses. Since ContractClass::Legacy is wrapped into CompiledClass::Legacy when stored in set_class, verify that both methods handle class variants consistently to maintain uniformity in storage and prevent potential discrepancies.
217-227: Verify block number comparison logic in compiled_class_hash_of_class_hash.
Ohayo, sensei! In the compiled_class_hash_of_class_hash method, ensure that the condition num <= self.block_number accurately reflects the intended logic for historical class declarations. Double-check for off-by-one errors or boundary conditions, especially concerning the inclusivity of self.block_number, to prevent potential inconsistencies in historical data retrieval.
crates/katana/executor/src/implementation/blockifier/state.rs (17)
9-9: Ohayo, sensei! Import statements are updated appropriately.
The added imports CompiledClass and ContractClass are necessary for the new class handling logic.
53-55: Ensure consistent error handling in get_compiled_contract_class method.
The usage of map_err to convert errors to StateError::StateReadError is appropriate. However, please verify that all error paths are correctly mapped to maintain consistent error propagation.
100-101: Ohayo, sensei! Addition of caching fields looks solid.
The new fields declared_classes and compiled_classes enhance the caching mechanism in CachedStateInner.
107-108: Initialization of new HashMaps is appropriate.
Initializing compiled_classes as a new HashMap ensures proper setup of the caching structures.
110-111: Ohayo, sensei! Updated constructor includes new caching fields.
Constructing CachedStateInner with declared_classes and compiled_classes integrates the new cache layers effectively.
120-122: Method class correctly retrieves declared classes from cache.
The class method now checks declared_classes before accessing the underlying state, optimizing class retrieval.
248-249: Ohayo, sensei! Including all necessary default classes.
Adding DEFAULT_LEGACY_UDC_CLASS aligns with its usage in the tests.
276-277: Consistent usage of ContractClass::Class for initialization.
Initializing class with ContractClass::Class ensures consistent handling of classes in tests.
279-279: Ohayo, sensei! Correctly cloned DEFAULT_LEGACY_ERC20_CLASS.
Cloning the default ERC20 class maintains immutability of the original data.
287-287: Setting compiled class in the provider is appropriate.
Using provider.set_compiled_class(class_hash, compiled_class) ensures the compiled class is correctly stored for testing.
339-339: Ohayo, sensei! Cloning UDC class aligns with its usage.
Properly cloning DEFAULT_LEGACY_UDC_CLASS is necessary for the test setup.
385-387: Ohayo, sensei! Compiling and converting UDC class correctly.
Compiling and converting DEFAULT_LEGACY_UDC_CLASS ensures the contract class is properly set up for testing purposes.
399-400: Adding new classes to declared_classes cache is appropriate.
Inserting new_legacy_class_hash and new_class_hash into declared_classes helps in testing the caching mechanism.
414-414: Ohayo, sensei! Retrieving compiled class confirms caching works.
Ensuring sp.compiled_class(class_hash) returns the expected class validates the caching functionality.
422-427: Assertion of compiled class correctness is accurate.
Validating that actual_compiled_class matches DEFAULT_ACCOUNT_CLASS_CASM confirms compilation and retrieval are functioning.
438-438: Ohayo, sensei! Retrieving legacy compiled class validates caching logic.
Getting actual_legacy_compiled_class confirms that compiled classes are correctly stored and accessed.
453-454: Assertion of new compiled Sierra class is precise.
Confirming that actual_new_compiled_class equals new_compiled_sierra_class verifies the integration of new classes into the cache.
crates/katana/rpc/rpc-types/src/transaction.rs (5)
4-4: Ohayo, sensei! Updated imports to include ContractClass
Including ContractClass from katana_primitives::class is necessary for handling the new class representation.
7-7: Ohayo, sensei! Correct import of legacy_rpc_to_class
The addition of legacy_rpc_to_class aligns with the new abstraction level.
98-109: Ohayo, sensei! Proper handling of DeclareTxV1 with new class abstraction
The code correctly uses legacy_rpc_to_class to obtain class_hash and class, and constructs DeclareTxWithClass accordingly.
113-125: Ohayo, sensei! Correct implementation for DeclareTxV2 with ContractClass
The changes appropriately obtain class_hash using tx.contract_class.class_hash() and create class using ContractClass::Class.
130-148: Ohayo, sensei! Consistent updates in DeclareTxV3 handling
The modifications for DeclareTxV3 maintain consistency with V1 and V2, ensuring uniform handling of transaction classes.
crates/katana/storage/provider/src/providers/fork/backend.rs (3)
15-15: Ohayo, sensei! Updated imports look good.
The addition of ContractClass to the imports aligns with the changes made in the codebase.
513-539: Ohayo, sensei! Caching logic for compiled classes is solid.
The implementation effectively checks the cache before fetching and compiling classes, optimizing performance.
482-510: Ensure all RpcContractClass variants are handled properly.
The match statement handles Legacy and Sierra variants, but it's important to ensure that all possible variants of RpcContractClass are considered to prevent future issues if new variants are added.
Run the following script to verify all variants of RpcContractClass are accounted for:
✅ Verification successful
RpcContractClass variants are handled comprehensively, sensei!
From our search results, we can see that RpcContractClass is imported from starknet::core::types and is used consistently throughout the code. The match statement in the provided code handles both known variants (Legacy and Sierra) exhaustively, as evidenced by the usage patterns we found:
RpcContractClass::Legacy(_)
RpcContractClass::Sierra(sierra_class)Since Rust's enum matching is exhaustive by default, the compiler would fail if any variant was missing. The current implementation is safe and future-proof as any new variants would trigger a compilation error, forcing developers to handle them explicitly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all variants of RpcContractClass to ensure complete handling.
# Expectation: Should only find Legacy and Sierra variants.
rg 'pub enum RpcContractClass' -A 10
Length of output: 38
Script:
#!/bin/bash
# Let's try searching in a different way to find the RpcContractClass definition
fd -e rs | xargs rg "RpcContractClass"
# Also search for enum definitions containing both Legacy and Sierra as variants
fd -e rs | xargs rg "enum.*\{.*Legacy.*Sierra" -A 5
Length of output: 1120
Script:
#!/bin/bash
# Let's check the starknet crate's types for RpcContractClass definition
ast-grep --pattern 'enum RpcContractClass {
$$$
}'
Length of output: 54
crates/katana/executor/src/implementation/blockifier/utils.rs (2)
49-49: Ohayo, sensei! The import statement update looks good
Importing StateUpdatesWithClasses ensures that the updated structures are correctly referenced. This aligns well with the refactoring goals.
400-400: Ohayo, sensei! Function return type updated appropriately
Updating the return type to StateUpdatesWithClasses reflects the new structure and simplifies data handling. This change is consistent and looks good.
crates/katana/primitives/src/genesis/json.rs (2)
28-30: Ensure Conditional Compilation Attributes are Consistent
Ohayo, sensei! The import statements for CONTROLLER_ACCOUNT_CLASS and CONTROLLER_CLASS_HASH on line 28 are conditionally included with #[cfg(feature = "slot")]. However, the constants DEFAULT_ACCOUNT_CLASS, DEFAULT_ACCOUNT_CLASS_HASH, and DEFAULT_ACCOUNT_COMPILED_CLASS_HASH on line 30 are not conditionally imported. Please verify if these constants should also be conditionally compiled to maintain consistency and prevent unnecessary inclusion when the slot feature is disabled.
357-357: Verify Class Hash Uniqueness When Inserting into classes
Ohayo, sensei! At line 357, classes are inserted into the classes map using class_hash as the key. Ensure that class_hash values are unique to prevent inadvertent overwriting of classes, which could lead to unexpected behavior.
crates/katana/rpc/rpc/src/starknet/mod.rs (2)
Line range hint 257-267: Excellent handling of ContractClass variants in class_at_hash, sensei!
The function correctly manages both Legacy and Class variants, converting them to StarknetRsContractClass appropriately. This ensures that different contract class types are properly supported.
290-290: Consistent return type in class_at_address, sensei!
Updating the return type to StarknetApiResult<StarknetRsContractClass> aligns this function with class_at_hash, ensuring consistent handling of contract classes across the API.
| class: ContractClass::Class(DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap()) | ||
| .into(), |
There was a problem hiding this comment.
Handle potential flatten() failures gracefully
The flatten().unwrap() calls could panic if the flattening operation fails. Consider handling these potential failures more gracefully.
- class: ContractClass::Class(DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap())
- .into(),
+ class: ContractClass::Class(DEFAULT_ACCOUNT_CLASS.clone().flatten()
+ .expect("Failed to flatten DEFAULT_ACCOUNT_CLASS"))
+ .into(),Similar changes should be applied to the CONTROLLER_ACCOUNT_CLASS flattening.
Also applies to: 133-136
| @@ -338,7 +338,7 @@ | |||
| }; | |||
|
|
|||
| let hash = TransactionHash(hash); | |||
| let class = to_class(contract_class).unwrap(); | |||
| let class = to_class(compiled).unwrap(); | |||
There was a problem hiding this comment.
Ohayo, sensei! Avoid using unwrap() to prevent potential panics
Using .unwrap() may lead to a panic if to_class(compiled) returns an error. Consider handling the potential error by using the ? operator or matching on the Result to manage the error case appropriately.
| class: ContractClass::Class(DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap()) | ||
| .into(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Potential Errors When Flattening in Genesis Conversion
Ohayo, sensei! In lines 905-906, flatten().unwrap() is used again. To prevent potential panics and improve error handling, consider using expect() with a descriptive message or properly handling the error.
Apply this diff to address the issue:
- class: ContractClass::Class(DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap())
+ class: ContractClass::Class(
+ DEFAULT_ACCOUNT_CLASS
+ .clone()
+ .flatten()
+ .expect("Failed to flatten DEFAULT_ACCOUNT_CLASS"),
+ )📝 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.
| class: ContractClass::Class(DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap()) | |
| .into(), | |
| class: ContractClass::Class( | |
| DEFAULT_ACCOUNT_CLASS | |
| .clone() | |
| .flatten() | |
| .expect("Failed to flatten DEFAULT_ACCOUNT_CLASS"), | |
| ) | |
| .into(), |
| class: ContractClass::Class(CONTROLLER_ACCOUNT_CLASS.clone().flatten().unwrap()) | ||
| .into(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Using unwrap() to Prevent Potential Panics
Ohayo, sensei! In lines 295-296, flatten().unwrap() is used when setting the class field. Using unwrap() can lead to a panic if flatten() returns an Err. To enhance robustness, consider handling the potential error or using expect() with a meaningful message.
Apply this diff to handle the error gracefully:
class: ContractClass::Class(
- CONTROLLER_ACCOUNT_CLASS.clone().flatten().unwrap()
+ CONTROLLER_ACCOUNT_CLASS
+ .clone()
+ .flatten()
+ .expect("Failed to flatten CONTROLLER_ACCOUNT_CLASS")
)
.into(),📝 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.
| class: ContractClass::Class(CONTROLLER_ACCOUNT_CLASS.clone().flatten().unwrap()) | |
| .into(), | |
| class: ContractClass::Class( | |
| CONTROLLER_ACCOUNT_CLASS | |
| .clone() | |
| .flatten() | |
| .expect("Failed to flatten CONTROLLER_ACCOUNT_CLASS") | |
| ) | |
| .into(), |
| class: ContractClass::Class( | ||
| CONTROLLER_ACCOUNT_CLASS.clone().flatten().unwrap(), | ||
| ) | ||
| .into(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Potential Errors When Flattening Controller Account Class
Ohayo, sensei! Similar to previous instances, lines 913-916 use flatten().unwrap() for CONTROLLER_ACCOUNT_CLASS. To enhance robustness, handle the potential error explicitly.
Apply this diff for better error handling:
class: ContractClass::Class(
- CONTROLLER_ACCOUNT_CLASS.clone().flatten().unwrap(),
+ CONTROLLER_ACCOUNT_CLASS
+ .clone()
+ .flatten()
+ .expect("Failed to flatten CONTROLLER_ACCOUNT_CLASS"),
)
.into(),📝 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.
| class: ContractClass::Class( | |
| CONTROLLER_ACCOUNT_CLASS.clone().flatten().unwrap(), | |
| ) | |
| .into(), | |
| class: ContractClass::Class( | |
| CONTROLLER_ACCOUNT_CLASS | |
| .clone() | |
| .flatten() | |
| .expect("Failed to flatten CONTROLLER_ACCOUNT_CLASS"), | |
| ) | |
| .into(), |
| class: ContractClass::Class( | ||
| DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap(), | ||
| ) | ||
| .into(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Potential Errors When Flattening DEFAULT_ACCOUNT_CLASS
Ohayo, sensei! The use of flatten().unwrap() on lines 389-392 can cause a panic if an error occurs during flattening. It's advisable to handle this potential error explicitly or provide a clear expect() message.
Apply this diff to improve error handling:
class: ContractClass::Class(
- DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap(),
+ DEFAULT_ACCOUNT_CLASS
+ .clone()
+ .flatten()
+ .expect("Failed to flatten DEFAULT_ACCOUNT_CLASS"),
)
.into(),📝 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.
| class: ContractClass::Class( | |
| DEFAULT_ACCOUNT_CLASS.clone().flatten().unwrap(), | |
| ) | |
| .into(), | |
| class: ContractClass::Class( | |
| DEFAULT_ACCOUNT_CLASS | |
| .clone() | |
| .flatten() | |
| .expect("Failed to flatten DEFAULT_ACCOUNT_CLASS"), | |
| ) | |
| .into(), |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
crates/katana/storage/provider/src/traits/contract.rs (1)
40-43: Consider enhancing documentation, sensei!The new
ContractClassWriterExttrait is a clean abstraction, but could benefit from additional documentation:
- Implementation requirements
- Expected behavior when compilation fails
- Relationship with the base trait's operations
Add documentation like this:
#[auto_impl::auto_impl(&, Box, Arc)] pub trait ContractClassWriterExt: ContractClassWriter { + /// Set the compiled class for the given class hash. + /// + /// # Implementation Requirements + /// - Should store the compiled class in a way that's efficiently retrievable + /// - Should maintain consistency with the base trait's class storage + /// + /// # Error Handling + /// - Should return an error if compilation verification fails + /// - Should maintain atomicity with related state updates fn set_compiled_class(&self, hash: ClassHash, class: CompiledClass) -> ProviderResult<()>; }crates/katana/storage/db/src/version.rs (1)
84-84: Consider enhancing version-related test coverage.While the basic version assertion is good, we should add more comprehensive tests.
Consider adding tests for:
- Version mismatch scenarios
- Migration paths (once implemented)
- Error handling cases
crates/katana/storage/provider/src/providers/db/state.rs (2)
196-205: Extract common class retrieval logicOhayo, sensei! There's significant code duplication between
HistoricalStateProviderandLatestStateProviderin their class retrieval logic.Consider extracting the common logic into a private helper function:
fn get_contract_class<T: DbTx>(tx: &T, hash: ClassHash) -> ProviderResult<Option<ContractClass>> { match tx.get::<tables::CompiledClasses>(hash)? { None => Ok(None), Some(CompiledClass::Class(..)) => { let class = tx.get::<tables::SierraClasses>(hash)?; Ok(class.map(ContractClass::Class)) } Some(CompiledClass::Legacy(class)) => Ok(Some(ContractClass::Legacy(class))), } }This would reduce duplication and make the code more maintainable.
223-229: Consider caching block declaration resultsOhayo, sensei! The block declaration check in
compiled_class_hash_of_class_hashcould benefit from caching, especially when multiple queries are made for the same class hash within the same block number.Consider implementing a simple LRU cache for block declaration results, keyed by
(class_hash, block_number). This would reduce database lookups for frequently accessed classes.crates/katana/storage/provider/src/providers/fork/mod.rs (1)
538-543: Clean extension trait implementation, sensei!The new
ContractClassWriterExttrait withset_compiled_classmethod provides a clean way to handle compiled classes when needed, maintaining good separation of concerns.This extension trait pattern allows for:
- Clear separation between basic class operations and compilation-specific operations
- Future extensibility without modifying the core trait
- Optional implementation by providers that need compilation capabilities
crates/katana/executor/src/implementation/blockifier/state.rs (1)
135-141: Consider caching compiled classes persistently, sensei!As mentioned in the PR objectives, there's an expected regression where classes need to be compiled twice. While this is acceptable for now, consider these future optimizations:
- Implement persistent caching of compiled classes
- Add background compilation to prepare compiled classes ahead of time
- Consider lazy loading strategies to defer compilation until absolutely necessary
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
crates/katana/executor/src/implementation/blockifier/state.rs(14 hunks)crates/katana/storage/db/src/version.rs(2 hunks)crates/katana/storage/provider/src/lib.rs(5 hunks)crates/katana/storage/provider/src/providers/db/state.rs(5 hunks)crates/katana/storage/provider/src/providers/fork/mod.rs(5 hunks)crates/katana/storage/provider/src/test_utils.rs(2 hunks)crates/katana/storage/provider/src/traits/contract.rs(1 hunks)crates/katana/storage/provider/src/traits/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/storage/provider/src/traits/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/storage/provider/src/test_utils.rs
🔇 Additional comments (16)
crates/katana/storage/provider/src/traits/contract.rs (3)
1-1: Ohayo! Import changes look good, sensei!
The addition of ContractClass to imports aligns well with the PR's objective of abstracting compiled class operations.
29-37: Clean and consistent changes, sensei!
The update to set_class() to use ContractClass maintains consistency with the provider's new abstraction approach.
8-18: Heads up about performance implications, sensei!
The default implementation of compiled_class() compiles the class on demand, which aligns with the PR objectives but introduces the expected performance regression mentioned in the PR description. As noted, this will cause double compilation during declare transactions.
Let's check for potential impact:
✅ Verification successful
Let's search for declare transaction handlers to understand the impact better:
Ohayo! The performance impact is expected and handled properly, sensei!
The default implementation in compiled_class() that compiles on demand is actually the intended behavior. Looking at the codebase:
- The storage providers (db, fork) have their own optimized implementations that store compiled classes
- The trait's default implementation is a fallback that ensures compatibility when compiled classes aren't cached
- The double compilation during declare transactions is an acknowledged trade-off mentioned in the PR description
The code shows proper separation between storage and execution concerns:
- Storage providers can optimize by caching compiled classes
- The trait provides a safe default for simpler implementations
- Performance-critical paths like the DB provider have their own optimized implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for declare transaction handlers to identify affected code paths
rg -A 5 "declare.*transaction"
Length of output: 137552
Script:
#!/bin/bash
# Search for declare transaction validation and execution code
ast-grep --pattern $'fn validate_declare_transaction($_, $_) {
$$$
}'
# Also search for compiled_class usage
rg -A 3 "compiled_class\("
Length of output: 15579
crates/katana/storage/provider/src/providers/db/state.rs (1)
73-82: Consider adding transaction isolation for sierra class storage
Ohayo, sensei! The implementation correctly handles both variants, but there's a potential race condition when storing sierra classes. If two transactions attempt to store the same class hash simultaneously, one might overwrite the other's data.
Let's verify the transaction isolation level:
Consider implementing optimistic locking or using a transaction with proper isolation level when storing sierra classes to prevent potential race conditions.
crates/katana/storage/provider/src/providers/fork/mod.rs (3)
13-13: Ohayo! Import changes align with the refactoring objectives.
The changes to imports reflect the shift from declared compiled classes to a more abstract representation, which aligns perfectly with the PR's goal of simplifying data structures.
Also applies to: 17-17
523-524: Ohayo sensei! ContractClass abstraction looks good.
The change from CompiledClass to ContractClass in set_class method properly implements the abstraction of compiled classes at the provider level.
469-469: Verify performance impact on declare transactions.
The change from StateUpdatesWithDeclaredClasses to StateUpdatesWithClasses aligns with the refactoring goals. However, as mentioned in the PR objectives, this will cause classes to be compiled twice during declare transactions.
Let's analyze the impact:
crates/katana/executor/src/implementation/blockifier/state.rs (2)
95-101: Ohayo! Clean structural changes, sensei!
The simplified data structure with separate declared_classes and compiled_classes maps aligns well with the PR's objective of deferring compilation operations to the provider level.
385-387: Skip: Error handling in tests already addressed.
A previous review comment already suggested handling compilation errors gracefully.
Also applies to: 458-460
crates/katana/storage/provider/src/lib.rs (7)
9-9: Ohayo sensei! Updated imports reflect new class definitions
The updated import statement correctly includes ContractClass, CompiledClass, and their associated hashes, aligning with the new class handling abstractions.
13-13: Included StateUpdatesWithClasses in imports
Importing StateUpdatesWithClasses is appropriate for the updated state update handling, reflecting the removal of declared_compiled_classes.
18-18: Added ContractClassWriterExt to imports
Including ContractClassWriterExt allows for extended functionality in contract class writing at the provider level, aligning with the abstraction refactor.
272-279: Implemented class and compiled_class methods in ContractClassProvider
The addition of the class method returning ContractClass and the compiled_class method enhances class retrieval functionality, delegating these operations to the provider level as intended.
326-326: Updated set_class method to accept ContractClass
Changing the parameter to ContractClass aligns with the goal of deferring compilation to the provider level, simplifying upstream operations and data structures.
339-344: Implemented ContractClassWriterExt for BlockchainProvider
By implementing set_compiled_class, we enable setting the CompiledClass directly, which is consistent with abstracting compiled class operations to the provider level.
134-134: Ensure all usages of insert_block_with_states_and_receipts are updated
The method signature now accepts states: StateUpdatesWithClasses. Please verify that all invocations of this method are updated to match the new signature to prevent any mismatches.
Run the following script to verify method usage:
✅ Verification successful
Let me check the method invocations to ensure they match the signature.
All usages of insert_block_with_states_and_receipts are correctly updated
Ohayo sensei! After checking all invocations, I can confirm that all calls to the method are using the correct parameter type StateUpdatesWithClasses. The method signature is consistently defined across all implementations (trait, fork provider, and db provider), and all calls either pass states directly or use StateUpdatesWithClasses::default().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all usages of `insert_block_with_states_and_receipts` have the updated parameter type.
# Expectation: All occurrences should use `StateUpdatesWithClasses` as the parameter type.
rg -A 3 'fn insert_block_with_states_and_receipts' --type rust
Length of output: 1453
Script:
#!/bin/bash
# Search for all calls to insert_block_with_states_and_receipts
rg "insert_block_with_states_and_receipts\(" --type rust -B 2 -A 2
Length of output: 6949
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
crates/katana/primitives/Cargo.toml (1)
28-28: Consider documenting the rationale for making flate2 a core dependency, sensei!Since this change makes flate2 a core dependency (previously optional), it would be helpful to document why it's now required for the primitives crate, especially in relation to the compiled class abstraction changes.
[dependencies] +# flate2 is required for handling compiled class compression/decompression +# as part of the provider-level class abstraction flate2.workspace = truecrates/katana/primitives/src/chain_spec.rs (1)
412-414: Consider using const for magic numbers.The assertions use magic numbers (3 and 4) for the expected length of classes. Consider defining these as constants to make the code more maintainable.
+ const EXPECTED_CLASSES_LEN: usize = 3; + const EXPECTED_CLASSES_LEN_WITH_SLOT: usize = 4; if cfg!(feature = "slot") { - assert!(actual_state_updates.classes.len() == 4); + assert!(actual_state_updates.classes.len() == EXPECTED_CLASSES_LEN_WITH_SLOT); } else { - assert!(actual_state_updates.classes.len() == 3); + assert!(actual_state_updates.classes.len() == EXPECTED_CLASSES_LEN); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/katana/primitives/Cargo.toml(1 hunks)crates/katana/primitives/src/chain_spec.rs(12 hunks)crates/katana/primitives/src/conversion/mod.rs(0 hunks)crates/katana/storage/provider/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/primitives/src/conversion/mod.rs
🔇 Additional comments (4)
crates/katana/primitives/Cargo.toml (1)
28-28: Ohayo! Let's verify the flate2 dependency usage.
The change from optional to core dependency aligns with the PR's goal of abstracting compiled class operations. However, let's verify its usage across the codebase.
✅ Verification successful
Ohayo! The flate2 dependency is correctly used for core functionality, sensei!
The verification shows that flate2 is actively used in the primitives crate for:
- Core contract class compression/decompression in
conversion/rpc.rs - Handling
CompressedLegacyContractClasstypes - Supporting GZIP compression via
GzEncoder
This aligns perfectly with the PR's goal of abstracting compiled class operations, making it a justified core dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how flate2 is used in the codebase
# Expected: Usage related to compiled class compression/decompression
# Search for flate2 usage
rg -l "use.*flate2"
# Search for specific compression/decompression operations
rg "(?:Compress|Decompress)"
Length of output: 7809
crates/katana/storage/provider/Cargo.toml (2)
11-11: Clean workspace inheritance implementation!
The simplified dependency declaration using .workspace = true aligns well with Rust's recommended practices for workspace management.
11-11: Ohayo sensei! Verify the removal of RPC feature.
The simplified workspace inheritance looks clean, but we should verify that removing the rpc feature doesn't break any functionality, especially given the PR's goal of deferring operations from RPC to provider level.
✅ Verification successful
Ohayo sensei! The RPC feature removal is safe and intentional.
The RPC imports in the fork provider are for external RPC client functionality (JsonRpcClient, HttpTransport) and RPC type conversions, which are different from the removed RPC feature. These are necessary for the fork provider to communicate with external nodes. The simplified workspace dependency is correct as the RPC feature was not essential for the provider's core functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if RPC features are defined in workspace
# and verify no direct RPC dependencies exist in the codebase
# Check workspace Cargo.toml for RPC features
echo "Checking workspace Cargo.toml for RPC features:"
rg -l "features.*=.*[\"']rpc[\"']" Cargo.toml
# Check for RPC usage in the provider
echo "Checking for RPC imports in provider code:"
rg -l "use.*rpc" crates/katana/storage/provider/src/
Length of output: 424
Script:
#!/bin/bash
# Let's check the actual RPC usage in the fork provider files
echo "Checking RPC usage in fork provider files:"
rg "rpc" crates/katana/storage/provider/src/providers/fork/mod.rs -B 2 -A 2
echo "\nChecking RPC usage in fork backend:"
rg "rpc" crates/katana/storage/provider/src/providers/fork/backend.rs -B 2 -A 2
# Check if katana-primitives has RPC features in its Cargo.toml
echo "\nChecking katana-primitives Cargo.toml for RPC features:"
rg "rpc" crates/katana/primitives/Cargo.toml
# Check workspace features for RPC
echo "\nChecking workspace features:"
cat Cargo.toml | grep -A 5 "\[workspace.features\]" || echo "No workspace features section found"
Length of output: 2231
crates/katana/primitives/src/chain_spec.rs (1)
77-84: LGTM! Clean implementation of state updates.
The refactoring to use classes directly instead of declared_compiled_classes aligns well with the PR objective of deferring class compilation to the provider level.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2709 +/- ##
==========================================
+ Coverage 56.17% 56.23% +0.05%
==========================================
Files 411 410 -1
Lines 52958 52892 -66
==========================================
- Hits 29749 29743 -6
+ Misses 23209 23149 -60 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/storage/provider/src/providers/db/state.rs (1)
122-132: Consider optimizing table structure as noted in TODO.Ohayo, sensei! The current implementation requires multiple table lookups for Class variants. As noted in the TODO, storing ContractClass directly could improve performance.
Would you like help designing an optimized table structure that reduces the number of lookups?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
crates/katana/executor/src/implementation/blockifier/state.rs(14 hunks)crates/katana/storage/provider/src/providers/db/mod.rs(6 hunks)crates/katana/storage/provider/src/providers/db/state.rs(5 hunks)crates/saya/core/src/blockchain/mod.rs(3 hunks)crates/saya/provider/src/provider.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/katana/executor/src/implementation/blockifier/state.rs
- crates/saya/core/src/blockchain/mod.rs
- crates/saya/provider/src/provider.rs
🔇 Additional comments (7)
crates/katana/storage/provider/src/providers/db/state.rs (4)
9-9: LGTM! Import changes align with the PR objectives.
The addition of ContractClass import supports the abstraction of compiled classes at the provider level.
99-105: Previous review comment about class hash validation still applies.
Line range hint 197-234: LGTM! Robust historical state validation.
Ohayo, sensei! The implementation properly validates class existence against block numbers and handles both class variants correctly. The additional checks ensure historical state consistency.
73-82: LGTM! Class storage logic properly handles both variants.
Ohayo, sensei! The implementation correctly stores different class types in their respective tables. Let's verify the class storage behavior across the codebase.
✅ Verification successful
Let me gather more information about the class storage implementation to ensure consistency.
Storage implementation for both class variants is consistent and correct
Ohayo, sensei! After reviewing the implementation patterns across the codebase, I can confirm that:
- The storage logic correctly handles both Sierra and Legacy classes in their respective tables
- The retrieval logic in
get_classmatches the storage pattern, properly reconstructingContractClassvariants - The pattern is consistently used across the codebase, including the DB provider and state implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify class storage patterns and usage
# Check for any potential class storage inconsistencies
# Look for direct table access patterns
rg -A 3 "put::<tables::SierraClasses>|put::<tables::CompiledClasses>"
# Check for proper class variant handling
ast-grep --pattern 'match $_ {
ContractClass::Class($_) => $$$,
ContractClass::Legacy($_) => $$$
}'
Length of output: 2585
Script:
#!/bin/bash
# Check for class retrieval patterns to ensure they match storage patterns
rg -A 3 "get::<tables::SierraClasses>|get::<tables::CompiledClasses>"
# Look for any other ContractClass enum usage
ast-grep --pattern 'ContractClass::$_'
# Check for any other class-related table operations
rg -A 3 "tables::SierraClasses|tables::CompiledClasses"
Length of output: 10602
crates/katana/storage/provider/src/providers/db/mod.rs (3)
30-30: LGTM! Import change aligns with PR objectives.
The change from StateUpdatesWithDeclaredClasses to StateUpdatesWithClasses reflects the architectural shift to handle compiled classes at the provider level.
614-614: Ohayo sensei! Implementation looks solid with proper error handling.
The changes successfully move class compilation to the provider level and handle both Sierra and compiled classes appropriately. The error handling uses the ? operator as recommended, avoiding potential panics.
Also applies to: 663-671
792-792: Test updates maintain coverage while adapting to new types.
The test helper functions have been properly updated to use StateUpdatesWithClasses while maintaining the same test scenarios and coverage.
Also applies to: 817-843
defer any compiled class (ie casm) operations to the provider abstraction level instead of requiring class compilation to happen on the rpc level (current behaviour).
motivation for this is to simplify the data structures as compiled classes are only ever used by the executor. so we're unnecessarily enforcing the class to be compiled at a very early stage throughout the whole node operations.
this pr also removes the
declared_compiled_classesfield fromStateUpdatesWithDeclaredClassesstruct as the compiled format can still be derived from the classes.this changes include an expected regression anytime a declare tx is executed. because now the class has to be compiled twice, first when its being executed and later once the state update is being stored in the db. i believe we can easily optimize for this later but its very important to make sure the data structures and the flow simple first.