bin/sozo/src/commands/execute.rs (2)

109-115: Improve logging for each executed call

Ohayo, 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 execution

Ohayo, 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),
    }
}

Configuration used: .coderabbit.yaml
Review profile: CHILL

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:

1. Batch error handling:

• Currently, the first error stops all subsequent calls
• Consider collecting errors and continuing execution

2. 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"));
}

Configuration used: .coderabbit.yaml
Review profile: CHILL

bin/sozo/src/commands/execute.rs (3)

43-63: Remove commented code but preserve documentation.

Ohayo, sensei! The commented CallArgs struct 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:

1. No batch transaction support (all calls are separate transactions)
2. No strategy for handling partial failures (what happens if call Implement World contract #5 fails in a sequence of 10?)
3. No progress indication for multiple calls
4. Redundant world_diff lookups for each call

Consider these improvements:

1. Add support for batch transactions where applicable
2. Implement a failure strategy (continue/abort on failure)
3. Add progress reporting for multiple calls
4. 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);

Configuration used: .coderabbit.yaml
Review profile: CHILL

bin/sozo/src/commands/execute.rs (3)

23-28: Enhance help text with format specification

Ohayo, 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 feedback

Ohayo, sensei! While the implementation works, consider these improvements:

1. Add progress indication for multiple calls
2. Provide clear success/failure status for each call
3. 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 options

Ohayo, sensei! The current implementation continues executing remaining calls even if a call fails. Consider adding options for:

1. Abort-on-failure mode
2. Collecting all errors before reporting
3. Optional transaction rollback support

Would you like assistance in implementing these error handling improvements?

Configuration used: .coderabbit.yaml
Review profile: CHILL

bin/sozo/src/commands/execute.rs (4)

23-28: Update help text to reflect multi-call support

Ohayo, 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 case

Ohayo, 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 implementation

Ohayo, sensei! The current implementation can be optimized:

1. Use Iterator::try_collect for more idiomatic error handling
2. 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 reporting

Ohayo, 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(())

Configuration used: .coderabbit.yaml
Review profile: CHILL

bin/sozo/src/commands/execute.rs (5)

26-27: Enhance help text with more examples

Ohayo, sensei! While the help text provides a basic example, it would be more helpful to show multiple examples covering different scenarios:

1. Basic call without arguments
2. Call with multiple arguments
3. 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 types

Ohayo, sensei! The FromStr implementation looks good, but we can make it more robust:

1. Define custom error types for better error handling
2. Extract validation constants
3. 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 variant

Ohayo, 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 calls

Ohayo, 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 output

Ohayo, 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(())

Configuration used: .coderabbit.yaml
Review profile: CHILL

bin/sozo/src/commands/execute.rs (3)

25-28: Enhance help text to clarify multi-call support

Ohayo, 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 validation

Ohayo, sensei! The implementation looks good but could be improved with some refinements:

1. Define constants for magic numbers and error messages
2. Add maximum length validation for entrypoint
3. 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 case

Ohayo, 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."
+            )
         }

Configuration used: .coderabbit.yaml
Review profile: CHILL

-            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<_>>>()?;