Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sozo) : multicall from CLI #2679

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

Manush-2005
Copy link

@Manush-2005 Manush-2005 commented Nov 11, 2024

Description

This PR will ad the Sozo mutiple call feature. This PR is related to #2620 which is closed

Related issue

This is second PR of the issue #2620

Summary by CodeRabbit

  • New Features

    • Enhanced command execution to allow multiple calls in a single command.
    • Introduced a new structure for detailed call arguments, improving clarity and usability.
  • Bug Fixes

    • Improved error handling for call parsing, ensuring better feedback for invalid input formats.
    • Enhanced transaction configuration error handling for improved reliability.

Copy link

coderabbitai bot commented Nov 11, 2024

Walkthrough

Ohayo, sensei! The changes in this pull request modify the ExecuteArgs structure in execute.rs by replacing the previous fields for single calls with a new calls field, enabling multiple calls in one command. A new structure, CallArgs, has been introduced to encapsulate details of each call, along with parsing logic for string representations. The run method and the invocation of the Invoker have been updated to accommodate this new structure, enhancing error handling and control flow for executing multiple calls.

Changes

File Path Change Summary
bin/sozo/src/commands/execute.rs - Updated ExecuteArgs: removed tag_or_address, entrypoint, calldata; added calls and diff.
- Added new struct CallArgs: includes tag_or_address, entrypoint, calldata.
- Implemented FromStr for CallArgs for parsing logic.
- Modified run method to handle multiple calls and updated error handling.
- Retained resolve_contract_address for processing CallArgs.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ExecuteArgs
    participant CallArgs
    participant Invoker

    User->>ExecuteArgs: Execute command with multiple calls
    ExecuteArgs->>CallArgs: Parse calls into CallArgs instances
    CallArgs->>ExecuteArgs: Return parsed CallArgs
    ExecuteArgs->>Invoker: Invoke multiple calls
    Invoker->>ExecuteArgs: Return execution results
    ExecuteArgs->>User: Return results of multiple calls
Loading

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Manush-2005
Copy link
Author

Hey @glihm , I did what I could from my side.

@Manush-2005
Copy link
Author

Can you suggest me changes in the current file to resolve these conflicts

@Manush-2005
Copy link
Author

It will be big help

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
bin/sozo/src/commands/execute.rs (1)

65-65: Ensure consistency in error messages.

Ohayo, sensei! The error message at line 65 refers to <CONTRACT_NAME>, while the help text uses <CONTRACT_ADDRESS/TAG>. To maintain clarity and consistency, consider updating the error message to match the format in the help text.

Apply this diff to update the error message:

-return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,..."));
+return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_ADDRESS/TAG>,<ENTRYPOINT>,<ARG1>,<ARG2>,..."));
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 74280d4 and 6e4a960.

📒 Files selected for processing (1)
  • bin/sozo/src/commands/execute.rs (3 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/execute.rs (1)

72-74: Reevaluate entrypoint name validation.

Ohayo, sensei! The current validation only allows alphanumeric characters and underscores in entrypoint names. Depending on the system's requirements, entrypoint names might include additional characters or be case-sensitive. Please verify if this restriction aligns with the allowed naming conventions.

If you need assistance adjusting the validation logic, I'm here to help!

Comment on lines +77 to +79
tag_or_address: parts[0].parse()?,
entrypoint: entrypoint.to_string(),
calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve calldata parsing to handle commas within arguments.

Ohayo, sensei! The current parsing logic joins calldata arguments with commas, which may cause issues if an argument itself contains a comma. Consider enhancing the parsing mechanism to handle such cases, possibly by using a different delimiter or supporting argument escaping.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the modification.

Some other comments, and if you could rebase on dojo/main that would be perfect to then prepare for merging if the next iteration is ok. 👍

If you have questions, reach out to me on the dojo discord.

fn from_str(s: &str) -> Result<Self, Self::Err> {
let s = s.trim();
if s.is_empty() {
return Err(anyhow!("Empty call string"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call args may be empty, some entrypoints don't take any arguments.

Comment on lines 93 to 94
.get_contract_address(selector)
.ok_or_else(|| anyhow!("Contract {descriptor} not found in the world diff."))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the way you can get the contract address, since the world diff is not always computed if we have a local manifest.

let contracts = utils::contracts_from_manifest_or_diff(

@glihm glihm changed the title feat(sozo) : Mutiple call feature feat(sozo) : multicall from CLI Nov 11, 2024
@Manush-2005
Copy link
Author

Hey @glihm , I did the changes you suggested.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (4)
bin/sozo/src/commands/execute.rs (4)

23-31: Enhance help text clarity for better user experience.

Ohayo, sensei! The help text could be more descriptive about the optional nature of calldata and include multiple examples covering different scenarios.

-        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\
+               - With args: dojo_examples:actions,execute,1,2\n\
+               - Without args: my_contract,initialize\n\
+               - Multiple calls: contract1,method1,1 contract2,method2"

61-78: Consider enhancing argument parsing robustness.

Ohayo, sensei! While the current implementation handles basic parsing well, consider these improvements:

  1. Support for quoted arguments to handle special characters
  2. Proper error messages for malformed inputs
  3. Trimming of whitespace for each argument, not just the first part
 fn from_str(s: &str) -> Result<Self, Self::Err> {
     let s = s.trim();
-    let parts: Vec<&str> = s.split(',').collect();
+    let parts: Vec<&str> = s
+        .split(',')
+        .map(str::trim)
+        .collect();
     if parts.len() < 2 {
-        return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,..."));
+        return Err(anyhow!(
+            "Invalid call format. Expected at least contract name and entrypoint.\n\
+             Format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>[,<ARG1>,<ARG2>,...]"
+        ));
     }

136-172: Consider adding safeguards for multiple calls.

Ohayo, sensei! While the implementation handles multiple calls well, consider adding:

  1. Maximum batch size limit to prevent excessive gas usage
  2. Gas estimation before execution
  3. Progress feedback for multiple calls
+    const MAX_BATCH_SIZE: usize = 10; // Adjust based on requirements
+
     let call_args_list: Vec<CallArgs> = self.calls.iter()
         .map(|s| s.parse())
         .collect::<Result<Vec<_>>>()?;

+    if call_args_list.len() > MAX_BATCH_SIZE {
+        return Err(anyhow!(
+            "Batch size {} exceeds maximum allowed size of {}",
+            call_args_list.len(),
+            MAX_BATCH_SIZE
+        ));
+    }

     for call_args in call_args_list {

157-161: Enhance error handling for calldata parsing.

Ohayo, sensei! The calldata parsing could benefit from more descriptive error messages and validation:

 let calldata = if let Some(cd) = call_args.calldata {
-    calldata_decoder::decode_calldata(&cd)?
+    calldata_decoder::decode_calldata(&cd).map_err(|e| {
+        anyhow!(
+            "Failed to decode calldata '{}' for entrypoint '{}': {}",
+            cd,
+            call_args.entrypoint,
+            e
+        )
+    })?
 } else {
     vec![]
 };
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6e4a960 and 9a4eeb9.

📒 Files selected for processing (1)
  • bin/sozo/src/commands/execute.rs (3 hunks)

Comment on lines 82 to 103
fn resolve_contract_address(
descriptor: &ResourceDescriptor,
world_diff: &WorldDiff,
) -> Result<Address> {
match descriptor {
ResourceDescriptor::Address(address) => Ok(*address),
ResourceDescriptor::Tag(tag) => {
let contracts = utils::contracts_from_manifest_or_diff(
self.account.clone(),
self.starknet.clone(),
self.world,
&ws,
self.diff,
)
.await?;

(contracts.get(tag).map(|c| c.address), contracts)
}
ResourceDescriptor::Name(_) => {
unimplemented!("Expected to be a resolved tag with default namespace.")
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix contract address resolution implementation.

Ohayo, sensei! There are several issues in this function:

  1. The function uses self but is not a method (lines 90-92)
  2. The Name variant is unimplemented but should handle the default namespace case
 fn resolve_contract_address(
     descriptor: &ResourceDescriptor,
     world_diff: &WorldDiff,
+    options: &ExecuteArgs,
+    ws: &Workspace,
 ) -> Result<Address> {
     match descriptor {
         ResourceDescriptor::Address(address) => Ok(*address),
         ResourceDescriptor::Tag(tag) => {
             let contracts = utils::contracts_from_manifest_or_diff(
-                self.account.clone(),
-                self.starknet.clone(),
-                self.world,
+                options.account.clone(),
+                options.starknet.clone(),
+                options.world,
                 &ws,
-                self.diff,
+                options.diff,
             )
             .await?;

             (contracts.get(tag).map(|c| c.address), contracts)
         }
         ResourceDescriptor::Name(name) => {
-            unimplemented!("Expected to be a resolved tag with default namespace.")
+            // Handle name with default namespace
+            let tag = format!("{}:{}", profile_config.namespace.default, name);
+            resolve_contract_address(&ResourceDescriptor::Tag(tag), world_diff, options, ws)
         }
     }
 }

Committable suggestion skipped: line range outside the PR's diff.

@Manush-2005
Copy link
Author

Ok @glihm , after resolving all the commits and conflicts I have done it

@Manush-2005
Copy link
Author

And also @glihm , made the changes you requested

@Manush-2005
Copy link
Author

Thanks @glihm , for being a kind maintainer and helping me. I have learned a lot from this PR.
A sincere thank you

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
bin/sozo/src/commands/execute.rs (3)

24-31: Enhance help text clarity for better user experience.

Ohayo, sensei! While the help text provides a basic example, it could be more comprehensive. Consider adding:

  • Multiple call examples
  • Explanation of optional calldata
  • Clear distinction between address and tag usage
-        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 format: <CONTRACT_ADDRESS/TAG>,<ENTRYPOINT>[,<ARG1>,<ARG2>,...]\n\
+               Examples:\n\
+               - Single call: dojo_examples:actions,execute,1,2\n\
+               - Multiple calls: 0x123...,write,1 dojo_examples:other,read\n\
+               Note: Arguments are optional. Both addresses and tags are supported."

46-51: Consider adding validation methods to CallArgs.

Ohayo, sensei! The struct looks good, but consider adding validation methods to encapsulate common checks:

impl CallArgs {
    pub fn validate(&self) -> Result<()> {
        if self.entrypoint.is_empty() {
            return Err(anyhow!("Empty entrypoint"));
        }
        if !self.entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') {
            return Err(anyhow!("Invalid entrypoint format"));
        }
        Ok(())
    }
}

138-175: Consider adding batch size limits and progress feedback.

Ohayo, sensei! While the multicall implementation is solid, consider these enhancements:

  1. Add a maximum batch size limit to prevent excessive gas usage
  2. Provide progress feedback for multiple calls
+            const MAX_BATCH_SIZE: usize = 20; // Adjust based on gas considerations
+            if call_args_list.len() > MAX_BATCH_SIZE {
+                return Err(anyhow!("Batch size exceeds maximum allowed ({})", MAX_BATCH_SIZE));
+            }

+            println!("Processing {} calls...", call_args_list.len());
             for (i, call_args) in call_args_list.iter().enumerate() {
+                println!("Processing call {}/{}: {}", i + 1, call_args_list.len(), call_args.entrypoint);
                 // ... existing code ...
             }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9a4eeb9 and 970e489.

📒 Files selected for processing (1)
  • bin/sozo/src/commands/execute.rs (3 hunks)
🔇 Additional comments (2)
bin/sozo/src/commands/execute.rs (2)

61-78: ⚠️ Potential issue

Improve calldata parsing robustness.

Ohayo, sensei! The current implementation has a limitation with commas in calldata, as noted in previous reviews. Consider using a more robust parsing approach:

-        let parts: Vec<&str> = s.split(',').collect();
+        // Split only on first two commas for contract and entrypoint
+        let mut parts = s.splitn(3, ',');
+        let contract = parts.next().ok_or_else(|| anyhow!("Missing contract address/tag"))?;
+        let entrypoint = parts.next().ok_or_else(|| anyhow!("Missing entrypoint"))?;
+        let calldata = parts.next().map(|s| s.to_string());
+
         Ok(CallArgs {
-            tag_or_address: parts[0].parse()?,
-            entrypoint: entrypoint.to_string(),
-            calldata: if parts.len() > 2 { Some(parts[2..].join(",")) } else { None },
+            tag_or_address: contract.parse()?,
+            entrypoint: entrypoint.trim().to_string(),
+            calldata,
         })

82-106: ⚠️ Potential issue

Address the unimplemented Name variant.

Ohayo, sensei! The Name variant is still unimplemented, as noted in previous reviews:

The implementation should handle the default namespace case as suggested in previous reviews.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Manush-2005!

I'll do a functional review later today, would you mind adjusting the formatting and checking your code compiles?

You have some scripts/ available that you can run to help you having everything ready for the CI. 👍

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (4)
bin/sozo/src/commands/execute.rs (4)

26-28: Enhance help text for better clarity.

Ohayo, sensei! The help text could be more helpful with:

  1. Example showing multiple calls
  2. Clarification that calldata is optional
     #[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>,...] \
+               Multiple calls can be specified. Examples:\n\
+               - Single call: dojo_examples:actions,execute,1,2\n\
+               - Multiple calls: dojo_examples:actions,execute,1,2 other_contract:actions,execute"
     )]

58-72: Add additional validation checks for robustness.

Ohayo, sensei! Consider adding these validations:

  1. Check for empty tag/address in parts[0]
  2. Add maximum length check for entrypoint (e.g., 31 characters for Cairo)
  3. Consider trimming whitespace from all parts, not just the first one
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let s = s.trim();
 
         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 tag_or_address = parts[0].trim();
+        if tag_or_address.is_empty() {
+            return Err(anyhow!("Empty contract address or tag"));
+        }
+
         let entrypoint = parts[1].trim();
         if entrypoint.is_empty() {
             return Err(anyhow!("Empty entrypoint"));
         }
+        if entrypoint.len() > 31 {
+            return Err(anyhow!("Entrypoint name too long. Maximum length is 31 characters"));
+        }
         if !entrypoint.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') {
             return Err(anyhow!("Invalid entrypoint format. Must contain only alphanumeric characters and underscores"));
         }

103-103: Improve error message specificity.

Ohayo, sensei! The error message could be more helpful by including the actual reason why the contract wasn't found (e.g., invalid tag, network issues, etc.).

-                .ok_or_else(|| anyhow!("Contract {descriptor} not found in the world diff."))
+                .ok_or_else(|| anyhow!("Contract {descriptor} not found. Please ensure the tag exists in the manifest or the contract is deployed if using an address."))

144-176: Consider adding safety limits for multicalls.

Ohayo, sensei! While the multicall implementation works, it might be good to add some safety measures:

  1. Maximum number of calls in a batch
  2. Maximum total calldata size
  3. Gas estimation before execution

Consider adding configuration options for these limits and implementing checks like:

const MAX_CALLS_PER_BATCH: usize = 20;
const MAX_TOTAL_CALLDATA_SIZE: usize = 1024 * 1024; // 1MB

if call_args_list.len() > MAX_CALLS_PER_BATCH {
    return Err(anyhow!("Too many calls in batch. Maximum is {}", MAX_CALLS_PER_BATCH));
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 970e489 and e25f6c7.

📒 Files selected for processing (1)
  • bin/sozo/src/commands/execute.rs (4 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/execute.rs (1)

178-178: ⚠️ Potential issue

Fix undefined variable in invoke call.

Ohayo, sensei! The invoke method is using an undefined call variable. This should be removed as the calls are already added to the invoker.

-            let tx_result = invoker.invoke(call).await?; // Invoking the multi-call
+            let tx_result = invoker.invoke().await?; // Invoking the multi-call

Likely invalid or redundant comment.

if let Some(local_address) = ws.get_contract_address(&descriptor) {
local_address
} else {
resolve_contract_address(&descriptor, &self, &ws).await?;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect argument in resolve_contract_address call.

Ohayo, sensei! There's a bug in the resolve_contract_address call. It's passing &self instead of &world_diff.

-                        resolve_contract_address(&descriptor, &self, &ws).await?;
+                        resolve_contract_address(&descriptor, &world_diff, &self, &ws).await?;

Committable suggestion skipped: line range outside the PR's diff.

@Manush-2005
Copy link
Author

Hey @glihm , I have done some fmt and also removed some errors in build process

@Manush-2005
Copy link
Author

Let me know if there are any more stuff to be done

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (2)
bin/sozo/src/commands/execute.rs (2)

26-28: LGTM! Consider enhancing the help text.

Ohayo, sensei! The help text is clear, but we could make it even more helpful by adding examples for different types of arguments (strings, numbers, addresses).

-        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>,... (ex: dojo_examples:actions,execute,1,2 or my_contract,set_value,\"hello world\",0x1234)"

163-167: Enhance error handling for calldata decoding.

Ohayo, sensei! Consider adding more context to the error when calldata decoding fails.

 let calldata = if let Some(cd) = call_args.calldata {
-    calldata_decoder::decode_calldata(&cd)?
+    calldata_decoder::decode_calldata(&cd).map_err(|e| {
+        anyhow!(
+            "Failed to decode calldata for contract {:?}, entrypoint {}: {}",
+            descriptor,
+            call_args.entrypoint,
+            e
+        )
+    })?
 } else {
     vec![]
 };
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e25f6c7 and 608bd9f.

📒 Files selected for processing (1)
  • bin/sozo/src/commands/execute.rs (4 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/execute.rs (1)

178-178: ⚠️ Potential issue

Fix undefined variable in invoke call.

Ohayo, sensei! The invoke method is using an undefined variable 'call'.

-            let tx_result = invoker.invoke(call).await?; // Invoking the multi-call
+            let tx_result = invoker.invoke().await?; // Invoking the multi-call

Likely invalid or redundant comment.

Comment on lines +153 to +154
resolve_contract_address(&descriptor, &world_diff, &ws).await?;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect arguments in resolve_contract_address call.

Ohayo, sensei! The resolve_contract_address call is missing the options argument.

-                        resolve_contract_address(&descriptor, &world_diff, &ws).await?;
+                        resolve_contract_address(&descriptor, &world_diff, &self, &ws).await?;
📝 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.

Suggested change
resolve_contract_address(&descriptor, &world_diff, &ws).await?;
};
resolve_contract_address(&descriptor, &world_diff, &self, &ws).await?;
};

Comment on lines +105 to +107
ResourceDescriptor::Name(_) => {
unimplemented!("Expected to be a resolved tag with default namespace.")
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement the Name variant resolution.

Ohayo, sensei! The Name variant is currently unimplemented, which could cause runtime panics.

-        ResourceDescriptor::Name(_) => {
-            unimplemented!("Expected to be a resolved tag with default namespace.")
-        }
+        ResourceDescriptor::Name(name) => {
+            let tag = format!("{}:{}", profile_config.namespace.default, name);
+            resolve_contract_address(
+                &ResourceDescriptor::Tag(tag),
+                world_diff,
+                options,
+                ws
+            ).await
+        }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +55 to +80
impl std::str::FromStr for CallArgs {
type Err = anyhow::Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let s = s.trim();

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 },
})
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance the parsing logic for better robustness.

Ohayo, sensei! The current parsing implementation has some potential issues:

  1. It doesn't handle escaped commas in arguments
  2. Missing validation for empty tag/address
  3. Individual parts after split aren't trimmed
 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();
+    // Handle escaped commas
+    let mut parts = Vec::new();
+    let mut current = String::new();
+    let mut escaped = false;
+    
+    for c in s.chars() {
+        match (c, escaped) {
+            ('\\', false) => escaped = true,
+            (',', false) => {
+                parts.push(current.trim().to_string());
+                current.clear();
+            },
+            (c, _) => {
+                current.push(c);
+                escaped = false;
+            }
+        }
+    }
+    parts.push(current.trim().to_string());
+
     if parts.len() < 2 {
         return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,..."));
     }
 
-    let entrypoint = parts[1].trim();
+    let tag_or_address = parts[0].trim();
+    if tag_or_address.is_empty() {
+        return Err(anyhow!("Empty contract address/tag"));
+    }
+
+    let entrypoint = parts[1].trim();
     if entrypoint.is_empty() {
         return Err(anyhow!("Empty entrypoint"));
     }
📝 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.

Suggested change
impl std::str::FromStr for CallArgs {
type Err = anyhow::Error;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let s = s.trim();
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 },
})
}
}
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"));
}
// Handle escaped commas
let mut parts = Vec::new();
let mut current = String::new();
let mut escaped = false;
for c in s.chars() {
match (c, escaped) {
('\\', false) => escaped = true,
(',', false) => {
parts.push(current.trim().to_string());
current.clear();
},
(c, _) => {
current.push(c);
escaped = false;
}
}
}
parts.push(current.trim().to_string());
if parts.len() < 2 {
return Err(anyhow!("Invalid call format. Expected format: <CONTRACT_NAME>,<ENTRYPOINT_NAME>,<ARG1>,<ARG2>,..."));
}
let tag_or_address = parts[0].trim();
if tag_or_address.is_empty() {
return Err(anyhow!("Empty contract address/tag"));
}
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 },
})
}
}

Comment on lines +55 to +67
impl std::str::FromStr for CallArgs {
type Err = anyhow::Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let s = s.trim();

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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please write a unit test for this function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kariy, can you tell me the steps to write a unit test for this function? I am kinda new to this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @glihm , I am actually kinda getting an error running the test
This errors are like this:
Compiling zstd v0.13.2 Compiling nextest-runner v0.65.0 Finishedreleaseprofile [optimized] target(s) in 2m 27s Installing /home/manush2005/.cargo/bin/cargo-nextest Installed packagecargo-nextest v0.9.82(executablecargo-nextest) manush2005@Manush:/mnt/c/Users/Manush/OneDrive/Desktop/Manush/dojo$ cargo nextest run --all-features -p sozo Updating crates.io index Updating git repository https://github.com/dojoengine/dojo`
error: failed to get dojo-lang as a dependency of package scarb v2.8.4 (https://github.com/dojoengine/scarb?rev=7eac49b3e61236ce466e712225d9c989f9db1ef3#7eac49b3)
... which satisfies git dependency scarb (locked to 2.8.4) of package dojo-test-utils v1.0.0 (/mnt/c/Users/Manush/OneDrive/Desktop/Manush/dojo/crates/dojo/test-utils)
... which satisfies path dependency dojo-test-utils (locked to 1.0.0) of package dojo-utils v1.0.0 (/mnt/c/Users/Manush/OneDrive/Desktop/Manush/dojo/crates/dojo/utils)
... which satisfies path dependency dojo-utils (locked to 1.0.0) of package katana v1.0.0 (/mnt/c/Users/Manush/OneDrive/Desktop/Manush/dojo/bin/katana)

Caused by:
failed to load source for dependency dojo-lang

Caused by:
Unable to update https://github.com/dojoengine/dojo?rev=6725a8f20af56213fa7382aa1e158817f3ee623c#6725a8f2

Caused by:
failed to fetch into: /home/manush2005/.cargo/git/db/dojo-10cac2e09298cf35

Caused by:
network failure seems to have happened
if a proxy or similar is necessary net.git-fetch-with-cli may help here
https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
SSL error: unknown error; class=Ssl (16)`

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also , @glihm and @kariy , can you please highlight some changes I need to make in the current code. It is diffcult for me to grasp as it is my frist time contributing to dojo

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Manush-2005 how are you trying to compile the code locally?

You should follow the CONTRIBUTING.md guide to help you out.
If you have any question, you're welcome in the dojo discord and happy to have a call to show you the flow if you still have complications to build.

@glihm
Copy link
Collaborator

glihm commented Nov 19, 2024

Do you have some updates here @Manush-2005?

@Manush-2005
Copy link
Author

Do you have some updates here @Manush-2005?

Hey, @glihm , I am so sorry for my incovience.My university exams are going on.So I am busy with that this days. I will look into this PR

@glihm
Copy link
Collaborator

glihm commented Nov 20, 2024

Do you have some updates here @Manush-2005?

Hey, @glihm , I am so sorry for my incovience.My university exams are going on.So I am busy with that this days. I will look into this PR

No pressure, wanted to know if you were ok to continue or not, but seems yes. :) Sounds good, best of luck on your exams and thank you for the update. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants