fix(sozo): add back -vvv and add label to class declaration #2675
Conversation
glihm
changed the title
WalkthroughOhayo, sensei! This pull request introduces several changes across multiple files. Key modifications include updates to the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
bin/sozo/src/args.rs (1)
58-65: Consider extracting log filter strings to constants, sensei!The log filter strings could be moved to constants to improve maintainability and make it easier to update logging configurations.
+ const VERBOSE_LOG_FILTER: &str = "info,hyper=off,scarb=off,salsa=off,sozo=trace,dojo_world=trace,dojo_utils=trace,sozo_ops=trace"; + const DEFAULT_LOG_FILTER: &str = "info,hyper=off,scarb=off,salsa=off,sozo=info,dojo_world=info"; pub fn init_logging(&self, clap_verbosity: &clap_verbosity_flag::Verbosity) -> Result<(), Box<dyn std::error::Error>> { let verbose = clap_verbosity.log_level_filter().as_trace() >= LevelFilter::DEBUG; - let default_log_filter: &str = if verbose { - "info,hyper=off,scarb=off,salsa=off,sozo=trace,dojo_world=trace,dojo_utils=trace,\ - sozo_ops=trace" - } else { - "info,hyper=off,scarb=off,salsa=off,sozo=info,dojo_world=info" - }; + let default_log_filter = if verbose { VERBOSE_LOG_FILTER } else { DEFAULT_LOG_FILTER };crates/dojo/utils/src/tx/declarer.rs (1)
Line range hint
90-104: Consider enhancing error logging.While the success path is well-logged, the error case could benefit from more detailed tracing.
Consider adding more context to the error case:
match account.provider().get_class(BlockId::Tag(BlockTag::Pending), class_hash).await { Err(ProviderError::StarknetError(StarknetError::ClassHashNotFound)) => {} Ok(_) => { tracing::trace!( label = labeled_class.label, class_hash = format!("{:#066x}", class_hash), "Class already declared." ); return Ok(TransactionResult::Noop); } - Err(e) => return Err(TransactionError::Provider(e)), + Err(e) => { + tracing::debug!( + label = labeled_class.label, + class_hash = format!("{:#066x}", class_hash), + error = ?e, + "Failed to check if class is declared" + ); + return Err(TransactionError::Provider(e)); + } }crates/sozo/ops/src/migrate/mod.rs (1)
469-475: Ohayo, sensei! Consider refactoring duplicated code for inserting 'LabeledClass'.The code for creating and inserting
LabeledClassinto theclassesHashMap is duplicated across multiple functions. To embrace the DRY principle and improve maintainability, consider refactoring this logic into a helper function.For example, you could create a function:
fn create_labeled_class( tag: &str, casm_class_hash: Felt, class: FlattenedSierraClass, ) -> LabeledClass { LabeledClass { label: tag.to_string(), casm_class_hash, class, } }And then use it:
-classes.insert( - casm_class_hash, - LabeledClass { label: tag.clone(), casm_class_hash, class }, -); +classes.insert(casm_class_hash, create_labeled_class(&tag, casm_class_hash, class));Also applies to: 496-502, 535-541, 561-566, 602-608, 628-633
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
bin/sozo/src/args.rs(1 hunks)bin/sozo/src/main.rs(1 hunks)crates/dojo/utils/src/tx/declarer.rs(5 hunks)crates/sozo/ops/src/migrate/mod.rs(16 hunks)examples/simple/manifest_dev.json(6 hunks)
🔇 Additional comments (17)
bin/sozo/src/main.rs (1)
20-20: Ohayo sensei! The logging initialization looks good!
The update to init_logging correctly passes the verbosity flag, aligning with the new method signature.
Let's verify the verbosity behavior:
✅ Verification successful
Ohayo sensei! The verbosity implementation is consistent across the codebase!
The search results confirm proper verbosity handling:
args.rscorrectly processes verbosity levels and initializes loggingui_verbosity()is consistently used across different components- The verbosity flag is properly propagated to the logging system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent verbosity handling across the codebase
# Test: Look for other verbosity-related code to ensure consistent implementation
rg -A 3 "verbose|verbosity" --type rust
Length of output: 2903
bin/sozo/src/args.rs (2)
54-57: Ohayo! Clean method signature update, sensei!
The explicit verbosity parameter improves the function's interface design by making the dependency clear.
72-72: Clean fallback handling, sensei!
The use of unwrap_or_else with the default filter provides a robust fallback mechanism.
crates/dojo/utils/src/tx/declarer.rs (4)
22-30: Ohayo! Clean and well-structured struct definition, sensei!
The LabeledClass struct is well-documented and follows good Rust practices by grouping related data together. The public fields make sense for this use case.
43-43: LGTM! Consistent type update.
The change to use LabeledClass in the HashMap maintains the same key type while enriching the value with additional metadata.
56-64: Elegant implementation using HashMap's entry API, sensei!
The methods are concise and handle duplicate entries correctly. The use of entry().or_insert() is idiomatic Rust.
107-112: Comprehensive tracing implementation!
The tracing logs provide excellent visibility with all relevant fields (label, class_hash, casm_class_hash).
examples/simple/manifest_dev.json (4)
2131-2131: Event handlers updated with new implementations.
The class_hash values for two events have been updated:
- ns-E: Updated to 0x943620824729c411...
- ns-EH: Updated to 0x5ed10e08c25eb6a1...
The selectors remain unchanged, maintaining backward compatibility.
Also applies to: 2137-2137
Line range hint 1-2138: Overall manifest structure maintained.
The manifest maintains its structural integrity while updating deployment configurations:
- All required sections (world, contracts, models, events) are present
- Selector values remain consistent
- ABIs are well-defined and unchanged
1246-1247: Contract deployments updated with new addresses.
Multiple contracts have been updated with new addresses and class_hashes. Notably:
- Two contracts (at lines 1247 and 1857) share the same class_hash (0x340e197b0...)
- Systems and init_calldata configurations remain consistent
Let's verify the contract configurations:
Also applies to: 1504-1504, 1680-1680, 1856-1857
✅ Verification successful
Contract class hash reuse is valid and intentional sensei!
The verification shows that the same class hash (0x340e197b...) is used for two different contract addresses in manifest_dev.json, which is a valid pattern. This indicates both contracts are instances of the same contract class but deployed at different addresses, similar to how multiple instances of the same class can be created in object-oriented programming. The configurations are consistent and properly structured in the manifest file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for contract class hash references
# Look for shared class hash usage
rg --type json "0x340e197b0fac61961591acdd872a89b0cb862893272ab72455356f5534baa7e"
Length of output: 328
Script:
#!/bin/bash
# Let's check the context around these class hash usages to understand their purpose
rg --type json -B 2 -A 2 "0x340e197b0fac61961591acdd872a89b0cb862893272ab72455356f5534baa7e"
# Also check for any references to these addresses in other files
rg "0x366a86098f39b0c3e026067c846367e83111c727b88fa036597120d154d44f5"
Length of output: 1038
3-4: Ohayo sensei! World contract upgrade detected.
The world contract has been updated with new class_hash and address values. These changes suggest a contract upgrade or redeployment.
Let's verify the world contract deployment:
crates/sozo/ops/src/migrate/mod.rs (6)
24-24: Ohayo, sensei! Ensure 'LabeledClass' is properly imported.
The addition of LabeledClass to the imports is appropriate for the new code changes.
305-305: Ohayo, sensei! Initialize 'classes' with the updated type.
Changing the type of classes to HashMap<Felt, LabeledClass> aligns with the new structure and usage.
363-363: Ohayo, sensei! Update 'extend_classes' to accept 'LabeledClass'.
Passing Vec<LabeledClass> to declarer.extend_classes ensures compatibility with the updated method signature.
376-378: Ohayo, sensei! Distribute classes among declarers efficiently.
Using a round-robin assignment to distribute LabeledClass instances across declarers is effective.
453-453: Ohayo, sensei! Update function signatures to return 'LabeledClass'.
The updated return types in contracts_calls_classes, models_calls_classes, and events_calls_classes now include HashMap<Felt, LabeledClass>, which is consistent with the new class structure.
Also applies to: 519-519, 586-586
660-665: Ohayo, sensei! Ensure 'LabeledClass' for the world is correctly initialized.
Initializing labeled_class for the world with the proper label and class data is appropriate.
Also applies to: 717-722
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2675 +/- ##
==========================================
+ Coverage 57.40% 57.59% +0.18%
==========================================
Files 403 404 +1
Lines 50833 51101 +268
==========================================
+ Hits 29183 29433 +250
- Misses 21650 21668 +18 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
LabeledClassstructure for improved class declaration clarity.Bug Fixes
Documentation
manifest_dev.jsonwith newclass_hashandaddressvalues for various entities, reflecting recent changes in contract configurations.Chores
LabeledClassstructure across multiple modules, enhancing traceability and organization.