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

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
36 changes: 24 additions & 12 deletions bin/sozo/src/commands/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,11 @@ async fn clone_permissions(
.external_writers
.iter()
.filter_map(|(resource_selector, writers)| {
if writers.contains(&from_address) { Some(*resource_selector) } else { None }
if writers.contains(&from_address) {
Some(*resource_selector)
} else {
None
}
})
.collect();

Expand All @@ -251,7 +255,11 @@ async fn clone_permissions(
.external_owners
.iter()
.filter_map(|(resource_selector, owners)| {
if owners.contains(&from_address) { Some(*resource_selector) } else { None }
if owners.contains(&from_address) {
Some(*resource_selector)
} else {
None
}
})
.collect();

Expand Down Expand Up @@ -303,16 +311,20 @@ async fn clone_permissions(
writers_resource_selectors.extend(external_writer_of.iter().copied());
owners_resource_selectors.extend(external_owner_of.iter().copied());

writer_of.extend(
external_writer_of
.iter()
.map(|r| if r != &WORLD { format!("{:#066x}", r) } else { "World".to_string() }),
);
owner_of.extend(
external_owner_of
.iter()
.map(|r| if r != &WORLD { format!("{:#066x}", r) } else { "World".to_string() }),
);
writer_of.extend(external_writer_of.iter().map(|r| {
if r != &WORLD {
format!("{:#066x}", r)
} else {
"World".to_string()
}
}));
owner_of.extend(external_owner_of.iter().map(|r| {
if r != &WORLD {
format!("{:#066x}", r)
} else {
"World".to_string()
}
}));

// Sort the tags to have a deterministic output.
let mut writer_of = writer_of.into_iter().collect::<Vec<_>>();
Expand Down
6 changes: 4 additions & 2 deletions bin/sozo/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ pub enum Commands {
Build(Box<BuildArgs>),
#[command(about = "Build and migrate the world every time a file changes")]
Dev(Box<DevArgs>),
#[command(about = "Run a migration, declaring and deploying contracts as necessary to update \
the world")]
#[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.")]
Execute(Box<ExecuteArgs>),
Expand Down
81 changes: 80 additions & 1 deletion bin/sozo/src/commands/model.rs
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
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