feat: add sozo stats back #2684
Conversation
WalkthroughOhayo, sensei! This pull request introduces several modifications across various files, primarily focusing on enhancing the handling of CASM classes and updating dependencies in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
crates/dojo/world/src/local/resource.rs (2)
25-26: Nice addition of CASM class stats, sensei!The optional CASM class field is well-documented and appropriately typed. Since this is used for stats and size limits, consider adding size validation methods to help implement the warnings mentioned in the PR objectives.
Consider adding helper methods to
CommonLocalInfofor:
- Getting Sierra file size
- Getting CASM bytecode size
- Implementing size limit checks with appropriate warning thresholds
152-160: The common() method looks solid, but let's enhance the docs, sensei!The implementation is clean and follows Rust patterns, but the panic behavior for Namespace variant should be explicitly documented.
Consider updating the method documentation like this:
/// Returns the common information of the resource. + /// + /// # Panics + /// + /// Panics if called on a `ResourceLocal::Namespace` variant since namespaces + /// don't have common information. pub fn common(&self) -> &CommonLocalInfo {crates/dojo/world/src/local/mod.rs (2)
33-34: Field addition looks good, sensei!The
casm_classfield is well-placed and correctly typed asOption<CompiledClass>. Consider enhancing the documentation to explain when this field might beNoneand its relationship withcasm_class_hash./// The casm class of the world. + /// This field might be None when [explain conditions]. + /// When present, it corresponds to the compiled version of the Sierra class + /// and is used to compute the casm_class_hash. pub casm_class: Option<CompiledClass>,
162-162: Consider enhancing test coverage, sensei!While the
casm_classfield is correctly initialized in the test cases, consider adding test cases that cover:
- Initialization with
Some(CompiledClass)- Behavior differences between
NoneandSomecasesAlso applies to: 179-179
crates/dojo/world/src/local/artifact_to_local.rs (1)
52-58: Consider adding error context for CASM class loadingWhile the CASM class loading is implemented correctly, consider enhancing the error context when deserialization fails.
let casm_class = if casm_path.exists() { - Some(serde_json::from_reader::<_, CompiledClass>(std::fs::File::open( - &casm_path, - )?)?) + Some(serde_json::from_reader::<_, CompiledClass>(std::fs::File::open(&casm_path)?) + .map_err(|e| anyhow::anyhow!("Failed to parse CASM class at {}: {}", casm_path.display(), e))?) } else { None };crates/dojo/world/src/diff/mod.rs (1)
382-382: Ohayo sensei! Consider enhancing test coverage for casm_class field.While the initialization of
casm_classasNoneis correct, we might want to add test cases that cover scenarios wherecasm_classhas a value to ensure the comparison logic works correctly in all cases.Consider adding a test case like this:
#[test] fn test_world_diff_with_casm_class() { let ns = "ns".to_string(); let namespace_config = NamespaceConfig::new(&ns); let profile_config = ProfileConfig::new("test", "seed", namespace_config.clone()); let mut local = WorldLocal::new(profile_config.clone()); let mut remote = WorldRemote::default(); let local_contract = ResourceLocal::Contract(ContractLocal { common: CommonLocalInfo { name: "c".to_string(), namespace: ns.clone(), class: empty_sierra_class(), casm_class: Some(/* initialize with appropriate CasmClass */), class_hash: Felt::ONE, casm_class_hash: Felt::ZERO, }, systems: vec![], }); // Add assertions to verify casm_class handling }Also applies to: 421-421
bin/sozo/src/commands/build.rs (6)
139-139: Consider handling errors instead of usingexpect().Ohayo, sensei! Using
.expect("Error generating bindings")can cause a panic if an error occurs during binding generation. It's preferable to handle the error gracefully or propagate it using?to improve robustness.Consider modifying the code as follows:
- config.tokio_handle().block_on(bindgen.generate(None)).expect("Error generating bindings"); + config.tokio_handle().block_on(bindgen.generate(None))?;
144-144: Avoid usingunwrap()on potentially failing operations.Ohayo, sensei! Calling
.unwrap()onws.load_profile_config()can lead to a panic if the configuration fails to load. It's better to handle the error or propagate it to maintain application stability.Modify the code to handle the error:
- ws.load_profile_config().unwrap(), + ws.load_profile_config()?,
156-164: Ensure consistent sorting by setting a default sort order.Currently, if no sorting option is selected in
StatOptions, the statistics are displayed in the order they were added. To enhance user experience, consider setting a default sorting criterion or informing the user about the current sort order.You might add a default sort, such as sorting by tag:
+ } else { + stats.sort_by_key(|s| s.tag.clone()); }
172-185: Adjust the indentation of the multi-line warning message.Ohayo, sensei! The indentation within the multi-line string may cause formatting issues in the output, affecting readability.
Consider aligning the text to the left margin for better display:
- r#" - All the casm bytecode sizes are 0, did you forget to enable casm compilation? - To enable casm compilation, add the following to your Scarb.toml: - - [[target.starknet-contract]] - sierra = true - casm = true - "# + r#"All the casm bytecode sizes are 0, did you forget to enable casm compilation? +To enable casm compilation, add the following to your Scarb.toml: +[[target.starknet-contract]] +sierra = true +casm = true"#
235-236: Document the source of the limit constants.Ohayo, sensei! The constants
MAX_SIERRA_SIZE_BYTESandMAX_CASM_FELTSare used without context. Providing documentation or references enhances maintainability and helps future contributors understand their significance.Consider adding comments or linking to official documentation:
const MAX_SIERRA_SIZE_BYTES: usize = 4_089_446; // Max Sierra size in bytes as per StarkNet docs const MAX_CASM_FELTS: usize = 81_290; // Max Casm felts allowed on public networks
280-284: Optimize file size calculation without reserialization.Ohayo, sensei! Reserializing the class to JSON to determine the file size can be inefficient. If the original serialized file is accessible, reading its metadata directly would improve performance.
Consider accessing the file size using
std::fs::metadata:- Ok(serde_json::to_string(&self.common().class)?.len()) + let path = self.common().sierra_path(); + Ok(std::fs::metadata(path)?.len() as usize)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
bin/sozo/Cargo.toml(2 hunks)bin/sozo/src/commands/build.rs(4 hunks)crates/dojo/world/src/contracts/contract_info.rs(1 hunks)crates/dojo/world/src/diff/compare.rs(3 hunks)crates/dojo/world/src/diff/mod.rs(2 hunks)crates/dojo/world/src/local/artifact_to_local.rs(7 hunks)crates/dojo/world/src/local/mod.rs(5 hunks)crates/dojo/world/src/local/resource.rs(3 hunks)examples/simple/Scarb.toml(1 hunks)
🔇 Additional comments (15)
bin/sozo/Cargo.toml (2)
38-38: Ohayo! The addition of serde_json looks good, sensei!
This addition aligns perfectly with the PR's objective of bringing back sozo stats and handling size limits, as it will enable proper JSON serialization of the statistics.
58-58: Nice optimization moving tokio to dev-dependencies!
Moving tokio to dev-dependencies is a good practice since it's only needed for testing purposes. This helps reduce the production dependency footprint.
crates/dojo/world/src/local/resource.rs (1)
2-2: Ohayo! Import looks good, sensei!
The addition of CompiledClass to the imports is well-placed and necessary for the new CASM class functionality.
crates/dojo/world/src/local/mod.rs (2)
14-14: Ohayo sensei! Clean import addition.
The CompiledClass import is well-placed alongside the existing SierraClass import from the same module.
68-68: Default implementation is properly updated, sensei!
The casm_class field is correctly initialized to None in the Default implementation, maintaining consistency with the struct definition.
crates/dojo/world/src/diff/compare.rs (3)
96-99: Ohayo! The initialization looks good, sensei!
The addition of casm_class: None to CommonLocalInfo is consistent with the new structure requirements and maintains proper test coverage.
131-134: The event test initialization is properly aligned, sensei!
The casm_class field initialization maintains consistency with other test cases.
180-183: Consider verifying CASM class initialization scenarios, sensei!
While initializing casm_class as None is consistent across all test cases, we should verify if there are scenarios where it should be Some.
crates/dojo/world/src/local/artifact_to_local.rs (6)
4-4: Ohayo! Clean import addition for PathBuf
The addition of PathBuf to the Path import is well-aligned with its usage in CASM path handling.
36-38: LGTM! World state initialization
The new world_casm_class variable is properly initialized as None, maintaining consistency with other optional world state variables.
79-79: Clean world CASM class assignment
The assignment of world_casm_class is properly placed within the World resource type handling.
99-99: Consistent CASM class integration across resource types
The casm_class field is consistently added to Contract, Model, and Event resources with proper cloning.
Also applies to: 125-125, 151-151
187-187: LGTM! World initialization with CASM class
The world initialization properly includes the new casm_class field.
195-202: Ohayo sensei! Enhanced error message readability
The error message for missing world artifact has been improved with a clear, formatted example using raw strings. This makes it much more user-friendly!
bin/sozo/src/commands/build.rs (1)
306-310: Optimize file size calculation without reserialization.
Ohayo, sensei! Similar to the previous suggestion, avoid reserializing the class for size calculation in WorldLocal.
Apply the same optimization here:
- Ok(serde_json::to_string(&self.class)?.len())
+ let path = self.sierra_path();
+ Ok(std::fs::metadata(path)?.len() as usize)| @@ -190,6 +190,7 @@ mod tests { | |||
| }, | |||
| abi: vec![], | |||
| }, | |||
| casm_class: None, | |||
There was a problem hiding this comment.
💡 Codebase verification
ohayo! sensei!
Inconsistency Found in CommonLocalInfo Struct Definitions
The casm_class field is missing in some CommonLocalInfo struct definitions. Please ensure all instances include the casm_class field for consistency.
- crates/dojo/world/src/local/resource.rs
- crates/dojo/world/src/contracts/contract_info.rs
🔗 Analysis chain
Ohayo! The CASM class field addition looks good, sensei!
The addition of casm_class: None in the test's CommonLocalInfo structure aligns well with the PR's objective of enhancing CASM class handling.
Let's verify the consistency of this field across related files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent CommonLocalInfo structure usage
# Expected: All CommonLocalInfo structs should have casm_class field
# Search for CommonLocalInfo struct definitions
ast-grep --pattern 'struct CommonLocalInfo {
$$$
}'
# Search for CommonLocalInfo instantiations
rg -A 5 'CommonLocalInfo \{'
Length of output: 6953
| let casm_path = PathBuf::from( | ||
| path.to_string_lossy() | ||
| .to_string() | ||
| .replace("contract_class.json", "compiled_contract_class.json"), | ||
| ); | ||
|
|
||
| let casm_class = if casm_path.exists() { | ||
| Some(serde_json::from_reader::<_, CompiledClass>(std::fs::File::open( | ||
| &casm_path, | ||
| )?)?) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei!
⚠ Missing compiled_contract_class.json for a contract_class.json file
Please ensure the following contract_class.json files have corresponding compiled_contract_class.json files:
- crates/katana/contracts/build/controller_CartridgeAccount.contract_class.json → crates/katana/contracts/build/controller_CartridgeAccount.compiled_contract_class.json
🔗 Analysis chain
Verify CASM path construction reliability
The CASM path construction logic assumes a specific file naming pattern. While the implementation is correct, we should ensure robustness across different environments.
Let's verify the file naming assumptions:
Ohayo, sensei! It seems the previous script is mistakenly picking up Makefile files. Let's refine our search to accurately identify all contract_class.json files and their corresponding compiled_contract_class.json files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent contract class file naming patterns
# Expected: All contract class files should follow the pattern we're replacing
# Search for any contract class files that don't match our expected pattern
rg -l "contract_class\.json$" | while read -r file; do
# Check if the corresponding compiled class exists with our expected naming
compiled_file="${file/contract_class.json/compiled_contract_class.json}"
if [ -f "$compiled_file" ]; then
echo "✓ Found matching pair: $file -> $compiled_file"
else
echo "⚠ Missing compiled class: $file -> $compiled_file"
fi
done
Length of output: 748
Script:
#!/bin/bash
# Description: Verify all 'contract_class.json' files have corresponding 'compiled_contract_class.json' files
# Expected: Every 'contract_class.json' should have a 'compiled_contract_class.json' counterpart
fd -e json -g '*contract_class.json' | while read -r file; do
compiled_file="${file/contract_class.json/compiled_contract_class.json}"
if [ -f "$compiled_file" ]; then
echo "✓ Found matching pair: $file -> $compiled_file"
else
echo "⚠ Missing compiled class: $file -> $compiled_file"
fi
done
Length of output: 668
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2684 +/- ##
==========================================
- Coverage 57.57% 57.47% -0.10%
==========================================
Files 404 404
Lines 51139 51271 +132
==========================================
+ Hits 29443 29469 +26
- Misses 21696 21802 +106 ☔ View full report in Codecov by Sentry. |
Limits are warned to the user for the size of Sierra file and Casm byte code size.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation