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): support multicall for execute command #2897

Merged
merged 4 commits into from
Jan 16, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 98 additions & 72 deletions bin/sozo/src/commands/execute.rs
Original file line number Diff line number Diff line change
@@ -18,26 +18,37 @@
use crate::utils;

#[derive(Debug, Args)]
#[command(about = "Execute a system with the given calldata.")]
#[command(about = "Execute one or several systems with the given calldata.")]
pub struct ExecuteArgs {
#[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>,
#[arg(num_args = 1..)]
remybar marked this conversation as resolved.
Show resolved Hide resolved
#[arg(required = true)]
#[arg(help = "A list of calls to execute, separated by a /.

A call is made up of a <TAG_OR_ADDRESS>, an <ENTRYPOINT> and an optional <CALLDATA>:

- <TAG_OR_ADDRESS>: the address or the tag (ex: dojo_examples-actions) of the contract to be \
called,

- <ENTRYPOINT>: the name of the entry point to be called,

- <CALLDATA>: the calldata to be passed to the system.

Space 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.

EXAMPLE

sozo execute 0x1234 run / ns-Actions move 1 2

Executes the run function of the contract at the address 0x1234 without calldata,
and the move function of the ns-Actions contract, with the calldata [1,2].")]
pub calls: Vec<String>,

Check warning on line 51 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L51

Added line #L51 was not covered by tests

#[arg(long)]
#[arg(help = "If true, sozo will compute the diff of the world from the chain to translate \
@@ -65,8 +76,6 @@

let profile_config = ws.load_profile_config()?;

let descriptor = self.tag_or_address.ensure_namespace(&profile_config.namespace.default);

#[cfg(feature = "walnut")]
let walnut_debugger = WalnutDebugger::new_from_flag(
self.transaction.walnut,
@@ -76,64 +85,81 @@
let txn_config: TxnConfig = self.transaction.try_into()?;

config.tokio_handle().block_on(async {
let (contract_address, contracts) = match &descriptor {
ResourceDescriptor::Address(address) => (Some(*address), Default::default()),
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.")
}
};

let contract_address = contract_address.ok_or_else(|| {
let mut message = format!("Contract {descriptor} not found in the manifest.");
if self.diff {
message.push_str(
" Run the command again with `--diff` to force the fetch of data from the \
chain.",
);
}
anyhow!(message)
})?;

trace!(
contract=?descriptor,
entrypoint=self.entrypoint,
calldata=?self.calldata,
"Executing Execute command."
);

let calldata = if let Some(cd) = self.calldata {
calldata_decoder::decode_calldata(&cd)?
} else {
vec![]
};

let call = Call {
calldata,
to: contract_address,
selector: snutils::get_selector_from_name(&self.entrypoint)?,
};

let (provider, _) = self.starknet.provider(profile_config.env.as_ref())?;

let contracts = utils::contracts_from_manifest_or_diff(
self.account.clone(),
self.starknet.clone(),
self.world,
&ws,
self.diff,
)
.await?;

Check warning on line 97 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L90-L97

Added lines #L90 - L97 were not covered by tests

let account = self
.account
.account(provider, profile_config.env.as_ref(), &self.starknet, &contracts)
.await?;

let invoker = Invoker::new(&account, txn_config);
let tx_result = invoker.invoke(call).await?;
let mut invoker = Invoker::new(&account, txn_config);

let mut arg_iter = self.calls.into_iter();

Check warning on line 106 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L104-L106

Added lines #L104 - L106 were not covered by tests

while let Some(arg) = arg_iter.next() {
Comment on lines +106 to +108
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo, sensei! Add validation for minimum number of calls

The iterator implementation should validate that at least one complete call (address + entrypoint) is provided.

Add this validation before the while loop:

             let mut arg_iter = self.calls.into_iter();
+            if self.calls.len() < 2 {
+                return Err(anyhow!("At least one complete call (address and entrypoint) is required"));
+            }
 
             while let Some(arg) = arg_iter.next() {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let mut arg_iter = self.calls.into_iter();
while let Some(arg) = arg_iter.next() {
let mut arg_iter = self.calls.into_iter();
if self.calls.len() < 2 {
return Err(anyhow!("At least one complete call (address and entrypoint) is required"));
}
while let Some(arg) = arg_iter.next() {

let tag_or_address = arg;
let descriptor = ResourceDescriptor::from_string(&tag_or_address)?
.ensure_namespace(&profile_config.namespace.default);

Check warning on line 111 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L108-L111

Added lines #L108 - L111 were not covered by tests

let contract_address = match &descriptor {
ResourceDescriptor::Address(address) => Some(*address),
ResourceDescriptor::Tag(tag) => contracts.get(tag).map(|c| c.address),

Check warning on line 115 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L113-L115

Added lines #L113 - L115 were not covered by tests
ResourceDescriptor::Name(_) => {
unimplemented!("Expected to be a resolved tag with default namespace.")

Check warning on line 117 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L117

Added line #L117 was not covered by tests
}
};
Comment on lines +113 to +119
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo, sensei! Handle ResourceDescriptor::Name variant to prevent runtime panics

Using unimplemented!() in the match arm for ResourceDescriptor::Name(_) will cause a runtime panic if this case is encountered. Consider implementing proper error handling for this variant to prevent unexpected panics.

Apply this diff to handle the Name variant gracefully:

                         ResourceDescriptor::Name(_) => {
-                            unimplemented!("Expected to be a resolved tag with default namespace.")
+                            return Err(anyhow!("Contract address could not be resolved for descriptor: {descriptor}"));
                         }

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


let contract_address = contract_address.ok_or_else(|| {
let mut message = format!("Contract {descriptor} not found in the manifest.");
if self.diff {
message.push_str(
" Run the command again with `--diff` to force the fetch of data from \
the chain.",
);
}
anyhow!(message)
})?;

Check warning on line 130 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L121-L130

Added lines #L121 - L130 were not covered by tests
Comment on lines +123 to +130
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo, sensei! Fix the error message condition to display the suggestion correctly

Currently, the error message suggests running the command again with --diff when self.diff is true. However, if self.diff is already true, the suggestion is redundant. The condition should be inverted to check when self.diff is false.

Apply this diff to fix the condition:

-                        if self.diff {
+                        if !self.diff {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.diff {
message.push_str(
" Run the command again with `--diff` to force the fetch of data from \
the chain.",
);
}
anyhow!(message)
})?;
if !self.diff {
message.push_str(
" Run the command again with `--diff` to force the fetch of data from \
the chain.",
);
}
anyhow!(message)
})?;


let entrypoint = arg_iter.next().ok_or_else(|| {
anyhow!(
"You must specify the entry point of the contract `{tag_or_address}` to \
invoke, and optionally the calldata."
)
})?;

Check warning on line 137 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L132-L137

Added lines #L132 - L137 were not covered by tests

let mut calldata = vec![];
for arg in &mut arg_iter {
let arg = match arg.as_str() {
"/" | "-" | "\\" => break,
_ => calldata_decoder::decode_single_calldata(&arg)?,

Check warning on line 143 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L139-L143

Added lines #L139 - L143 were not covered by tests
};
calldata.extend(arg);

Check warning on line 145 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L145

Added line #L145 was not covered by tests
}
Comment on lines +139 to +146
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo, sensei! Add safety check for calldata parsing

The calldata parsing loop could potentially continue indefinitely if a separator is missing. Additionally, there's no validation of calldata size.

Add safety checks:

                 let mut calldata = vec![];
+                let mut calldata_count = 0;
+                const MAX_CALLDATA_SIZE: usize = 1000; // Adjust based on requirements
                 for arg in &mut arg_iter {
+                    calldata_count += 1;
+                    if calldata_count > MAX_CALLDATA_SIZE {
+                        return Err(anyhow!("Calldata size exceeds maximum limit"));
+                    }
                     let arg = match arg.as_str() {
                         "/" | "-" | "\\" => break,
                         _ => calldata_decoder::decode_single_calldata(&arg)?,
                     };
                     calldata.extend(arg);
                 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let mut calldata = vec![];
for arg in &mut arg_iter {
let arg = match arg.as_str() {
"/" | "-" | "\\" => break,
_ => calldata_decoder::decode_single_calldata(&arg)?,
};
calldata.extend(arg);
}
let mut calldata = vec![];
let mut calldata_count = 0;
const MAX_CALLDATA_SIZE: usize = 1000; // Adjust based on requirements
for arg in &mut arg_iter {
calldata_count += 1;
if calldata_count > MAX_CALLDATA_SIZE {
return Err(anyhow!("Calldata size exceeds maximum limit"));
}
let arg = match arg.as_str() {
"/" | "-" | "\\" => break,
_ => calldata_decoder::decode_single_calldata(&arg)?,
};
calldata.extend(arg);
}


trace!(

Check warning on line 148 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L148

Added line #L148 was not covered by tests
contract=?descriptor,
entrypoint=entrypoint,
calldata=?calldata,
"Decoded call."

Check warning on line 152 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L152

Added line #L152 was not covered by tests
);

invoker.add_call(Call {
to: contract_address,
selector: snutils::get_selector_from_name(&entrypoint)?,
calldata,

Check warning on line 158 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L155-L158

Added lines #L155 - L158 were not covered by tests
});
}

let tx_result = invoker.multicall().await?;

Check warning on line 162 in bin/sozo/src/commands/execute.rs

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L162

Added line #L162 was not covered by tests
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ohayo, sensei! Add transaction size validation

Before executing the multicall, validate the total transaction size.

+            // Validate total transaction size
+            const MAX_TRANSACTION_SIZE: usize = 5000; // Adjust based on chain limits
+            let total_size = invoker.get_calls().iter().map(|call| call.calldata.len()).sum::<usize>();
+            if total_size > MAX_TRANSACTION_SIZE {
+                return Err(anyhow!(
+                    "Total transaction size {} exceeds maximum limit {}",
+                    total_size,
+                    MAX_TRANSACTION_SIZE
+                ));
+            }
+
             let tx_result = invoker.multicall().await?;

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


#[cfg(feature = "walnut")]
if let Some(walnut_debugger) = walnut_debugger {
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/mod.rs
Original file line number Diff line number Diff line change
@@ -49,7 +49,7 @@ pub enum Commands {
#[command(about = "Run a migration, declaring and deploying contracts as necessary to update \
the world")]
Migrate(Box<MigrateArgs>),
#[command(about = "Execute a system with the given calldata.")]
#[command(about = "Execute one or several systems with the given calldata.")]
Execute(Box<ExecuteArgs>),
#[command(about = "Inspect the world")]
Inspect(Box<InspectArgs>),
4 changes: 2 additions & 2 deletions crates/dojo/world/src/config/calldata_decoder.rs
Original file line number Diff line number Diff line change
@@ -140,7 +140,7 @@ pub fn decode_calldata(input: &str) -> DecoderResult<Vec<Felt>> {
let mut calldata = vec![];

for item in items {
calldata.extend(decode_inner(item)?);
calldata.extend(decode_single_calldata(item)?);
}

Ok(calldata)
@@ -154,7 +154,7 @@ pub fn decode_calldata(input: &str) -> DecoderResult<Vec<Felt>> {
///
/// # Returns
/// A vector of [`Felt`]s.
fn decode_inner(item: &str) -> DecoderResult<Vec<Felt>> {
pub fn decode_single_calldata(item: &str) -> DecoderResult<Vec<Felt>> {
let item = item.trim();

let felts = if let Some((prefix, value)) = item.split_once(ITEM_PREFIX_DELIMITER) {
2 changes: 1 addition & 1 deletion crates/sozo/ops/src/resource_descriptor.rs
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@ use anyhow::Result;
use dojo_world::contracts::naming;
use starknet::core::types::Felt;

#[derive(Debug, Clone)]
#[derive(Debug, Clone, PartialEq)]
pub enum ResourceDescriptor {
Address(Felt),
Name(String),