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.

Sign up for GitHub

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

feat(sozo): support multicall for execute command #2897

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad1ce8b
sozo: support multicall for execute command
remybar Jan 11, 2025
09b00d1
use space instead of comma as separator
remybar Jan 14, 2025
01aad32
improve entrypoint error message
remybar Jan 14, 2025
6101109
fix: reword error message for entrypoint
glihm Jan 16, 2025
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
165 changes: 93 additions & 72 deletions bin/sozo/src/commands/execute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

View check run for this annotation

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 \
Expand Down Expand Up @@ -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,
Expand All @@ -76,64 +85,76 @@
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

View check run for this annotation

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

View check run for this annotation

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 = ResourceDescriptor::from_string(&arg)?;
let descriptor = tag_or_address.ensure_namespace(&profile_config.namespace.default);

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L108 - L110 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 114 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L112-L114 

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L116 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 129 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L120-L129 

Added lines #L120 - L129 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!("Unexpected number of arguments"))?;

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L131 - L132 were not covered by tests
remybar marked this conversation as resolved.
Show resolved Hide resolved

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 138 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L134-L138 

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L140 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 143 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

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

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L147 was not covered by tests
);

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

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L150-L153 

Added lines #L150 - L153 were not covered by tests
});
}

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L157 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 {
Expand Down
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>),
Expand Down
4 changes: 2 additions & 2 deletions crates/dojo/world/src/config/calldata_decoder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion crates/sozo/ops/src/resource_descriptor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Loading