feat(sozo): add model command back #2618
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes across several files, primarily focusing on the type safety and handling of contract identifiers by replacing 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: 4
🧹 Outside diff range and nitpick comments (8)
crates/sozo/ops/src/resource_descriptor.rs (3)
1-15: Ohayo! Consider enhancing documentation with examples, sensei!The enum definition looks clean and well-structured! To make it even more helpful for other developers, consider adding usage examples in the documentation.
Add examples like:
/// Examples: /// ``` /// use sozo_ops::ResourceDescriptor; /// /// let address = ResourceDescriptor::from_string("0x1234")?; /// let name = ResourceDescriptor::from_string("my_contract")?; /// let tag = ResourceDescriptor::from_string("namespace-contract")?; /// ```
23-31: Consider strengthening address validation, sensei!While the current address parsing works, it might be good to add additional validation for the hex string format beyond just checking the "0x" prefix.
Here's a suggested improvement:
pub fn from_string(s: &str) -> Result<Self> { if s.starts_with("0x") { + // Validate hex string format + if !s[2..].chars().all(|c| c.is_ascii_hexdigit()) { + anyhow::bail!("Invalid hex string format"); + } Ok(ResourceDescriptor::Address(Felt::from_str(s)?)) } else if naming::is_valid_tag(s) { Ok(ResourceDescriptor::Tag(s.to_string())) } else { Ok(ResourceDescriptor::Name(s.to_string())) } }
55-63: Extract magic number into a constant, sensei!The Display implementation looks good, but the hex width (66) should be extracted into a named constant for better maintainability.
Consider this improvement:
+const ADDRESS_HEX_WIDTH: usize = 66; + impl fmt::Display for ResourceDescriptor { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - ResourceDescriptor::Address(address) => write!(f, "{:#066x}", address), + ResourceDescriptor::Address(address) => write!(f, "{:#0width$x}", address, width = ADDRESS_HEX_WIDTH), ResourceDescriptor::Name(name) => write!(f, "{}", name), ResourceDescriptor::Tag(tag) => write!(f, "{}", tag), } } }bin/sozo/src/commands/model.rs (1)
122-186: Consider refactoring repetitive code in command handlersSensei, the code for each command variant repeats similar logic:
- Ensuring the namespace with
tag_or_name.ensure_namespace(&default_ns)- Retrieving
world_diffandproviderusingutils::get_world_diff_and_provider- Calling the respective
modelfunction with similar parametersRefactoring this repetitive code into a common function or utility could improve maintainability and reduce duplication.
crates/sozo/ops/src/model.rs (4)
Line range hint
205-340: Ohayo, sensei! Consider refactoring to reduce code duplicationThe functions
get_printable_layout_list_from_struct,get_printable_layout_list_from_enum,get_printable_layout_list_from_tuple, andget_printable_layout_list_from_arrayshare similar logic for processing layouts. Refactoring these into a generic function or utilizing traits can improve maintainability and reduce code duplication.
Line range hint
425-433: Ohayo, sensei! Add checks to prevent panics informat_arrayIn
format_array, accessingvalueswithout ensuring it has sufficient elements can lead to panics. It's important to verify the length ofvaluesbefore removing elements or iterating over it.Apply this diff to add necessary checks:
fn format_array(item: &Ty, values: &mut Vec<Felt>, level: usize, start_indent: bool) -> String { - let length: u32 = values.remove(0).to_u32().unwrap(); + if values.is_empty() { + return format!("{}Error: insufficient values for array length", _start_indent(level, start_indent)); + } + let length = values.remove(0).to_u32().unwrap(); let mut items = vec![]; for _ in 0..length { + if values.is_empty() { + items.push(format!("{}Error: insufficient values for array item", INDENT.repeat(level + 1))); + break; + } items.push(format_record_value(item, values, level + 1, true)); } format!( "{}[\n{}\n{}]", _start_indent(level, start_indent), items.join(",\n"), INDENT.repeat(level) ) }
Line range hint
475-482: Ohayo, sensei! Add bounds checking informat_enumto prevent panicsAccessing
schema.options[variant_index]without verifying thatvariant_indexis within bounds can cause a panic if the index is invalid. Ensure that you check the bounds before accessing the array.Apply this diff to include bounds checking:
fn format_enum(schema: &Enum, values: &mut Vec<Felt>, level: usize, start_indent: bool) -> String { let variant_index: u8 = values.remove(0).to_u8().unwrap(); let variant_index: usize = variant_index.into(); + if variant_index >= schema.options.len() { + return format!("{}Error: invalid variant index {}", _start_indent(level, start_indent), variant_index); + } let variant_name = format!("{}::{}", schema.name, schema.options[variant_index].name); let variant_data = format_record_value(&schema.options[variant_index].ty, values, level + 1, true); if variant_data.is_empty() { format!("{}{variant_name}", _start_indent(level, start_indent),) } else { format!( "{}{variant_name}(\n{}\n{})", _start_indent(level, start_indent), variant_data, INDENT.repeat(level) ) } }
Line range hint
402-408: Ohayo, sensei! Clarify cloning behavior informat_primitiveIn
format_primitive, cloning the primitive type withlet mut _p = *p;may not be necessary ifPrimitiveimplements theCopytrait. Ensure that this cloning is intentional and consider using references to avoid unnecessary copies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
bin/sozo/src/commands/call.rs(4 hunks)bin/sozo/src/commands/execute.rs(4 hunks)bin/sozo/src/commands/mod.rs(5 hunks)bin/sozo/src/commands/model.rs(7 hunks)bin/sozo/src/utils.rs(0 hunks)crates/sozo/ops/Cargo.toml(1 hunks)crates/sozo/ops/src/lib.rs(1 hunks)crates/sozo/ops/src/model.rs(14 hunks)crates/sozo/ops/src/resource_descriptor.rs(1 hunks)
💤 Files with no reviewable changes (1)
- bin/sozo/src/utils.rs
✅ Files skipped from review due to trivial changes (1)
- crates/sozo/ops/src/lib.rs
🔇 Additional comments (16)
crates/sozo/ops/Cargo.toml (1)
16-16: Ohayo! The dependency change looks good, sensei! ✨
Moving dojo-types to regular dependencies aligns perfectly with the introduction of the ResourceDescriptor type and the model command functionality.
crates/sozo/ops/src/resource_descriptor.rs (1)
1-63: Ohayo! Overall implementation looks solid, sensei!
The ResourceDescriptor implementation provides a robust and type-safe way to handle different types of resource identifiers. The code is well-structured, properly documented, and follows Rust best practices. The suggested improvements are minor and optional.
bin/sozo/src/commands/mod.rs (4)
17-17: LGTM! Module declaration follows consistent pattern.
Ohayo sensei! The new model module declaration is properly placed and follows the existing pattern.
29-29: LGTM! Import follows consistent pattern.
The ModelArgs import maintains consistency with other command argument imports.
53-54: LGTM! Command declaration follows best practices.
The Model command is properly documented and uses Box<ModelArgs> following the established pattern for complex command arguments.
69-69: LGTM! Command handling is properly implemented.
The new Model command is correctly integrated into both the Display implementation and command execution logic.
Let's verify that the new command follows the same pattern as other boxed commands:
Also applies to: 92-92
✅ Verification successful
The previous pattern might have been too specific. Let's look at the actual implementation to verify the command handling consistency.
Ohayo sensei! The Model command implementation is perfectly consistent
Looking at the output, the Model command follows the exact same pattern as all other commands in the codebase:
- It's properly defined in the Commands enum
- It has the same Display implementation format:
Commands::Model(_) => write!(f, "Model") - It follows the same execution pattern:
Commands::Model(args) => args.run(config)
This consistency ensures the command integrates seamlessly with the existing command infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistency of command implementations
# Test: Check if all boxed commands follow the same pattern
ast-grep --pattern 'Commands::$_($_(Box<$_>))'
Length of output: 48
Script:
#!/bin/bash
# Let's check the Commands enum definition and its variants
rg -A 5 "^pub enum Commands"
# Also check all matches of Commands:: to see the pattern
rg "Commands::" -A 2
Length of output: 2979
bin/sozo/src/commands/execute.rs (1)
26-26: Ohayo, sensei! The refactoring to use ResourceDescriptor enhances type safety and clarity
Changing tag_or_address from a String to ResourceDescriptor and updating its usage with ensure_namespace improves type safety and makes the code more maintainable.
Also applies to: 63-63
bin/sozo/src/commands/call.rs (4)
5-5: Import ResourceDescriptor correctly introduced
Ohayo sensei! The ResourceDescriptor is properly imported from sozo_ops::resource_descriptor, which will enhance the clarity and manageability of resource descriptors in the codebase.
21-21: Update of tag_or_address to ResourceDescriptor improves type safety
Ohayo sensei! Changing the type of tag_or_address to ResourceDescriptor enhances type safety and clarity in the CallArgs struct.
57-57: Correct usage of ensure_namespace on tag_or_address
Ohayo sensei! The method ensure_namespace is appropriately called on tag_or_address to ensure the default namespace is applied when necessary.
70-71: Correct handling of ResourceDescriptor variants
Ohayo sensei! The match arms for ResourceDescriptor::Address and ResourceDescriptor::Tag are correctly implemented, ensuring proper retrieval of the contract address.
bin/sozo/src/commands/model.rs (3)
5-6: Ohayo sensei! Properly importing ResourceDescriptor
The addition of ResourceDescriptor import ensures that the new type is available for use, maintaining code integrity.
25-25: Consistent use of ResourceDescriptor for tag_or_name
Sensei, updating the tag_or_name parameter to use ResourceDescriptor across all commands enhances type safety and clarity.
Also applies to: 37-37, 68-68, 80-80, 96-96
115-117: Ohayo sensei! Correctly loading workspace and profile configuration
The workspace and profile configurations are properly loaded, ensuring access to the default namespace and necessary settings.
crates/sozo/ops/src/model.rs (2)
3-5: Ohayo, sensei! Imports updated appropriately
The new imports of Primitive, Enum, Member, Struct, and Ty are necessary for the updated type handling in the code. This enhances type safety and clarity.
495-506: Ohayo, sensei! Ensure all variants of Ty are handled
In format_record_value, consider handling all possible variants of Ty to prevent unexpected behavior. If new variants are added to the Ty enum in the future, the lack of a catch-all case may lead to unhandled cases.
Run the following script to check for unhandled Ty variants:
| ResourceDescriptor::Name(_) => { | ||
| unimplemented!("Expected to be a resolved tag with default namespace.") | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Replace unimplemented!() with proper error handling
Using unimplemented!() will cause a panic at runtime if ResourceDescriptor::Name(_) is encountered. This is not ideal in production code as it can lead to unexpected crashes. Consider returning a descriptive error to handle this case gracefully.
Apply this diff to handle the error appropriately:
ResourceDescriptor::Name(_) => {
- unimplemented!("Expected to be a resolved tag with default namespace.")
+ return Err(anyhow!(
+ "Unresolved resource descriptor: expected a resolved tag with default namespace."
+ ));
}📝 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.
| ResourceDescriptor::Name(_) => { | |
| unimplemented!("Expected to be a resolved tag with default namespace.") | |
| } | |
| ResourceDescriptor::Name(_) => { | |
| return Err(anyhow!( | |
| "Unresolved resource descriptor: expected a resolved tag with default namespace." | |
| )); | |
| } |
| ResourceDescriptor::Name(_) => { | ||
| unimplemented!("Expected to be a resolved tag with default namespace.") | ||
| } |
There was a problem hiding this comment.
Handle ResourceDescriptor::Name to prevent runtime panics
Ohayo sensei! Using unimplemented! for the ResourceDescriptor::Name variant will cause a panic if encountered at runtime. It's recommended to handle this variant properly or return a descriptive error to prevent unexpected crashes.
Apply this diff to handle the variant:
ResourceDescriptor::Name(_) => {
- unimplemented!("Expected to be a resolved tag with default namespace.")
+ return Err(anyhow!(
+ "Expected a resolved tag with default namespace for descriptor: {}",
+ descriptor
+ ));
}📝 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.
| ResourceDescriptor::Name(_) => { | |
| unimplemented!("Expected to be a resolved tag with default namespace.") | |
| } | |
| ResourceDescriptor::Name(_) => { | |
| return Err(anyhow!( | |
| "Expected a resolved tag with default namespace for descriptor: {}", | |
| descriptor | |
| )); | |
| } |
| if let Layout::Fixed(lf) = layout { | ||
| println!("\n{} (packed)", name); | ||
| println!(" selector : {:#x}", get_selector_from_name(name).unwrap()); |
There was a problem hiding this comment.
Ohayo, sensei! Avoid using unwrap() to prevent potential panics
Using unwrap() on get_selector_from_name(name) may cause a panic if the function returns an error. Consider handling the error gracefully to improve the robustness of the code.
Apply this diff to handle the potential error:
- println!(" selector : {:#x}", get_selector_from_name(name).unwrap());
+ match get_selector_from_name(name) {
+ Ok(selector) => println!(" selector : {:#x}", selector),
+ Err(e) => println!(" selector : Error getting selector ({})", e),
+ }📝 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.
| if let Layout::Fixed(lf) = layout { | |
| println!("\n{} (packed)", name); | |
| println!(" selector : {:#x}", get_selector_from_name(name).unwrap()); | |
| if let Layout::Fixed(lf) = layout { | |
| println!("\n{} (packed)", name); | |
| match get_selector_from_name(name) { | |
| Ok(selector) => println!(" selector : {:#x}", selector), | |
| Err(e) => println!(" selector : Error getting selector ({})", e), | |
| } |
| fn format_field_value(member: &Member, values: &mut Vec<Felt>, level: usize) -> String { | ||
| let field_repr = format_record_value(&member.ty, values, level, false); | ||
| format!("{}{:<16}: {field_repr}", INDENT.repeat(level), member.name) | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Ensure sufficient elements in values to prevent panics
In format_field_value, removing elements from values without checking its length can cause a panic if values is empty. Consider verifying that values has enough elements before proceeding.
Apply this diff to add a check:
fn format_field_value(member: &Member, values: &mut Vec<Felt>, level: usize) -> String {
- let field_repr = format_record_value(&member.ty, values, level, false);
+ let field_repr = if values.is_empty() {
+ format!("Error: insufficient values for field {}", member.name)
+ } else {
+ format_record_value(&member.ty, values, level, false)
+ };
format!("{}{:<16}: {field_repr}", INDENT.repeat(level), member.name)
}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2618 +/- ##
==========================================
- Coverage 57.92% 57.24% -0.68%
==========================================
Files 393 396 +3
Lines 48587 49162 +575
==========================================
Hits 28144 28144
- Misses 20443 21018 +575 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
Modelcommand for inspecting models.ResourceDescriptorenum for improved handling of resource identifiers.Enhancements
tag_or_addressfields across various commands to useResourceDescriptor, enhancing type safety and clarity.Bug Fixes
Chores
Cargo.tomlfile for better integration.modelandresource_descriptorin the library structure.