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 @@ -44,8 +44,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 a system with the given calldata.")]
Execute(Box<ExecuteArgs>),
Expand Down
46 changes: 43 additions & 3 deletions 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,8 +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,...")]
keys: Vec<Felt>,
#[arg(help = "Comma separated values e.g., \
0x12345,0x69420,sstr:\"hello\",sstr:\"misty\". Supported prefixes:\n \
- sstr: A cairo short string\n \
- no prefix: A cairo felt")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the case of the keys, only one felt long serialized type are supported (everything that can fit in one felt).

That's great you've only put the sstr which is the only one supported actually.

Suggested change
#[arg(help = "Comma separated values e.g., \
0x12345,0x69420,sstr:\"hello\",sstr:\"misty\". Supported prefixes:\n \
- sstr: A cairo short string\n \
- no prefix: A cairo felt")]
#[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")]

Copy link
Collaborator

Choose a reason for hiding this comment

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

We may add a check that only this prefix is actually used, wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the check will be quite useful. I'll add it.

keys: String,

#[command(flatten)]
world: WorldOptions,
Expand All @@ -119,7 +123,9 @@ 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>,
},
}
Expand Down Expand Up @@ -205,6 +211,8 @@ impl ModelArgs {
let (world_diff, provider, _) =
utils::get_world_diff_and_provider(starknet, world, &ws).await?;

let keys = calldata_decoder::decode_calldata(&keys)?;

let (record, _, _) = model::model_get(
tag.to_string(),
keys,
Expand All @@ -222,3 +230,35 @@ 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 super::*;
use starknet::core::utils::cairo_short_string_to_felt;

#[test]
fn test_short_string_equals_felt() {
// Test that sstr:"misty" equals 0x6d69737479
let with_prefix = "sstr:\"misty\"";
let with_hex = "0x6d69737479";

let felt_from_string = calldata_decoder::decode_calldata(with_prefix).unwrap();
let felt_from_hex = calldata_decoder::decode_calldata(with_hex).unwrap();

assert_eq!(felt_from_string, felt_from_hex);
assert_eq!(felt_from_string[0], Felt::from_hex_str("0x6d69737479").unwrap());
}

#[test]
fn test_hex_equals_decimal() {
// Test that 0x6d69737479 equals 469920609401
let with_hex = "0x6d69737479";
let with_decimal = "469920609401";

let felt_from_hex = calldata_decoder::decode_calldata(with_hex).unwrap();
let felt_from_decimal = calldata_decoder::decode_calldata(with_decimal).unwrap();

assert_eq!(felt_from_hex, felt_from_decimal);
assert_eq!(felt_from_hex[0], Felt::from(469920609401u128));
}
}
Loading