-
Notifications
You must be signed in to change notification settings - Fork 189
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
69d83d0
f34a85e
2df1d0e
fee9cfb
2b0dac4
183ae33
efe1905
7d3d719
018847f
47df4b2
cfa44f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
use anyhow::Result; | ||
use clap::{Args, Subcommand}; | ||
use clap::{value_parser, Args, Subcommand}; | ||
use dojo_world::config::calldata_decoder; | ||
use scarb::core::Config; | ||
use sozo_ops::model; | ||
use sozo_ops::resource_descriptor::ResourceDescriptor; | ||
|
@@ -109,7 +110,11 @@ 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)] | ||
|
@@ -119,11 +124,22 @@ hashes, called 'hash' in the following documentation. | |
starknet: StarknetOptions, | ||
|
||
#[arg(short, long)] | ||
#[arg(help = "Block number at which to retrieve the model data (pending block by default)")] | ||
#[arg( | ||
help = "Block number at which to retrieve the model data (pending block by default)" | ||
)] | ||
block: Option<u64>, | ||
}, | ||
} | ||
|
||
// 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)?; | ||
Ok(felts[0]) | ||
} | ||
|
||
impl ModelArgs { | ||
pub fn run(self, config: &Config) -> Result<()> { | ||
trace!(args = ?self); | ||
|
@@ -222,3 +238,66 @@ impl ModelArgs { | |
}) | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 You could write a test on the argument though. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's actually good having those here for the command, thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My pleasure, sir. |
||
use super::*; | ||
use clap::Parser; | ||
use starknet::core::utils::cairo_short_string_to_felt; | ||
|
||
#[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", // Placeholder for the binary name | ||
"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()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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` There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)?, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohayo, sensei! Prevent potential index out-of-bounds panic
The code assumes that
felts
contains at least one element. Ifdecode_calldata(s)
returns an empty vector, accessingfelts[0]
will cause a panic. Consider adding a check to ensurefelts
is not empty before accessing the first element.Apply this diff to handle the potential empty
felts
:📝 Committable suggestion