feat(sozo): support arrays in calldata arguments #2917
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant enhancements to the calldata decoding system across multiple files. The changes focus on improving array handling capabilities by implementing new decoder structs for dynamic and fixed-size arrays. This modification allows for more flexible input processing, enabling the system to handle various array types with different prefixes like "arr", "u256arr", "farr", and "u256farr". The updates span several components, including the world configuration, CLI commands, and utility functions. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Decoder as Calldata Decoder
participant Contract as Contract Initialization
CLI->>Decoder: Pass calldata as Vec<String>
Decoder->>Decoder: Determine array type
Decoder->>Decoder: Decode array elements
Decoder->>Contract: Provide decoded calldata
Possibly related PRs
Suggested Labels
Ohayo, sensei! The changes look robust and well-structured! The new approach provides much more flexibility in handling complex calldata scenarios. Happy coding! 🚀🍱 🪧 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: 3
🧹 Nitpick comments (2)
crates/dojo/world/src/config/calldata_decoder.rs (2)
141-202: Ensure consistent error handling in array decodersOhayo, sensei! The array decoders currently do not handle potential parsing errors within the loop, which might occur if an individual item fails to decode.
Consider collecting errors from
DefaultCalldataDecoder.decode(item)?and returning a comprehensive error message if any item fails to decode.Would you like assistance in enhancing the error handling?
402-456: Expand test coverage for edge cases and errorsOhayo, sensei! The test suite comprehensively covers successful decoding scenarios but lacks tests for invalid inputs and error conditions. Including such tests ensures the robustness of the decoders.
Would you like me to help by adding tests for invalid prefixes, malformed inputs, and empty arrays?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/dojo/world/Cargo.toml(0 hunks)crates/dojo/world/src/config/calldata_decoder.rs(9 hunks)
💤 Files with no reviewable changes (1)
- crates/dojo/world/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
| fn split_unquoted(input: &str, delimiter: char) -> Vec<&str> { | ||
| let mut quoted = false; | ||
|
|
||
| input | ||
| .split(|c| { | ||
| if [STRING_DELIMITER, SHORT_STRING_DELIMITER].contains(&c) { | ||
| quoted = !quoted; | ||
| } | ||
| c == delimiter && !quoted | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve split_unquoted to handle escaped quotes
Ohayo, sensei! The split_unquoted function may not correctly handle cases where quotes are escaped within the input string. This could lead to incorrect splitting of the calldata, causing parsing errors.
Consider enhancing the function to account for escaped quotes. Do you want me to assist in implementing this improvement?
| @@ -157,12 +258,16 @@ | |||
| pub fn decode_single_calldata(item: &str) -> DecoderResult<Vec<Felt>> { | |||
| let item = item.trim(); | |||
|
|
|||
| println!("inner item: '{item}'"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug println! statement
Ohayo, sensei! The println! statement in decode_single_calldata is likely used for debugging purposes and could lead to unnecessary console output in production.
Apply this diff to remove the debug statement:
- println!("inner item: '{item}'");| fn remove_first_and_last_characters(value: &str) -> &str { | ||
| let mut chars = value.chars(); | ||
| chars.next(); | ||
| chars.next_back(); | ||
| chars.as_str() | ||
| } |
There was a problem hiding this comment.
Handle edge cases in remove_first_and_last_characters for short strings
Ohayo, sensei! The function remove_first_and_last_characters may panic or return unexpected results when the input string is shorter than two characters. This could occur if an empty string or a single-character string is provided.
To ensure robustness, consider adding a check to handle such cases gracefully.
Apply this diff to handle short strings:
fn remove_first_and_last_characters(value: &str) -> &str {
+ if value.len() < 2 {
+ return "";
+ }
let mut chars = value.chars();
chars.next();
chars.next_back();
chars.as_str()
}📝 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.
| fn remove_first_and_last_characters(value: &str) -> &str { | |
| let mut chars = value.chars(); | |
| chars.next(); | |
| chars.next_back(); | |
| chars.as_str() | |
| } | |
| fn remove_first_and_last_characters(value: &str) -> &str { | |
| if value.len() < 2 { | |
| return ""; | |
| } | |
| let mut chars = value.chars(); | |
| chars.next(); | |
| chars.next_back(); | |
| chars.as_str() | |
| } |
remybar
force-pushed
the
sozo-support_array_for_calldata
branch
2 times, most recently
from
8174c34 to
b665340
Compare
remybar
force-pushed
the
sozo-support_array_for_calldata
branch
from
b665340 to
c406534
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
crates/dojo/world/src/config/calldata_decoder.rs (4)
108-122: Consider consolidating array decoders to reduce code duplicationOhayo, sensei! The implementations of
DynamicArrayCalldataDecoder,U256DynamicArrayCalldataDecoder,FixedSizeArrayCalldataDecoder, andU256FixedSizeArrayCalldataDecodershare similar logic. To enhance maintainability and reduce code duplication, consider creating a generic array decoder that can handle different item decoders and array types.Apply this diff to refactor the array decoders:
+struct ArrayCalldataDecoder<D: CalldataDecoder> { + decoder: D, + is_dynamic: bool, +} + +impl<D: CalldataDecoder> CalldataDecoder for ArrayCalldataDecoder<D> { + fn decode(&self, input: &str) -> DecoderResult<Vec<Felt>> { + let items = input.split(ARRAY_ITEM_DELIMITER).collect::<Vec<_>>(); + let mut decoded_items = if self.is_dynamic { + vec![items.len().into()] + } else { + vec![] + }; + + for item in items { + decoded_items.extend(self.decoder.decode(item)?); + } + + Ok(decoded_items) + } +}Then replace the individual decoders with instances of
ArrayCalldataDecoderusing the appropriate item decoder.Also applies to: 124-137, 139-153, 155-168
184-200: Update documentation fordecode_calldatato reflect new input formatOhayo, sensei! The
decode_calldatafunction now accepts aVec<String>instead of a single&str. Please update the function documentation and examples to reflect this change for clarity.Apply this diff to update the documentation:
/// Decodes a vector of calldata items into a vector of Felts. /// /// # Arguments: /// -/// * `input` - The input vector to decode. Inputs can have prefixes to indicate the type of the +/// * `input` - The vector of input strings to decode. Each input can have prefixes to indicate the type of the /// item. /// /// # Returns /// A vector of [`Felt`]s. /// /// # Example /// /// ``` -/// let input = ["u256:0x1", "str:hello world", "64"]; +/// let input = vec!["u256:0x1".to_string(), "str:hello world".to_string(), "64".to_string()]; /// let result = decode_calldata(&input).unwrap(); /// ```Also applies to: 203-207
246-249: Avoid macro name collision by renamingvec_of_strings!Ohayo, sensei! The macro
vec_of_strings!might conflict with other macros. Consider renaming it to a less generic name, such ascalldata_vec!, to prevent potential name collisions.Apply this diff to rename the macro:
-macro_rules! vec_of_strings { +macro_rules! calldata_vec { ($($x:expr),*) => (vec![$($x.to_string()),*]); }
365-418: Ensure comprehensive test coverage for array decodersOhayo, sensei! The new tests for array decoders are great. However, consider adding tests for edge cases, such as empty arrays and arrays with invalid items, to ensure robustness.
Would you like assistance in writing these additional tests?
bin/sozo/src/commands/call.rs (2)
28-31: Clarify the help message forcalldataargumentOhayo, sensei! The help message for the
calldataargument can be made clearer by removing redundant formatting directives.Apply this diff to simplify the help message:
#[arg(num_args = 0..)] -#[arg(help = format!("The calldata to be passed to the system. -{CALLDATA_DOC}"))] +#[arg(help = concat!("The calldata to be passed to the system.\n", CALLDATA_DOC))] pub calldata: Vec<String>,
Line range hint
62-87: Optimize contract address resolution logicOhayo, sensei! The logic for resolving
contract_addresscan be streamlined for readability and efficiency. Consider refactoring the match block to reduce nesting and improve clarity.Apply this diff to refactor the code:
-let contract_address = match &descriptor { - ResourceDescriptor::Address(address) => Some(*address), - ResourceDescriptor::Tag(tag) => { - let contracts: HashMap<String, ContractInfo> = if self.diff || local_manifest.is_none() { - let (world_diff, _, _) = utils::get_world_diff_and_provider( - self.starknet.clone(), - self.world, - &ws, - ).await?; - (&world_diff).into() - } else { - (&local_manifest.unwrap()).into() - }; - contracts.get(tag).map(|c| c.address) - } - ResourceDescriptor::Name(_) => { - return Err(anyhow!("Contract name '{}' could not be resolved to an address.", descriptor)); - } -}.ok_or_else(|| anyhow!("Contract {descriptor} not found in the world diff."))?; +let contract_address = self.resolve_contract_address(&descriptor, local_manifest).await?;Then, implement the
resolve_contract_addressmethod to encapsulate the logic.crates/sozo/ops/src/migrate/mod.rs (2)
274-274: Ensure proper error handling when decoding calldataOhayo, sensei! The error handling for
decode_calldata(args)ininitialize_contractslacks context. Consider providing more detailed error messages to help diagnose issues during migration.Apply this diff to improve error handling:
let args = if let Some(args) = init_call_args { - decode_calldata(args).map_err(|_| MigrationError::InitCallArgs)? + decode_calldata(args).map_err(|e| MigrationError::InitCallArgsWithContext(e.to_string()))? } else { vec![] };And update the
MigrationErrorenum to include the new variant with context.
Line range hint
481-487: Avoid cloning classes unnecessarilyOhayo, sensei! In the
sync_resourcesmethod, the note mentions cloning classes, which is not ideal. Consider refactoring to avoid cloning by restructuring how class information is extracted and stored.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bin/sozo/src/commands/call.rs(3 hunks)bin/sozo/src/commands/execute.rs(2 hunks)bin/sozo/src/utils.rs(1 hunks)crates/dojo/world/src/config/calldata_decoder.rs(7 hunks)crates/sozo/ops/src/migrate/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- bin/sozo/src/utils.rs
- bin/sozo/src/commands/execute.rs
| "arr" => DynamicArrayCalldataDecoder.decode(value)?, | ||
| "u256arr" => U256DynamicArrayCalldataDecoder.decode(value)?, | ||
| "farr" => FixedSizeArrayCalldataDecoder.decode(value)?, | ||
| "u256farr" => U256FixedSizeArrayCalldataDecoder.decode(value)?, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle unknown prefixes in decode_single_calldata
Ohayo, sensei! The decode_single_calldata function falls back to DefaultCalldataDecoder when an unknown prefix is encountered. To prevent unexpected behavior, consider returning an error for unknown prefixes to alert the user.
Apply this diff to handle unknown prefixes:
match prefix {
"u256" => U256CalldataDecoder.decode(value)?,
"str" => StrCalldataDecoder.decode(value)?,
"sstr" => ShortStrCalldataDecoder.decode(value)?,
"int" => SignedIntegerCalldataDecoder.decode(value)?,
"arr" => DynamicArrayCalldataDecoder.decode(value)?,
"u256arr" => U256DynamicArrayCalldataDecoder.decode(value)?,
"farr" => FixedSizeArrayCalldataDecoder.decode(value)?,
"u256farr" => U256FixedSizeArrayCalldataDecoder.decode(value)?,
- _ => DefaultCalldataDecoder.decode(item)?,
+ _ => return Err(CalldataDecoderError::ParseError(format!("Unknown prefix: {}", prefix))),
}Committable suggestion skipped: line range outside the PR's diff.
| let calldata = calldata_decoder::decode_calldata(&self.calldata)?; | ||
|
|
||
| let contract_address = match &descriptor { |
There was a problem hiding this comment.
Handle missing contract address when descriptor is a name
Ohayo, sensei! In the match statement for contract_address, the ResourceDescriptor::Name(_) arm is unimplemented. To improve the user experience, consider returning a meaningful error message or implementing this case.
Apply this diff to handle the case:
ResourceDescriptor::Name(name) => {
- unimplemented!("Expected to be a resolved tag with default namespace.")
+ return Err(anyhow!("Contract name '{}' could not be resolved to an address.", name));
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/world/src/config/calldata_decoder.rs (2)
114-123: Consider adding validation for empty arrays.Ohayo, sensei! The array decoders should validate that the input array is not empty to prevent potential issues with zero-length arrays.
Consider adding validation like this:
fn decode(&self, input: &str) -> DecoderResult<Vec<Felt>> { let items = input.split(ARRAY_ITEM_DELIMITER).collect::<Vec<_>>(); + if items.is_empty() { + return Err(CalldataDecoderError::ParseError("Empty array is not allowed".to_string())); + } let mut decoded_items: Vec<Felt> = vec![items.len().into()];Also applies to: 144-154
368-393: Consider adding test cases for invalid array items.Ohayo, sensei! While the array decoder tests are comprehensive for valid inputs, consider adding test cases for arrays containing invalid items.
Here's a suggested test case:
#[test] fn test_array_with_invalid_items() { let input = vec_of_strings!["arr:1,invalid,3"]; let result = decode_calldata(&input); assert!(result.is_err()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/world/src/config/calldata_decoder.rs(7 hunks)
🔇 Additional comments (6)
crates/dojo/world/src/config/calldata_decoder.rs (6)
22-23: LGTM! Well-structured error handling.The new
UnknownPrefixerror variant is well-documented and provides clear error messages for invalid calldata prefixes.
28-28: LGTM! Clear constant naming.The
ARRAY_ITEM_DELIMITERconstant clearly indicates its purpose for array item separation.
110-170: LGTM! Well-structured array decoder implementations.The array decoder implementations follow a consistent pattern and correctly handle both dynamic and fixed-size arrays for regular and U256 types.
190-202: LGTM! Well-documented function signature update.The updated
decode_calldatafunction signature and documentation clearly reflect the new array support capabilities.
229-233: LGTM! Improved prefix handling.The implementation now correctly handles array prefixes and returns an error for unknown prefixes, addressing the feedback from a previous review.
248-250: LGTM! Helpful test macro.The
vec_of_strings!macro improves test readability and reduces boilerplate code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2917 +/- ##
==========================================
+ Coverage 55.83% 55.91% +0.07%
==========================================
Files 443 443
Lines 57098 57190 +92
==========================================
+ Hits 31879 31975 +96
+ Misses 25219 25215 -4 ☔ View full report in Codecov by Sentry. |
Description
Add support of dynamic/fixed-size arrays in calldata arguments.
To do that, calldata argument format has changed a little bit:
sstr:'hello world'instead ofsstr:hello world),.Then, arrays are defined with a specific prefix (see below), and their items separated by commas.
arrand each item must fit on one felt (example:arr:1,2,3),farrand each item must fit on one felt (example:farr:1,2,3),u256arr(example:u256arr:1,2,3),u256farr(example:u256farr:1,2,3).Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements