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

Merged
merged 27 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
27 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
70a8da2
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 23, 2025
0af4d74
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 25, 2025
860c6c0
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 29, 2025
4fff908
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 29, 2025
ef3144b
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 30, 2025
597ba88
add support for all prfixes
emarc99 Jan 30, 2025
42a8b53
Merge branch 'fix-sozo-model-prefix' of https://github.com/emarc99/do…
emarc99 Jan 30, 2025
0cfa683
updated help text of model get keys to show supported prefixes
emarc99 Jan 30, 2025
d6d84fe
Merge branch 'main' into fix-sozo-model-prefix
emarc99 Jan 30, 2025
aa29464
remove trimming of quotes
emarc99 Jan 30, 2025
c0141d0
Merge branch 'fix-sozo-model-prefix' of https://github.com/emarc99/do…
emarc99 Jan 30, 2025
265cd5c
Merge remote-tracking branch 'dojo/main' into fix-sozo-model-prefix
glihm Feb 5, 2025
039ad49
refactor to use new calldata
glihm Feb 5, 2025
10d9419
add tests for quotes management in calldata decoder
glihm Feb 5, 2025
02ee540
Merge remote-tracking branch 'dojo/main' into fix-sozo-model-prefix
glihm Feb 5, 2025
acaea86
update test db and policies
glihm Feb 5, 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
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 {
glihm marked this conversation as resolved.
Show resolved Hide resolved
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)?
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
Contributor 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
Contributor 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

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need the " to make sure strings with spaces are actually included. So sstr:bla bla is not supported without ". So it is important to have them.

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