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
Show file tree
Hide file tree
Changes from 1 commit
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
209 changes: 139 additions & 70 deletions bin/sozo/src/commands/execute.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::str::FromStr;

use anyhow::{anyhow, Result};
use clap::Args;
use dojo_utils::{Invoker, TxnConfig};
Expand All @@ -9,6 +11,7 @@
use sozo_walnut::WalnutDebugger;
use starknet::core::types::Call;
use starknet::core::utils as snutils;
use starknet_crypto::Felt;
use tracing::trace;

use super::options::account::AccountOptions;
Expand All @@ -17,27 +20,64 @@
use super::options::world::WorldOptions;
use crate::utils;

#[derive(Debug, Clone)]
pub struct CallArguments {
pub tag_or_address: ResourceDescriptor,
pub entrypoint: String,
pub calldata: Vec<Felt>,
}

impl FromStr for CallArguments {
type Err = anyhow::Error;

fn from_str(s: &str) -> Result<Self> {
let parts = s.splitn(3, ",").collect::<Vec<_>>();

if parts.len() < 2 {
return Err(anyhow!(
"Expected call format: tag_or_address,entrypoint[,calldata1,...,calldataN]"
));
}

let tag_or_address = ResourceDescriptor::from_string(parts[0])?;
let entrypoint = parts[1].to_string();
let calldata =
if parts.len() > 2 { calldata_decoder::decode_calldata(parts[2])? } else { vec![] };

Ok(CallArguments { tag_or_address, entrypoint, calldata })
}
}

#[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(num_args = 1..)]
remybar marked this conversation as resolved.
Show resolved Hide resolved
#[arg(help = "A list of calls to execute.\n
A call is made up of 3 values, separated by a comma (<TAG_OR_ADDRESS>,<ENTRYPOINT>[,<CALLDATA>]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be more natural to have the calldata separated from the two first arguments.

Starkli is doing that: https://github.com/xJonathanLEI/starkli/blob/4c92ac0ee3c8a50289b09760ec535cf27dd64b2c/src/subcommands/invoke.rs#L62

Wondering what could be best to have something easy to use, wdyt on that?


#[arg(help = "The name of the entrypoint to be executed.")]
pub entrypoint: String,
- <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.

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.

#[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>,
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<CallArguments>,

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L80 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 +105,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 +114,62 @@
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 126 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L119-L126

Added lines #L119 - L126 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);

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L133 was not covered by tests

for call in self.calls {
let descriptor =
call.tag_or_address.ensure_namespace(&profile_config.namespace.default);

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L135 - L137 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 141 in bin/sozo/src/commands/execute.rs

View check run for this annotation

Codecov / codecov/patch

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

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L147 - L156 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)
})?;


trace!(

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L158 was not covered by tests
contract=?descriptor,
entrypoint=call.entrypoint,
calldata=?call.calldata,
"Executing Execute command."

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L162 was not covered by tests
);

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

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

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/execute.rs#L165-L168

Added lines #L165 - L168 were not covered by tests
});
}

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L172 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 All @@ -145,3 +181,36 @@
})
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_call_arguments_from_str() {
let res = CallArguments::from_str("0x1234,run").unwrap();
assert!(res.tag_or_address == ResourceDescriptor::from_string("0x1234").unwrap());
assert!(res.entrypoint == "run");

let res = CallArguments::from_str("dojo-Player,run").unwrap();
assert!(res.tag_or_address == ResourceDescriptor::from_string("dojo-Player").unwrap());
assert!(res.entrypoint == "run");

let res = CallArguments::from_str("Player,run").unwrap();
assert!(res.tag_or_address == ResourceDescriptor::from_string("Player").unwrap());
assert!(res.entrypoint == "run");

let res = CallArguments::from_str("0x1234,run,1,2,3").unwrap();
assert!(res.tag_or_address == ResourceDescriptor::from_string("0x1234").unwrap());
assert!(res.entrypoint == "run");
assert!(res.calldata == vec![Felt::ONE, Felt::TWO, Felt::THREE]);

// missing entry point
let res = CallArguments::from_str("0x1234");
assert!(res.is_err());

// bad tag_or_address format
let res = CallArguments::from_str("0x12X4,run");
assert!(res.is_err());
}
}
2 changes: 1 addition & 1 deletion crates/sozo/ops/src/resource_descriptor.rs
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