feat(sozo): add mutli-call support for execute command in sozo #2620
Conversation
Manush-2005
changed the title
WalkthroughOhayo, sensei! This pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (2)
bin/sozo/src/commands/execute.rs (2)
109-115: Improve logging for each executed callOhayo, sensei! The trace logs inside the loop provide valuable information for debugging. To enhance clarity, consider including the call index or total number of calls being executed. This makes it easier to identify issues with specific calls in a batch.
You can modify the trace statement as follows:
trace!( + call_index = calls.iter().position(|x| x == &call_args).unwrap_or(0) + 1, + total_calls = calls.len(), contract = ?descriptor, entrypoint = call_args.entrypoint, calldata = ?call_args.calldata, "Executing Execute command." );
127-130: Handle errors for individual calls without halting executionOhayo, sensei! Currently, if an error occurs during the execution of a call, the entire process stops. To enhance robustness, consider handling errors for each call individually so that one failure doesn't prevent subsequent calls from executing.
You might adjust the code like this:
for call_args in self.calls { // ... existing logic ... let tx_result = invoker.invoke(call).await; match tx_result { Ok(result) => println!("{}", result), Err(e) => eprintln!("Error executing call {}: {}", call_args.entrypoint, e), } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/sozo/src/commands/execute.rs(2 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/execute.rs (1)
94-130: Update loop to iterate over CallArgs
Ohayo, sensei! After changing calls to Vec<CallArgs>, the loop in your run method should correctly handle each CallArgs item. This ensures that call_args has the necessary fields for execution.
bin/sozo/src/commands/execute.rs
Outdated
| help = "The calls to be executed. Each call should include the address or tag, entrypoint, and calldata." | ||
| )] | ||
|
|
||
| pub calls: Vec<String>, |
There was a problem hiding this comment.
Change calls field to Vec<CallArgs>
Ohayo, sensei! The calls field is defined as Vec<String>, but within your loop, each call_args is used as if it's a CallArgs struct with fields like tag_or_address, entrypoint, and calldata. To fix this mismatch, consider changing the type of calls to Vec<CallArgs>.
Apply this diff to update the field:
pub struct ExecuteArgs {
#[arg(
help = "The calls to be executed. Each call should include the address or tag, entrypoint, and calldata."
)]
- pub calls: Vec<String>,
+ pub calls: Vec<CallArgs>,
#[command(flatten)]
pub starknet: StarknetOptions,
#[command(flatten)]
pub account: AccountOptions,
#[command(flatten)]
pub world: WorldOptions,
#[command(flatten)]
pub transaction: TransactionOptions,
}Additionally, you'll need to update the command-line argument parsing to handle CallArgs properly.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
bin/sozo/src/commands/execute.rs (2)
43-44: Consider adding input validation for CallArgs.Ohayo, sensei! While the CallArgs struct is well-designed, consider adding validation to ensure:
- Maximum length limits for calldata
- Valid entrypoint name format
- Reasonable restrictions on tag_or_address length
Example validation implementation:
impl CallArgs { pub fn validate(&self) -> Result<()> { if self.entrypoint.is_empty() { return Err(anyhow!("Entrypoint cannot be empty")); } if let Some(ref cd) = self.calldata { if cd.len() > 1024 { // example limit return Err(anyhow!("Calldata too long")); } } Ok(()) } }
95-131: Consider enhancing error handling and performance.Ohayo, sensei! The implementation looks good but could benefit from these improvements:
- Batch error handling:
- Currently, the first error stops all subsequent calls
- Consider collecting errors and continuing execution
- Performance optimization:
- Consider parallel execution for independent calls
- Cache contract addresses for repeated calls to the same contract
Here's a suggested approach for error collection:
let mut results = Vec::new(); for call_args in self.calls { match process_single_call(call_args).await { Ok(result) => { println!("{}", result); results.push(Ok(result)); } Err(e) => { eprintln!("Error processing call: {}", e); results.push(Err(e)); } } } if results.iter().any(|r| r.is_err()) { return Err(anyhow!("Some calls failed")); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/sozo/src/commands/execute.rs(2 hunks)
🔇 Additional comments (2)
bin/sozo/src/commands/execute.rs (2)
23-41: LGTM! Clean implementation of multi-call support.
Ohayo, sensei! The ExecuteArgs structure has been properly updated to support multiple calls using Vec. The help text is clear and informative, and the command options are properly flattened.
98-108: Add safeguards against recursive or circular calls.
Ohayo, sensei! The contract address resolution looks good, but consider adding protection against:
- Recursive calls to the same contract
- Circular dependencies between contracts
- Maximum call depth
This could prevent potential DoS vectors or unintended behavior.
Let's check for potential recursive calls in the codebase:
bin/sozo/src/commands/execute.rs
Outdated
| #[derive(Debug, Args)] | ||
| pub struct CallArgs { |
There was a problem hiding this comment.
Here you'll have an issue since you don't have any way to parse those data from the CLI.
You will need to find a way to define those multi calls in a parseable manner in order to have clap parsing them.
Accepting a Vec<String> and then parsing the calls is a good way to go. Or if you have a CallArgs type, then this one must be parseable from String.
There was a problem hiding this comment.
@glihm , in the latest commit I have kept Vec<String> as you have suggested, and then I am converting this string into CallArgs type for further processing.
There was a problem hiding this comment.
Also, let me know if you have any problems with current implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
bin/sozo/src/commands/execute.rs (3)
43-63: Remove commented code but preserve documentation.Ohayo, sensei! The commented
CallArgsstruct should be removed since we have a new implementation. However, the detailed documentation for the fields, especially the calldata format and supported prefixes, is valuable and should be moved to the new implementation.
111-152: Consider implementing batch transactions and improving error handling.Ohayo, sensei! The current implementation processes calls sequentially, which has several limitations:
- No batch transaction support (all calls are separate transactions)
- No strategy for handling partial failures (what happens if call Implement World contract #5 fails in a sequence of 10?)
- No progress indication for multiple calls
- Redundant world_diff lookups for each call
Consider these improvements:
- Add support for batch transactions where applicable
- Implement a failure strategy (continue/abort on failure)
- Add progress reporting for multiple calls
- Cache world_diff lookups
Would you like assistance in implementing any of these improvements?
149-152: Enhance transaction result output for multiple calls.Ohayo, sensei! When executing multiple calls, a simple println of the transaction result might not be sufficient. Consider adding more context to help users track multiple transactions.
- println!("{}", tx_result); + println!("Call {}/{} - {} -> {}: {}", + index + 1, + call_args_list.len(), + descriptor, + call_args.entrypoint, + tx_result);
bin/sozo/src/commands/execute.rs
Outdated
| fn from_string(s: &str) -> Result<Self> { | ||
| let parts: Vec<&str> = s.split(',').collect(); | ||
| if parts.len() < 2 { | ||
| return Err(anyhow!("Invalid call format")); | ||
| } | ||
|
|
||
| Ok(CallArgs { | ||
| tag_or_address: parts[0].parse()?, | ||
| entrypoint: parts[1].to_string(), | ||
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and validation in from_string parsing.
Ohayo, sensei! The current parsing implementation could be more robust. Consider these improvements:
- Add proper validation for empty strings and whitespace
- Provide specific error messages for different failure cases
- Validate the entrypoint name format
Here's a suggested implementation:
fn from_string(s: &str) -> Result<Self> {
+ let s = s.trim();
+ if s.is_empty() {
+ return Err(anyhow!("Empty call string"));
+ }
let parts: Vec<&str> = s.split(',').collect();
if parts.len() < 2 {
- return Err(anyhow!("Invalid call format"));
+ return Err(anyhow!("Invalid call format. Expected format: <address/tag>,<entrypoint>[,<calldata>]"));
}
+ let entrypoint = parts[1].trim();
+ if entrypoint.is_empty() {
+ return Err(anyhow!("Empty entrypoint"));
+ }
Ok(CallArgs {
tag_or_address: parts[0].parse()?,
- entrypoint: parts[1].to_string(),
+ entrypoint: entrypoint.to_string(),
calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None },
})📝 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 from_string(s: &str) -> Result<Self> { | |
| let parts: Vec<&str> = s.split(',').collect(); | |
| if parts.len() < 2 { | |
| return Err(anyhow!("Invalid call format")); | |
| } | |
| Ok(CallArgs { | |
| tag_or_address: parts[0].parse()?, | |
| entrypoint: parts[1].to_string(), | |
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | |
| }) | |
| } | |
| fn from_string(s: &str) -> Result<Self> { | |
| let s = s.trim(); | |
| if s.is_empty() { | |
| return Err(anyhow!("Empty call string")); | |
| } | |
| let parts: Vec<&str> = s.split(',').collect(); | |
| if parts.len() < 2 { | |
| return Err(anyhow!("Invalid call format. Expected format: <address/tag>,<entrypoint>[,<calldata>]")); | |
| } | |
| let entrypoint = parts[1].trim(); | |
| if entrypoint.is_empty() { | |
| return Err(anyhow!("Empty entrypoint")); | |
| } | |
| Ok(CallArgs { | |
| tag_or_address: parts[0].parse()?, | |
| entrypoint: entrypoint.to_string(), | |
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | |
| }) | |
| } |
There was a problem hiding this comment.
Let me know if you have any trouble and we can check it on a call on the discord server of Dojo.
But from your latest changes, now the idea is how you will decompose the call.
A call is having:
- A target contract (can be an address or a tag).
- The entry point to execute (name).
- Finally, the call data.
So you have to find an elegant way in which the multi call can be done.
Currently, this is the sozo execute syntax:
sozo execute <CONTRACT_TAG> <ENTRYPOINT_NAME> -c ARG1,ARG2,...If we want to multi call that, we will not be able to use -c for the calldata.
We could think about something like:
sozo execute <CONTRACT_NAME>,<ENTRYPOINT_NAME>,ARG1,ARG2 <CONTRACT_NAME2>,<ENTRYPOINT_NAME2>,<NOARGS>Do you see the idea? If you have better suggestions for separating values etc... go ahead.
glihm
changed the title
|
Yeah @glihm , I see the idea. I think it's better to split different calls using commas like you suggested. |
|
I have made the nesscary changes.Please let me know , @glihm. Today, I be joining the Discord as you have suggested. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
bin/sozo/src/commands/execute.rs (3)
23-28: Enhance help text with format specificationOhayo, sensei! The help text could be more specific about the expected format of each call string.
#[arg( - help = "The calls to be executed. Each call should include the address or tag, entrypoint, and calldata." + help = "The calls to be executed. Format for each call: <address/tag>,<entrypoint>[,<calldata>]. Multiple calls are space-separated." )]
133-174: Consider enhancing multi-call execution feedbackOhayo, sensei! While the implementation works, consider these improvements:
- Add progress indication for multiple calls
- Provide clear success/failure status for each call
- Consider implementing batch transaction support
let call_args_list: Vec<CallArgs> = self.calls.iter() .map(|s| CallArgs::from_string(s)) .collect::<Result<Vec<_>>>()?; + println!("Executing {} calls...", call_args_list.len()); + let mut results = Vec::new(); for call_args in call_args_list { + print!("Executing call to {}...", call_args.tag_or_address); let descriptor = call_args.tag_or_address.ensure_namespace(&profile_config.namespace.default); // ... existing code ... let tx_result = invoker.invoke(call).await?; - println!("{}", tx_result); + println!(" ✓ Success"); + results.push(tx_result); } + println!("\nExecution Summary:"); + for (i, result) in results.iter().enumerate() { + println!("Call {}: {}", i + 1, result); + }
171-174: Consider adding transaction failure handling optionsOhayo, sensei! The current implementation continues executing remaining calls even if a call fails. Consider adding options for:
- Abort-on-failure mode
- Collecting all errors before reporting
- Optional transaction rollback support
Would you like assistance in implementing these error handling improvements?
bin/sozo/src/commands/execute.rs
Outdated
| fn from_string(s: &str) -> Result<Self> { | ||
| let parts: Vec<&str> = s.split(',').collect(); | ||
| if parts.len() < 2 { | ||
| return Err(anyhow!("Invalid call format")); | ||
| } | ||
|
|
||
| Ok(CallArgs { | ||
| tag_or_address: parts[0].parse()?, | ||
| entrypoint: parts[1].to_string(), | ||
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance input validation in from_string parsing
Ohayo, sensei! The current parsing implementation could be more robust. Consider:
- Trimming whitespace
- Validating entrypoint format
- Providing more descriptive error messages
fn from_string(s: &str) -> Result<Self> {
+ let s = s.trim();
+ if s.is_empty() {
+ return Err(anyhow!("Empty call string"));
+ }
let parts: Vec<&str> = s.split(',').collect();
if parts.len() < 2 {
- return Err(anyhow!("Invalid call format"));
+ return Err(anyhow!("Invalid call format. Expected: <address/tag>,<entrypoint>[,<calldata>]"));
}
+ let entrypoint = parts[1].trim();
+ if !entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') {
+ return Err(anyhow!("Invalid entrypoint format. Must contain only alphanumeric characters and underscores"));
+ }
Ok(CallArgs {
tag_or_address: parts[0].parse()?,
- entrypoint: parts[1].to_string(),
+ entrypoint: entrypoint.to_string(),
calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None },
})📝 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 from_string(s: &str) -> Result<Self> { | |
| let parts: Vec<&str> = s.split(',').collect(); | |
| if parts.len() < 2 { | |
| return Err(anyhow!("Invalid call format")); | |
| } | |
| Ok(CallArgs { | |
| tag_or_address: parts[0].parse()?, | |
| entrypoint: parts[1].to_string(), | |
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | |
| }) | |
| } | |
| fn from_string(s: &str) -> Result<Self> { | |
| let s = s.trim(); | |
| if s.is_empty() { | |
| return Err(anyhow!("Empty call string")); | |
| } | |
| let parts: Vec<&str> = s.split(',').collect(); | |
| if parts.len() < 2 { | |
| return Err(anyhow!("Invalid call format. Expected: <address/tag>,<entrypoint>[,<calldata>]")); | |
| } | |
| let entrypoint = parts[1].trim(); | |
| if !entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { | |
| return Err(anyhow!("Invalid entrypoint format. Must contain only alphanumeric characters and underscores")); | |
| } | |
| Ok(CallArgs { | |
| tag_or_address: parts[0].parse()?, | |
| entrypoint: entrypoint.to_string(), | |
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | |
| }) | |
| } |
|
Let me know @glihm , if you have any issues |
Could you rebase to start the CI for you to have an idea of the state of the PR? |
bin/sozo/src/commands/execute.rs
Outdated
| // #[derive(Debug, Args)] | ||
| // pub struct CallArgs { | ||
| // #[arg( | ||
| // help = "The address or the tag (ex: dojo_examples:actions) of the contract to be executed." | ||
| // )] | ||
| // pub tag_or_address: ResourceDescriptor, | ||
|
|
||
| // #[arg(help = "The name of the entrypoint to be executed.")] | ||
| // pub entrypoint: String, | ||
|
|
||
| // #[arg(short, long)] | ||
| // #[arg(help = "The calldata to be passed to the system. Comma separated values e.g., \ | ||
| // 0x12345,128,u256:9999999999. Sozo supports some prefixes that you can use to \ | ||
| // automatically parse some types. The supported prefixes are: | ||
| // - u256: A 256-bit unsigned integer. | ||
| // - sstr: A cairo short string. | ||
| // - str: A cairo string (ByteArray). | ||
| // - int: A signed integer. | ||
| // - no prefix: A cairo felt or any type that fit into one felt.")] | ||
| // pub calldata: Option<String>, | ||
| // } |
There was a problem hiding this comment.
Remove if not used.
| // #[derive(Debug, Args)] | |
| // pub struct CallArgs { | |
| // #[arg( | |
| // help = "The address or the tag (ex: dojo_examples:actions) of the contract to be executed." | |
| // )] | |
| // pub tag_or_address: ResourceDescriptor, | |
| // #[arg(help = "The name of the entrypoint to be executed.")] | |
| // pub entrypoint: String, | |
| // #[arg(short, long)] | |
| // #[arg(help = "The calldata to be passed to the system. Comma separated values e.g., \ | |
| // 0x12345,128,u256:9999999999. Sozo supports some prefixes that you can use to \ | |
| // automatically parse some types. The supported prefixes are: | |
| // - u256: A 256-bit unsigned integer. | |
| // - sstr: A cairo short string. | |
| // - str: A cairo string (ByteArray). | |
| // - int: A signed integer. | |
| // - no prefix: A cairo felt or any type that fit into one felt.")] | |
| // pub calldata: Option<String>, | |
| // } |
bin/sozo/src/commands/execute.rs
Outdated
| #[arg( | ||
| help = "The address or the tag (ex: dojo_examples:actions) of the contract to be executed." | ||
| help = "The calls to be executed. Each call should include the address or tag, entrypoint, and calldata." |
There was a problem hiding this comment.
Could be good to guide the user by providing an example of the expected format.
bin/sozo/src/commands/execute.rs
Outdated
| fn from_string(s: &str) -> Result<Self> { | ||
| let parts: Vec<&str> = s.split(',').collect(); | ||
| if parts.len() < 2 { | ||
| return Err(anyhow!("Invalid call format")); |
There was a problem hiding this comment.
Why it's invalid. If we can guide the user, we must do it. 👍
bin/sozo/src/commands/execute.rs
Outdated
| // impl CallArgs { | ||
| // fn from_string(s: &str) -> Result<Self> { | ||
| // let parts: Vec<&str> = s.split(',').collect(); | ||
| // if parts.len() < 2 { | ||
| // return Err(anyhow!("Invalid call format")); | ||
| // } | ||
|
|
||
| // Ok(CallArgs { | ||
| // tag_or_address: parts[0].parse()?, | ||
| // entrypoint: parts[1].to_string(), | ||
| // calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | ||
| // }) | ||
| // } | ||
| // } |
There was a problem hiding this comment.
| // impl CallArgs { | |
| // fn from_string(s: &str) -> Result<Self> { | |
| // let parts: Vec<&str> = s.split(',').collect(); | |
| // if parts.len() < 2 { | |
| // return Err(anyhow!("Invalid call format")); | |
| // } | |
| // Ok(CallArgs { | |
| // tag_or_address: parts[0].parse()?, | |
| // entrypoint: parts[1].to_string(), | |
| // calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | |
| // }) | |
| // } | |
| // } |
bin/sozo/src/commands/execute.rs
Outdated
| } | ||
|
|
||
| impl CallArgs { | ||
| fn from_string(s: &str) -> Result<Self> { |
There was a problem hiding this comment.
There's a std trait FromStr. We could use this one?
bin/sozo/src/commands/execute.rs
Outdated
| let contract_address = match &descriptor { | ||
| ResourceDescriptor::Address(address) => Some(*address), | ||
| ResourceDescriptor::Tag(tag) => { | ||
| let selector = naming::compute_selector_from_tag(tag); | ||
| world_diff.get_contract_address(selector) | ||
| } | ||
| ResourceDescriptor::Name(_) => { | ||
| unimplemented!("Expected to be a resolved tag with default namespace.") | ||
| } | ||
| } | ||
| .ok_or_else(|| anyhow!("Contract {descriptor} not found in the world diff."))?; |
There was a problem hiding this comment.
This has been reworked upstream, should be extracted in a function.
bin/sozo/src/commands/execute.rs
Outdated
| selector: snutils::get_selector_from_name(&call_args.entrypoint)?, | ||
| }; | ||
|
|
||
| let invoker = Invoker::new(&account, txn_config); |
There was a problem hiding this comment.
You're not doing a multi call if you invoke the call at each iteration.
Instead, create the Invoker before starting the loop. Then use the Invoker.add_call. Finally, use the Invoker to multi call.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
bin/sozo/src/commands/execute.rs (4)
23-28: Update help text to reflect multi-call supportOhayo, sensei! The help text still describes a single contract execution. Let's update it to reflect the new multi-call functionality.
#[arg( - help = "The address or the tag (ex: dojo_examples:actions) of the contract to be executed." + help = "List of calls to execute. Each call should be in format: <CONTRACT_ADDRESS/TAG>,<ENTRYPOINT>,<ARG1>,<ARG2>,... (ex: dojo_examples:actions,execute,1,2)" )]
77-79: Document unimplemented caseOhayo, sensei! Let's add a more descriptive message for the unimplemented case to help future maintainers.
ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") + unimplemented!( + "Direct name resolution is not supported. Use a fully qualified tag with namespace \ + (e.g., 'namespace:contract') or an address instead." + ) }
116-147: Optimize multi-call implementationOhayo, sensei! The current implementation can be optimized:
- Use
Iterator::try_collectfor more idiomatic error handling- Move descriptor namespace resolution outside the loop
- let call_args_list: Vec<CallArgs> = self.calls.iter() - .map(|s| CallArgs::from_string(s)) - .collect::<Result<Vec<_>>>()?; + let call_args_list: Vec<CallArgs> = self.calls.iter() + .map(|s| s.parse::<CallArgs>()) + .try_collect()?; + // Ensure namespace once for all calls + let namespace = &profile_config.namespace.default; for call_args in call_args_list { - let descriptor = call_args.tag_or_address.ensure_namespace(&profile_config.namespace.default); + let descriptor = call_args.tag_or_address.ensure_namespace(namespace);
148-152: Enhance transaction result reportingOhayo, sensei! Let's provide more detailed feedback about the transaction execution:
let tx_result = invoker.invoke().await?; - println!("{}", tx_result); + println!("Transaction executed successfully!"); + println!("Transaction hash: {}", tx_result); + println!("Number of calls executed: {}", self.calls.len()); Ok(())
bin/sozo/src/commands/execute.rs
Outdated
| #[derive(Debug)] | ||
| pub struct CallArgs { | ||
| pub tag_or_address: ResourceDescriptor, | ||
| pub entrypoint: String, | ||
| pub calldata: Option<String>, | ||
| } | ||
|
|
||
| impl CallArgs { | ||
| fn from_string(s: &str) -> Result<Self> { | ||
| let parts: Vec<&str> = s.split(',').collect(); | ||
| if parts.len() < 2 { | ||
| return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,...")); | ||
| } | ||
|
|
||
| Ok(CallArgs { | ||
| tag_or_address: parts[0].parse()?, | ||
| entrypoint: parts[1].to_string(), | ||
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement FromStr trait and enhance validation
Ohayo, sensei! Let's improve the call parsing implementation:
- Use the standard
FromStrtrait as suggested in previous reviews - Add input validation
- Enhance error messages
#[derive(Debug)]
pub struct CallArgs {
pub tag_or_address: ResourceDescriptor,
pub entrypoint: String,
pub calldata: Option<String>,
}
-impl CallArgs {
- fn from_string(s: &str) -> Result<Self> {
+impl std::str::FromStr for CallArgs {
+ type Err = anyhow::Error;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ let s = s.trim();
+ if s.is_empty() {
+ return Err(anyhow!("Empty call string"));
+ }
let parts: Vec<&str> = s.split(',').collect();
if parts.len() < 2 {
return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,..."));
}
+
+ let entrypoint = parts[1].trim();
+ if entrypoint.is_empty() {
+ return Err(anyhow!("Empty entrypoint"));
+ }
+ if !entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') {
+ return Err(anyhow!("Invalid entrypoint format. Must contain only alphanumeric characters and underscores"));
+ }
Ok(CallArgs {
tag_or_address: parts[0].parse()?,
- entrypoint: parts[1].to_string(),
+ entrypoint: entrypoint.to_string(),
calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None },
})
}
-}📝 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.
| #[derive(Debug)] | |
| pub struct CallArgs { | |
| pub tag_or_address: ResourceDescriptor, | |
| pub entrypoint: String, | |
| pub calldata: Option<String>, | |
| } | |
| impl CallArgs { | |
| fn from_string(s: &str) -> Result<Self> { | |
| let parts: Vec<&str> = s.split(',').collect(); | |
| if parts.len() < 2 { | |
| return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,...")); | |
| } | |
| Ok(CallArgs { | |
| tag_or_address: parts[0].parse()?, | |
| entrypoint: parts[1].to_string(), | |
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | |
| }) | |
| } | |
| } | |
| #[derive(Debug)] | |
| pub struct CallArgs { | |
| pub tag_or_address: ResourceDescriptor, | |
| pub entrypoint: String, | |
| pub calldata: Option<String>, | |
| } | |
| impl std::str::FromStr for CallArgs { | |
| type Err = anyhow::Error; | |
| fn from_str(s: &str) -> Result<Self, Self::Err> { | |
| let s = s.trim(); | |
| if s.is_empty() { | |
| return Err(anyhow!("Empty call string")); | |
| } | |
| let parts: Vec<&str> = s.split(',').collect(); | |
| if parts.len() < 2 { | |
| return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,...")); | |
| } | |
| let entrypoint = parts[1].trim(); | |
| if entrypoint.is_empty() { | |
| return Err(anyhow!("Empty entrypoint")); | |
| } | |
| if !entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { | |
| return Err(anyhow!("Invalid entrypoint format. Must contain only alphanumeric characters and underscores")); | |
| } | |
| Ok(CallArgs { | |
| tag_or_address: parts[0].parse()?, | |
| entrypoint: entrypoint.to_string(), | |
| calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None }, | |
| }) | |
| } | |
| } |
|
Let me know @glihm , for any changes |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
bin/sozo/src/commands/execute.rs (5)
26-27: Enhance help text with more examplesOhayo, sensei! While the help text provides a basic example, it would be more helpful to show multiple examples covering different scenarios:
- Basic call without arguments
- Call with multiple arguments
- Multiple calls in sequence
#[arg( - help = "List of calls to execute. Each call should be in format: <CONTRACT_ADDRESS/TAG>,<ENTRYPOINT>,<ARG1>,<ARG2>,... (ex: dojo_examples:actions,execute,1,2)" + help = "List of calls to execute. Each call should be in format: <CONTRACT_ADDRESS/TAG>,<ENTRYPOINT>[,<ARG1>,<ARG2>,...]\n\ + Examples:\n\ + - Basic: dojo_examples:actions,execute\n\ + - With args: dojo_examples:actions,execute,1,2\n\ + - Multiple: dojo_examples:actions,execute,1 dojo_examples:other,call,3" )]
51-79: Enhance error handling with custom error typesOhayo, sensei! The FromStr implementation looks good, but we can make it more robust:
- Define custom error types for better error handling
- Extract validation constants
- Add unit tests
#[derive(Debug, thiserror::Error)] pub enum CallArgsError { #[error("Empty call string")] EmptyString, #[error("Invalid call format. Expected: <CONTRACT_NAME>,<ENTRYPOINT_NAME>[,<ARG1>,...]")] InvalidFormat, #[error("Empty entrypoint")] EmptyEntrypoint, #[error("Invalid entrypoint format: {0}")] InvalidEntrypoint(String), #[error("Invalid tag or address: {0}")] InvalidTagOrAddress(#[from] anyhow::Error), } impl FromStr for CallArgs { type Err = CallArgsError; fn from_str(s: &str) -> Result<Self, Self::Err> { const MIN_PARTS: usize = 2; let s = s.trim(); if s.is_empty() { return Err(CallArgsError::EmptyString); } let parts: Vec<&str> = s.split(',').collect(); if parts.len() < MIN_PARTS { return Err(CallArgsError::InvalidFormat); } let entrypoint = parts[1].trim(); if entrypoint.is_empty() { return Err(CallArgsError::EmptyEntrypoint); } if !entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { return Err(CallArgsError::InvalidEntrypoint( "Must contain only alphanumeric characters and underscores".into() )); } Ok(CallArgs { tag_or_address: parts[0].parse().map_err(CallArgsError::InvalidTagOrAddress)?, entrypoint: entrypoint.to_string(), calldata: if parts.len() > MIN_PARTS { Some(parts[2..].join(",")) } else { None }, }) } } #[cfg(test)] mod tests { use super::*; #[test] fn test_valid_call_args() { let cases = vec![ ("tag,entry", None), ("tag,entry,1,2", Some("1,2")), ]; for (input, expected_calldata) in cases { let result = CallArgs::from_str(input).unwrap(); assert_eq!(result.calldata.as_deref(), expected_calldata); } } #[test] fn test_invalid_call_args() { assert!(matches!( CallArgs::from_str("").unwrap_err(), CallArgsError::EmptyString )); assert!(matches!( CallArgs::from_str("tag").unwrap_err(), CallArgsError::InvalidFormat )); } }
93-95: Improve error message for unimplemented Name variantOhayo, sensei! The unimplemented! macro should provide more context about why this case isn't handled and what users should do instead.
ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") + unimplemented!( + "ResourceDescriptor::Name is not supported in this context. \ + The descriptor should be resolved to a tag with default namespace \ + before calling this function. Please use ensure_namespace() first." + ) }
136-163: Add progress feedback for multiple callsOhayo, sensei! When processing multiple calls, it would be helpful to provide feedback about the progress.
+ println!("Processing {} calls...", call_args_list.len()); + for call_args in call_args_list { let descriptor = call_args.tag_or_address.ensure_namespace(&profile_config.namespace.default); let contract_address = resolve_contract_address(&descriptor, &world_diff)?; trace!( contract=?descriptor, entrypoint=call_args.entrypoint, calldata=?call_args.calldata, "Executing Execute command." ); let calldata = if let Some(cd) = call_args.calldata { calldata_decoder::decode_calldata(&cd)? } else { vec![] }; let call = Call { calldata, to: contract_address, selector: snutils::get_selector_from_name(&call_args.entrypoint)?, }; invoker.add_call(call); + println!("✓ Added call to {} at {}", call_args.entrypoint, descriptor); } + println!("Executing all calls...");
164-167: Enhance transaction result outputOhayo, sensei! The transaction result could be more informative for multiple calls.
let tx_result = invoker.invoke().await?; - println!("{}", tx_result); + println!("Transaction successful!"); + println!("Transaction hash: {}", tx_result); + println!("All {} calls completed successfully", call_args_list.len()); Ok(())
bin/sozo/src/commands/execute.rs
Outdated
| let call_args_list: Vec<CallArgs> = self.calls.iter() | ||
| .map(|s| CallArgs::from_string(s)) | ||
| .collect::<Result<Vec<_>>>()?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for call parsing
Ohayo, sensei! The current implementation silently collects all errors. Consider handling errors individually to provide better feedback about which calls failed to parse.
- let call_args_list: Vec<CallArgs> = self.calls.iter()
- .map(|s| CallArgs::from_string(s))
- .collect::<Result<Vec<_>>>()?;
+ let mut call_args_list = Vec::new();
+ for (index, call) in self.calls.iter().enumerate() {
+ match call.parse::<CallArgs>() {
+ Ok(args) => call_args_list.push(args),
+ Err(e) => {
+ return Err(anyhow!(
+ "Failed to parse call #{}: '{}' - {}",
+ index + 1,
+ call,
+ e
+ ));
+ }
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Amazing, thanks for the update!
Last thing, the resolution of the tag is now better handled by sozo execute by first checking if there's a local manifest. Which ensure we don't have to pull all the diff from the chain if the local manifest is already here.
Would you mind rebasing and using this new way of resolving the contract's tag? This makes sozo execute much much faster 99% of the time. :)
Thank you for the work on this!
|
Hey @glihm , I did the contract tag change you suggested from my side. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
bin/sozo/src/commands/execute.rs (3)
25-28: Enhance help text to clarify multi-call supportOhayo, sensei! While the help text provides a good example, it could be more explicit about the multi-call capability.
#[arg( - help = "List of calls to execute. Each call should be in format: <CONTRACT_ADDRESS/TAG>,<ENTRYPOINT>,<ARG1>,<ARG2>,... (ex: dojo_examples:actions,execute,1,2)" + help = "List of calls to execute in sequence. Multiple calls can be provided, each in format: <CONTRACT_ADDRESS/TAG>,<ENTRYPOINT>,<ARG1>,<ARG2>,... (ex: dojo_examples:actions,execute,1,2 contract2:system,call,arg1)" )]
44-79: Enhance CallArgs implementation with constants and validationOhayo, sensei! The implementation looks good but could be improved with some refinements:
- Define constants for magic numbers and error messages
- Add maximum length validation for entrypoint
- Consider adding validation for calldata format
#[derive(Debug)] pub struct CallArgs { pub tag_or_address: ResourceDescriptor, pub entrypoint: String, pub calldata: Option<String>, } +const MAX_ENTRYPOINT_LENGTH: usize = 64; +const MIN_PARTS: usize = 2; +const ERR_EMPTY_CALL: &str = "Empty call string"; +const ERR_INVALID_FORMAT: &str = "Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,..."; +const ERR_EMPTY_ENTRYPOINT: &str = "Empty entrypoint"; +const ERR_INVALID_ENTRYPOINT: &str = "Invalid entrypoint format. Must contain only alphanumeric characters and underscores"; impl std::str::FromStr for CallArgs { type Err = anyhow::Error; fn from_str(s: &str) -> Result<Self, Self::Err> { let s = s.trim(); if s.is_empty() { - return Err(anyhow!("Empty call string")); + return Err(anyhow!(ERR_EMPTY_CALL)); } let parts: Vec<&str> = s.split(',').collect(); - if parts.len() < 2 { - return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,...")); + if parts.len() < MIN_PARTS { + return Err(anyhow!(ERR_INVALID_FORMAT)); } let entrypoint = parts[1].trim(); if entrypoint.is_empty() { - return Err(anyhow!("Empty entrypoint")); + return Err(anyhow!(ERR_EMPTY_ENTRYPOINT)); } + if entrypoint.len() > MAX_ENTRYPOINT_LENGTH { + return Err(anyhow!("Entrypoint name too long. Maximum length is {}", MAX_ENTRYPOINT_LENGTH)); + } if !entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { - return Err(anyhow!("Invalid entrypoint format. Must contain only alphanumeric characters and underscores")); + return Err(anyhow!(ERR_INVALID_ENTRYPOINT)); }
81-97: Improve error message for unimplemented caseOhayo, sensei! The unimplemented error message could be more descriptive to help users understand the limitation.
ResourceDescriptor::Name(_) => { - unimplemented!("Expected to be a resolved tag with default namespace.") + unimplemented!( + "Direct name resolution is not supported. Please use a fully qualified tag with default namespace or an address." + ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/sozo/src/commands/execute.rs(4 hunks)
🔇 Additional comments (2)
bin/sozo/src/commands/execute.rs (2)
130-170: LGTM! Well-implemented multi-call execution
Ohayo, sensei! The implementation correctly batches multiple calls into a single transaction using the Invoker, which is the recommended approach for handling multiple calls efficiently.
132-134:
Update to use FromStr trait's parse method
Ohayo, sensei! Since we've implemented FromStr, we should use its parse method instead of from_string.
- let call_args_list: Vec<CallArgs> = self.calls.iter()
- .map(|s| CallArgs::from_string(s))
- .collect::<Result<Vec<_>>>()?;
+ let call_args_list: Vec<CallArgs> = self.calls.iter()
+ .map(|s| s.parse())
+ .collect::<Result<Vec<_>>>()?;Likely invalid or redundant comment.
|
Hey @glihm , I am getting merge conflicts while I rebase. |
Ideally you resolve then by ensuring you're using the new contracts lookup I mentioned before. If you have too much conflicts hard to resolve it's sometime better restarting from scratch since you don't have too much additions. Finally if you don't see how to resolve then we can hop on a call and I can guide you (but not before Monday CST). |
|
Ok @glihm , we look into these changes tomorrow and commit and also rebase. |
Added multi-call support to sozo exceute command
Description
So this PR is adding a mulit-call support to sozo execute command.The variables used are The contract address that is targeted ,The entry point to call, The call data.
It is using a Vec and to mantain the calls and then parsing over each indiviual call to run it.
Related issue
Closes #2388
Summary by CodeRabbit
Summary by CodeRabbit