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

fix(sozo): model get not using prefixes #2867

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

fix(sozo): model get not using prefixes #2867

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
69d83d0
fix(sozo): model get not using prefixes
emarc99 Jan 6, 2025
f34a85e
rewrote model get key parser
emarc99 Jan 16, 2025
2df1d0e
Merge branch 'dojoengine:main' into fix-sozo-model-prefix
emarc99 Jan 16, 2025
fee9cfb
Merge branch 'fix-sozo-model-prefix' of https://github.com/emarc99/do…
emarc99 Jan 16, 2025
2b0dac4
handle the potential empty felts
emarc99 Jan 16, 2025
183ae33
rust fmt
emarc99 Jan 17, 2025
efe1905
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 17, 2025
7d3d719
Merge branch 'fix-sozo-model-prefix' of https://github.com/emarc99/do…
emarc99 Jan 17, 2025
018847f
rust fmt
emarc99 Jan 17, 2025
47df4b2
rust fmt
emarc99 Jan 17, 2025
cfa44f4
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 17, 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
69 changes: 20 additions & 49 deletions bin/sozo/src/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use auth::AuthArgs;
use clap::Subcommand;
use events::EventsArgs;
use scarb::core::{Config, Package, Workspace};
use semver::{Version, VersionReq};
use tracing::info_span;

pub(crate) mod auth;
Expand Down Expand Up @@ -33,8 +32,6 @@ use init::InitArgs;
use inspect::InspectArgs;
use migrate::MigrateArgs;
use model::ModelArgs;
#[cfg(feature = "walnut")]
use sozo_walnut::walnut::WalnutArgs;
use test::TestArgs;

pub(crate) const LOG_TARGET: &str = "sozo::cli";
Expand All @@ -50,7 +47,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 one or several systems with the given calldata.")]
#[command(about = "Execute a system with the given calldata.")]
Execute(Box<ExecuteArgs>),
#[command(about = "Inspect the world")]
Inspect(Box<InspectArgs>),
Expand All @@ -68,9 +65,6 @@ pub enum Commands {
Model(Box<ModelArgs>),
#[command(about = "Inspect events emitted by the world")]
Events(Box<EventsArgs>),
#[cfg(feature = "walnut")]
#[command(about = "Interact with walnut.dev - transactions debugger and simulator")]
Walnut(Box<WalnutArgs>),
}

impl fmt::Display for Commands {
Expand All @@ -89,8 +83,6 @@ impl fmt::Display for Commands {
Commands::Init(_) => write!(f, "Init"),
Commands::Model(_) => write!(f, "Model"),
Commands::Events(_) => write!(f, "Events"),
#[cfg(feature = "walnut")]
Commands::Walnut(_) => write!(f, "WalnutVerify"),
}
}
}
Expand All @@ -113,12 +105,10 @@ pub fn run(command: Commands, config: &Config) -> Result<()> {
Commands::Clean(args) => args.run(config),
Commands::Call(args) => args.run(config),
Commands::Test(args) => args.run(config),
Commands::Hash(args) => args.run(config).map(|_| ()),
Commands::Hash(args) => args.run().map(|_| ()),
Commands::Init(args) => args.run(config),
Commands::Model(args) => args.run(config),
Commands::Events(args) => args.run(config),
#[cfg(feature = "walnut")]
Commands::Walnut(args) => args.run(config),
}
}

Expand All @@ -139,43 +129,24 @@ pub fn check_package_dojo_version(ws: &Workspace<'_>, package: &Package) -> anyh
&& dojo_dep_str.contains("tag=v")
&& !dojo_dep_str.contains(dojo_version)
{
// safe to unwrap since we know the string contains "tag=v".
// "dojo * (git+https://github.com/dojoengine/dojo?tag=v1.0.10)"
let dojo_dep_version = dojo_dep_str.split("tag=v")
.nth(1) // Get the part after "tag=v"
.map(|s| s.trim_end_matches(')'))
.expect("Unexpected dojo dependency format");

let dojo_dep_version = Version::parse(dojo_dep_version).unwrap();

let version_parts: Vec<&str> = dojo_version.split('.').collect();
let major_minor = format!("{}.{}", version_parts[0], version_parts[1]);
let dojo_req_version = VersionReq::parse(&format!(">={}", major_minor)).unwrap();

if !dojo_req_version.matches(&dojo_dep_version) {
if let Ok(cp) = ws.current_package() {
// Selected package.
let path = if cp.id == package.id {
package.manifest_path()
} else {
ws.manifest_path()
};

anyhow::bail!(
"Found dojo-core version mismatch: expected {}. Please verify your dojo \
dependency in {}",
dojo_req_version,
path
)
} else {
// Virtual workspace.
anyhow::bail!(
"Found dojo-core version mismatch: expected {}. Please verify your dojo \
dependency in {}",
dojo_req_version,
ws.manifest_path()
)
}
if let Ok(cp) = ws.current_package() {
let path =
if cp.id == package.id { package.manifest_path() } else { ws.manifest_path() };

anyhow::bail!(
"Found dojo-core version mismatch: expected {}. Please verify your dojo \
dependency in {}",
dojo_version,
path
)
} else {
// Virtual workspace.
anyhow::bail!(
"Found dojo-core version mismatch: expected {}. Please verify your dojo \
dependency in {}",
dojo_version,
ws.manifest_path()
)
}
}
}
Expand Down
81 changes: 80 additions & 1 deletion bin/sozo/src/commands/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use anyhow::Result;
use clap::{Args, Subcommand};
use dojo_world::config::calldata_decoder;
use scarb::core::Config;
use sozo_ops::model;
use sozo_ops::resource_descriptor::ResourceDescriptor;
Expand Down Expand Up @@ -109,7 +110,9 @@ hashes, called 'hash' in the following documentation.

#[arg(value_name = "KEYS")]
#[arg(value_delimiter = ',')]
#[arg(help = "Comma seperated values e.g., 0x12345,0x69420,...")]
#[arg(help = "Comma separated values e.g., 0x12345,0x69420,sstr:\"hello\". Supported \
prefixes:\n - sstr: A cairo short string\n - no prefix: A cairo felt")]
#[arg(value_parser = model_key_parser)]
keys: Vec<Felt>,

#[command(flatten)]
Expand All @@ -124,6 +127,18 @@ hashes, called 'hash' in the following documentation.
},
}

// Custom parser for model keys
fn model_key_parser(s: &str) -> Result<Felt> {
if s.contains(':') && !s.starts_with("sstr:") {
anyhow::bail!("Only 'sstr:' prefix is supported for model keys");
}
let felts = calldata_decoder::decode_calldata(s)?;
if felts.is_empty() {
anyhow::bail!("Failed to parse key '{}': no values returned.", s);
}
Ok(felts[0])
}

impl ModelArgs {
pub fn run(self, config: &Config) -> Result<()> {
trace!(args = ?self);
Expand Down Expand Up @@ -222,3 +237,67 @@ impl ModelArgs {
})
}
}

#[cfg(test)]
mod tests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for thinking about tests.

In the current context, this test should already be covered into the calldata_decoder file. We can remove this one. 👍

You could write a test on the argument though.

Copy link
Author

Choose a reason for hiding this comment

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

Awesome, that'll be the better option. These tests were meant to go away eventually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's actually good having those here for the command, thank you!

Copy link
Author

Choose a reason for hiding this comment

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

My pleasure, sir.

use clap::Parser;
use starknet::core::utils::cairo_short_string_to_felt;

use super::*;

#[derive(Parser, Debug)]
struct TestCommand {
#[command(subcommand)]
command: ModelCommand,
}

#[test]
fn test_model_get_argument_parsing() {
// Test parsing with hex
let args = TestCommand::parse_from([
"model",
"get",
"Account",
"0x054cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5,0x6d69737479",
]);

if let ModelCommand::Get { keys, .. } = args.command {
let expected = vec![
Felt::from_hex(
"0x054cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5",
)
.unwrap(),
Felt::from_hex("0x6d69737479").unwrap(),
];
assert_eq!(keys, expected);
} else {
panic!("Expected Get command");
}

// Test parsing with string prefix
let args = TestCommand::parse_from([
"model",
"get",
"Account",
"0x054cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5,sstr:\"misty\"",
]);

if let ModelCommand::Get { keys, .. } = args.command {
let expected = vec![
Felt::from_hex(
"0x054cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5",
)
.unwrap(),
cairo_short_string_to_felt("misty").unwrap(),
];
assert_eq!(keys, expected);
} else {
panic!("Expected Get command");
}

// Test invalid prefix
let result =
TestCommand::try_parse_from(["model", "get", "Account", "0x123,str:\"hello\""]);
assert!(result.is_err());
}
}
9 changes: 8 additions & 1 deletion 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 @@ -161,7 +161,14 @@ pub fn decode_single_calldata(item: &str) -> DecoderResult<Vec<Felt>> {
match prefix {
"u256" => U256CalldataDecoder.decode(value)?,
"str" => StrCalldataDecoder.decode(value)?,
"sstr" => ShortStrCalldataDecoder.decode(value)?,
"sstr" => {
let value = if value.starts_with('"') && value.ends_with('"') {
value.trim_matches('"')
} else {
value
};
ShortStrCalldataDecoder.decode(value)?
Comment on lines +165 to +170
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't the quotes already taken in account? Would you mind explaining the choice here.
Maybe this should land in decode function in case we have some?

Copy link
Author

Choose a reason for hiding this comment

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

Second part of test kept failing with below error;

running 1 test
test commands::model::tests::test_model_get_argument_parsing ... FAILED

failures:

---- commands::model::tests::test_model_get_argument_parsing stdout ----
thread 'commands::model::tests::test_model_get_argument_parsing' panicked at bin/sozo/src/commands/model.rs:296:13:assertion `left == right` failed
  left: [0x54cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5, 0x226d6973747922]
 right: [0x54cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5, 0x6d69737479]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    commands::model::tests::test_model_get_argument_parsing

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 51 filtered out; finished in 1.30s

error: test failed, to rerun pass `--bin sozo`

Copy link
Author

Choose a reason for hiding this comment

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

The issue wanted below to work.

sozo --profile sepolia model get Account 0x054cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5,sstr:"misty"

Maybe what was intended was below, without quotes around the short string; if so I'll remove the check for trimming quotes.

sozo --profile sepolia model get Account 0x054cb935d86d80b5a0a6e756edf448ab33876d01dd2b07a2a4e63a41e06d0ef5,sstr:misty

}
"int" => SignedIntegerCalldataDecoder.decode(value)?,
_ => DefaultCalldataDecoder.decode(item)?,
}
Expand Down