feat(sozo): support multicall for execute command #2897
Conversation
WalkthroughOhayo, sensei! The pull request introduces significant modifications to 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
🧹 Nitpick comments (1)
bin/sozo/src/commands/execute.rs (1)
190-215: Ohayo, sensei! Consider adding more test cases forCallArguments::from_strThe current tests cover several cases, but you might want to add tests for invalid calldata formats to ensure robust parsing and error handling.
Here's an example of an additional test case:
// invalid calldata format let res = CallArguments::from_str("0x1234,run,invalid_calldata"); assert!(res.is_err());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/sozo/src/commands/execute.rs(5 hunks)crates/sozo/ops/src/resource_descriptor.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: clippy
🔇 Additional comments (1)
crates/sozo/ops/src/resource_descriptor.rs (1)
10-10: Ohayo, sensei! Nice addition ofPartialEqderive forResourceDescriptorDeriving
PartialEqallows for equality comparisons, which is useful in testing and other logical operations.
| if self.diff { | ||
| message.push_str( | ||
| " Run the command again with `--diff` to force the fetch of data from \ | ||
| the chain.", | ||
| ); | ||
| } | ||
| anyhow!(message) | ||
| })?; |
There was a problem hiding this comment.
Ohayo, sensei! Fix the error message condition to display the suggestion correctly
Currently, the error message suggests running the command again with --diff when self.diff is true. However, if self.diff is already true, the suggestion is redundant. The condition should be inverted to check when self.diff is false.
Apply this diff to fix the condition:
- if self.diff {
+ if !self.diff {📝 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 self.diff { | |
| message.push_str( | |
| " Run the command again with `--diff` to force the fetch of data from \ | |
| the chain.", | |
| ); | |
| } | |
| anyhow!(message) | |
| })?; | |
| if !self.diff { | |
| message.push_str( | |
| " Run the command again with `--diff` to force the fetch of data from \ | |
| the chain.", | |
| ); | |
| } | |
| anyhow!(message) | |
| })?; |
| let contract_address = match &descriptor { | ||
| ResourceDescriptor::Address(address) => Some(*address), | ||
| ResourceDescriptor::Tag(tag) => contracts.get(tag).map(|c| c.address), | ||
| ResourceDescriptor::Name(_) => { | ||
| unimplemented!("Expected to be a resolved tag with default namespace.") | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Handle ResourceDescriptor::Name variant to prevent runtime panics
Using unimplemented!() in the match arm for ResourceDescriptor::Name(_) will cause a runtime panic if this case is encountered. Consider implementing proper error handling for this variant to prevent unexpected panics.
Apply this diff to handle the Name variant gracefully:
ResourceDescriptor::Name(_) => {
- unimplemented!("Expected to be a resolved tag with default namespace.")
+ return Err(anyhow!("Contract address could not be resolved for descriptor: {descriptor}"));
}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2897 +/- ##
==========================================
- Coverage 55.85% 55.20% -0.65%
==========================================
Files 449 449
Lines 57730 58350 +620
==========================================
- Hits 32245 32214 -31
- Misses 25485 26136 +651 ☔ View full report in Codecov by Sentry. |
glihm
changed the title
bin/sozo/src/commands/execute.rs
Outdated
| pub tag_or_address: ResourceDescriptor, | ||
| #[arg(num_args = 1..)] | ||
| #[arg(help = "A list of calls to execute.\n | ||
| A call is made up of 3 values, separated by a comma (<TAG_OR_ADDRESS>,<ENTRYPOINT>[,<CALLDATA>]): |
There was a problem hiding this comment.
May be more natural to have the calldata separated from the two first arguments.
Starkli is doing that: https://github.com/xJonathanLEI/starkli/blob/4c92ac0ee3c8a50289b09760ec535cf27dd64b2c/src/subcommands/invoke.rs#L62
Wondering what could be best to have something easy to use, wdyt on that?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bin/sozo/src/commands/execute.rs (2)
115-117:⚠️ Potential issueHandle ResourceDescriptor::Name variant to prevent runtime panics.
Ohayo, sensei! Using
unimplemented!()could cause runtime panics. We should handle this case gracefully.ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") + return Err(anyhow!("Contract address could not be resolved for descriptor: {descriptor}")); }
122-129:⚠️ Potential issueFix the error message condition to display the suggestion correctly.
Ohayo, sensei! The suggestion to use
--diffshould only be shown when it's not already being used.- if self.diff { + if !self.diff {
🧹 Nitpick comments (2)
bin/sozo/src/commands/execute.rs (2)
137-137: Define call separator characters as constants.Ohayo, sensei! The separator characters should be defined as constants at the module level for better maintainability.
+const CALL_SEPARATORS: [&str; 3] = ["/", "-", "\\"]; // Then in the code: - "/" | "-" | "\\" => break, + s if CALL_SEPARATORS.contains(&s) => break,
131-132: Improve error message for missing entrypoint.Ohayo, sensei! The error message could be more descriptive about what argument is missing.
- arg_iter.next().ok_or_else(|| anyhow!("Unexpected number of arguments"))?; + arg_iter.next().ok_or_else(|| anyhow!("Missing entrypoint for contract {descriptor}"))?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/sozo/src/commands/execute.rs(2 hunks)bin/sozo/src/commands/mod.rs(1 hunks)crates/dojo/world/src/config/calldata_decoder.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (5)
crates/dojo/world/src/config/calldata_decoder.rs (1)
143-143: LGTM! Clean refactoring of the decoder function.The renaming from
decode_innertodecode_single_calldataand making it public improves code clarity and reusability. The function's implementation remains solid with comprehensive test coverage.Also applies to: 157-157
bin/sozo/src/commands/mod.rs (1)
49-52: LGTM! Documentation updates accurately reflect the new multicall capability.The command descriptions have been updated to clearly indicate that the Execute command now supports multiple system calls.
Also applies to: 54-54
bin/sozo/src/commands/execute.rs (3)
23-24: Add the required constraint before num_args for proper validation.Ohayo, sensei! Based on the past review comments, the argument constraints need to be reordered for proper application.
- #[arg(num_args = 1..)] - #[arg(required = true)] + #[arg(required = true)] + #[arg(num_args = 1..)]
90-97: LGTM! Clean implementation of contract resolution.The contract resolution logic properly handles both manifest-based and diff-based approaches.
150-154: LGTM! Proper multicall implementation.The implementation correctly builds and executes multiple calls in a single transaction.
remybar
force-pushed
the
sozo-multicall_support
branch
from
5ee7a30 to
09b00d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bin/sozo/src/commands/execute.rs (2)
115-118:⚠️ Potential issueOhayo, sensei! Replace unimplemented!() with proper error handling
Using
unimplemented!()for theResourceDescriptor::Namevariant could cause runtime panics.Apply this diff to handle the Name variant gracefully:
ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") + return Err(anyhow!("Contract address could not be resolved for descriptor: {descriptor}")); }
122-129:⚠️ Potential issueOhayo, sensei! Fix the error message condition
The error message suggests running with
--diffwhen it's already enabled.Apply this diff to fix the condition:
- if self.diff { + if !self.diff {
🧹 Nitpick comments (2)
bin/sozo/src/commands/execute.rs (2)
157-157: Ohayo, sensei! Consider adding transaction simulationThe multicall transaction could benefit from simulation before execution to catch potential issues early.
Consider adding a simulation step before the actual multicall:
+ // Simulate the transaction first + let simulation = invoker.simulate_multicall().await?; + trace!(simulation=?simulation, "Multicall simulation successful"); + let tx_result = invoker.multicall().await?;
104-155: Ohayo, sensei! The multicall implementation is well-structuredThe implementation efficiently batches multiple calls into a single transaction, which is a significant improvement for gas efficiency and atomic execution. The parsing logic for calls and calldata is robust and handles edge cases well.
Consider documenting the following characteristics in the codebase:
- Gas savings from batching multiple calls
- Atomicity guarantees (all calls succeed or all fail)
- Maximum recommended number of calls per multicall
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/sozo/src/commands/execute.rs(2 hunks)bin/sozo/src/commands/mod.rs(1 hunks)crates/dojo/world/src/config/calldata_decoder.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/sozo/src/commands/mod.rs
- crates/dojo/world/src/config/calldata_decoder.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: clippy
🔇 Additional comments (1)
bin/sozo/src/commands/execute.rs (1)
21-51: Ohayo, sensei! The command structure is well-documented and properly implemented.The help text is comprehensive, providing clear examples and detailed explanations of the call format and supported type prefixes. The implementation correctly follows the previous feedback about argument constraints.
| let mut arg_iter = self.calls.into_iter(); | ||
|
|
||
| while let Some(arg) = arg_iter.next() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Add validation for minimum number of calls
The iterator implementation should validate that at least one complete call (address + entrypoint) is provided.
Add this validation before the while loop:
let mut arg_iter = self.calls.into_iter();
+ if self.calls.len() < 2 {
+ return Err(anyhow!("At least one complete call (address and entrypoint) is required"));
+ }
while let Some(arg) = arg_iter.next() {📝 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.
| let mut arg_iter = self.calls.into_iter(); | |
| while let Some(arg) = arg_iter.next() { | |
| let mut arg_iter = self.calls.into_iter(); | |
| if self.calls.len() < 2 { | |
| return Err(anyhow!("At least one complete call (address and entrypoint) is required")); | |
| } | |
| while let Some(arg) = arg_iter.next() { |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bin/sozo/src/commands/execute.rs (2)
21-51: Ohayo, sensei! Consider enhancing the documentation with more comprehensive examplesWhile the documentation is thorough, consider adding examples that demonstrate:
- All supported prefixes (u256, sstr, str, int)
- Multiple calls with different calldata types
- The maximum number of calls supported in a single transaction
Add an expanded example section:
EXAMPLE sozo execute 0x1234 run / ns-Actions move 1 2 + + # Multiple calls with different data types + sozo execute \ + 0x1234 set_value u256:1000 / \ + ns-Actions move int:42 sstr:player / \ + ns-Config update_name str:ByteArrayValue
155-160: Ohayo, sensei! Consider adding debug assertions for call validationAdd debug assertions to validate each call's parameters before adding to the multicall.
+ debug_assert!(!calldata.is_empty(), "Calldata should not be empty"); + debug_assert!( + contract_address != starknet::core::types::FieldElement::ZERO, + "Contract address should not be zero" + ); invoker.add_call(Call { to: contract_address, selector: snutils::get_selector_from_name(&entrypoint)?, calldata, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/execute.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
bin/sozo/src/commands/execute.rs (3)
123-130: Ohayo, sensei! Fix the error message conditionThe error message suggests running with
--diffwhen it's already true.Apply this diff to fix the condition:
- if self.diff { + if !self.diff { message.push_str( " Run the command again with `--diff` to force the fetch of data from \ the chain.", ); }
116-119: Ohayo, sensei! Handle ResourceDescriptor::Name variant properlyUsing
unimplemented!()will cause runtime panics. Handle the Name variant gracefully:ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") + return Err(anyhow!("Contract address could not be resolved for descriptor: {descriptor}")); }
106-107:⚠️ Potential issueOhayo, sensei! Add validation for the number of calls
The iterator setup should validate both minimum and maximum number of calls to prevent:
- Empty or incomplete calls
- Exceeding Starknet's transaction size limits
Add validation before the iterator setup:
+ const MAX_CALLS: usize = 100; // Adjust based on Starknet's limits + if self.calls.len() < 2 { + return Err(anyhow!("At least one complete call (address and entrypoint) is required")); + } + if self.calls.len() > MAX_CALLS * 3 { + return Err(anyhow!("Number of calls exceeds maximum limit")); + } + let mut arg_iter = self.calls.into_iter();Likely invalid or redundant comment.
| let mut calldata = vec![]; | ||
| for arg in &mut arg_iter { | ||
| let arg = match arg.as_str() { | ||
| "/" | "-" | "\\" => break, | ||
| _ => calldata_decoder::decode_single_calldata(&arg)?, | ||
| }; | ||
| calldata.extend(arg); | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Add safety check for calldata parsing
The calldata parsing loop could potentially continue indefinitely if a separator is missing. Additionally, there's no validation of calldata size.
Add safety checks:
let mut calldata = vec![];
+ let mut calldata_count = 0;
+ const MAX_CALLDATA_SIZE: usize = 1000; // Adjust based on requirements
for arg in &mut arg_iter {
+ calldata_count += 1;
+ if calldata_count > MAX_CALLDATA_SIZE {
+ return Err(anyhow!("Calldata size exceeds maximum limit"));
+ }
let arg = match arg.as_str() {
"/" | "-" | "\\" => break,
_ => calldata_decoder::decode_single_calldata(&arg)?,
};
calldata.extend(arg);
}📝 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.
| let mut calldata = vec![]; | |
| for arg in &mut arg_iter { | |
| let arg = match arg.as_str() { | |
| "/" | "-" | "\\" => break, | |
| _ => calldata_decoder::decode_single_calldata(&arg)?, | |
| }; | |
| calldata.extend(arg); | |
| } | |
| let mut calldata = vec![]; | |
| let mut calldata_count = 0; | |
| const MAX_CALLDATA_SIZE: usize = 1000; // Adjust based on requirements | |
| for arg in &mut arg_iter { | |
| calldata_count += 1; | |
| if calldata_count > MAX_CALLDATA_SIZE { | |
| return Err(anyhow!("Calldata size exceeds maximum limit")); | |
| } | |
| let arg = match arg.as_str() { | |
| "/" | "-" | "\\" => break, | |
| _ => calldata_decoder::decode_single_calldata(&arg)?, | |
| }; | |
| calldata.extend(arg); | |
| } |
Related issue
#2388
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
Summary by CodeRabbit
New Features
Improvements
Technical Enhancements
Executecommand to clarify its functionality.