fix(sozo): model get not using prefixes #2867
Conversation
WalkthroughThe pull request introduces improvements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Ohayo, sensei! I've reviewed the changes and they look like a solid solution to the prefix parsing challenge for the 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bin/sozo/src/commands/model.rs (2)
113-116: Ohayo sensei! Nice usage of multi-line help docs.
This enhanced help text clearly explains how to supply comma-separated values with various prefixes. The additional detail helps users form correct input on the command line.
214-215: Sensei, consider robust error handling.
You raise an error ifdecode_calldatafails. That's good. If you anticipate user mistakes frequently, you might provide a more descriptive error or usage example upon failure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/sozo/src/commands/auth.rs(3 hunks)bin/sozo/src/commands/mod.rs(1 hunks)bin/sozo/src/commands/model.rs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- bin/sozo/src/commands/mod.rs
🔇 Additional comments (8)
bin/sozo/src/commands/model.rs (4)
3-3: Ohayo sensei! Great addition for the decoder import.
The import ofcalldata_decodersets the foundation for decoding user input with flexible prefixes. This is useful for supporting short-string notation and conventional felts.
117-117: Sensei, verify the impact of switching from Vec to String.
Swapping fromVec<Felt>toStringis beneficial for user-friendly input, but ensure that usage sites (including tests and other commands) are updated accordingly.
126-128: Ohayo sensei! Thorough documentation for the block argument.
No issues spotted; the docstring clarifies the usage of a block number or a pending block. Looks good to merge.
233-264: Ohayo sensei! Impressive test coverage for short strings and hex validation.
Testing both short-string prefixes and hex-to-decimal conversions is crucial. The tests confirm that your new input format is working as intended.bin/sozo/src/commands/auth.rs (4)
245-249: Sensei, good clarity in the resource filtering condition.
The explicitifchecks improve readability compared to a one-liner.
258-262: Ohayo sensei! Consistent approach for filtering owners.
Following the same pattern for owners and writers helps maintain a uniform code style.
314-320: Sensei, verifying name resolution for external writers.
You map resource selectors to hex strings or “World” if they matchWORLD. This is clear. Make sure there’s no confusion about which resources are truly global.
321-327: Ohayo sensei! Good consistency in naming for external owners.
Repeating the same logic for owners ensures symmetrical permission logic for resource tagging.
bin/sozo/src/commands/model.rs
Outdated
| #[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")] |
There was a problem hiding this comment.
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.
| #[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")] |
There was a problem hiding this comment.
We may add a check that only this prefix is actually used, wdyt?
There was a problem hiding this comment.
Yeah, the check will be quite useful. I'll add it.
| @@ -222,3 +230,35 @@ impl ModelArgs { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| mod tests { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Awesome, that'll be the better option. These tests were meant to go away eventually.
There was a problem hiding this comment.
That's actually good having those here for the command, thank you!
|
Don't forget to run the |
Noted, sir. |
Hey @emarc99, let me know if you have any blocker on that. |
The script |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bin/sozo/src/commands/model.rs (2)
113-117: Ohayo, sensei! Clarify the help message formattingThe help message for the
keysargument could be formatted for better readability in the command-line interface. Adjusting the indentation and line breaks will make the supported prefixes clearer to the user.Apply this diff to improve the formatting:
#[arg(value_name = "KEYS")] #[arg(value_delimiter = ',')] - #[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(help = "Comma separated values e.g., 0x12345,0x69420,sstr:\"hello\".\nSupported prefixes:\n - sstr: A Cairo short string\n - no prefix: A Cairo felt")]
136-137: Ohayo, sensei! Enhance error message for unsupported prefixesIf a user provides an unsupported prefix, the error message could be more informative. Including the invalid prefix in the message and listing supported prefixes will guide the user towards correct usage.
Apply this diff to improve the error message:
if s.contains(':') && !s.starts_with("sstr:") { - anyhow::bail!("Only 'sstr:' prefix is supported for model keys"); + anyhow::bail!("Unsupported prefix in key '{}'. Only 'sstr:' prefix is supported for model keys.", s); }crates/dojo/world/src/config/calldata_decoder.rs (1)
164-171: Ohayo, sensei! Handle mismatched quotes insstrdecodingWhen decoding
sstrprefixed values, the current implementation trims quotes if they are present but doesn't validate if they are correctly paired. This could lead to unexpected behavior with mismatched or unpaired quotes. Consider adding validation to check for properly matched quotes and provide an informative error message if they are not.Apply this diff to add quote validation:
"sstr" => { let value = if value.starts_with('"') && value.ends_with('"') { value.trim_matches('"') + } else if value.starts_with('"') || value.ends_with('"') { + return Err(CalldataDecoderError::ParseError("Mismatched quotes in 'sstr' value.".to_string())); } else { value }; ShortStrCalldataDecoder.decode(value)? },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/sozo/src/commands/mod.rs(1 hunks)bin/sozo/src/commands/model.rs(4 hunks)crates/dojo/world/src/config/calldata_decoder.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/sozo/src/commands/mod.rs
🔇 Additional comments (1)
bin/sozo/src/commands/model.rs (1)
243-303: Ohayo, sensei! Excellent addition of argument parsing testsGreat job adding tests for the
Getcommand's argument parsing. The tests cover both hexadecimal andsstrprefixed inputs, ensuring robust handling of various input formats.
bin/sozo/src/commands/model.rs
Outdated
| let felts = calldata_decoder::decode_calldata(s)?; | ||
| Ok(felts[0]) | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Prevent potential index out-of-bounds panic
The code assumes that felts contains at least one element. If decode_calldata(s) returns an empty vector, accessing felts[0] will cause a panic. Consider adding a check to ensure felts is not empty before accessing the first element.
Apply this diff to handle the potential empty felts:
let felts = calldata_decoder::decode_calldata(s)?;
+ if felts.is_empty() {
+ anyhow::bail!("Failed to parse key '{}': no values returned.", s);
+ }
Ok(felts[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let felts = calldata_decoder::decode_calldata(s)?; | |
| Ok(felts[0]) | |
| } | |
| let felts = calldata_decoder::decode_calldata(s)?; | |
| if felts.is_empty() { | |
| anyhow::bail!("Failed to parse key '{}': no values returned.", s); | |
| } | |
| Ok(felts[0]) | |
| } |
| let value = if value.starts_with('"') && value.ends_with('"') { | ||
| value.trim_matches('"') | ||
| } else { | ||
| value | ||
| }; | ||
| ShortStrCalldataDecoder.decode(value)? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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| @@ -222,3 +230,35 @@ impl ModelArgs { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| mod tests { | |||
There was a problem hiding this comment.
That's actually good having those here for the command, thank you!
There's progress with this command. Will apply it soon and pr. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bin/sozo/src/commands/model.rs (3)
113-115: Ohayo! Consider improving help text readabilityThe help text could be more readable with consistent indentation and alignment:
- #[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(help = "Comma separated values e.g., 0x12345,0x69420,sstr:\"hello\".\n\ + Supported prefixes:\n\ + - sstr: A cairo short string\n\ + - no prefix: A cairo felt")]
130-140: Ohayo! Consider additional input validationThe parser handles prefix validation and empty results well. Consider adding validation for malformed inputs:
fn model_key_parser(s: &str) -> Result<Felt> { + if s.trim().is_empty() { + anyhow::bail!("Empty key is not allowed"); + } 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]) }
241-303: Ohayo! Consider adding more edge case testsThe test coverage is good, but could be enhanced with additional edge cases:
#[test] fn test_model_get_edge_cases() { // Test empty input let result = TestCommand::try_parse_from(["model", "get", "Account", ""]); assert!(result.is_err()); // Test malformed sstr (missing quotes) let result = TestCommand::try_parse_from(["model", "get", "Account", "sstr:hello"]); assert!(result.is_err()); // Test malformed hex let result = TestCommand::try_parse_from(["model", "get", "Account", "0xZZZ"]); assert!(result.is_err()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/sozo/src/commands/model.rs(4 hunks)crates/dojo/world/src/config/calldata_decoder.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/world/src/config/calldata_decoder.rs
🔇 Additional comments (1)
bin/sozo/src/commands/model.rs (1)
3-3: Ohayo, sensei! Import looks good!The
calldata_decoderimport is correctly added and aligns with the new key parsing functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/sozo/src/commands/mod.rs (1)
132-149: Clean error handling improvement, sensei!The version check error handling is now more precise about manifest locations and handles both regular and virtual workspaces consistently. Nice work!
Consider making the error messages even more user-friendly by adding a hint about how to fix the version mismatch:
- "Found dojo-core version mismatch: expected {}. Please verify your dojo \ - dependency in {}", + "Found dojo-core version mismatch: expected {}. Please update your dojo \ + dependency to the expected version in {} (hint: update the tag in your dependency)",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/mod.rs(3 hunks)
🔇 Additional comments (2)
bin/sozo/src/commands/mod.rs (2)
108-108: Verify Hash command's config independence, sensei!The Hash command no longer uses the config parameter. This change looks intentional and aligns with the testing comment, but let's verify that the Hash command implementation doesn't require any config parameters.
50-51: Ohayo! Verify if Execute command functionality has changed.The documentation change from "Execute one or several systems" to "Execute a system" suggests a breaking change in functionality. Let's verify if this reflects an actual implementation change.
There was a problem hiding this comment.
Hey @emarc99, thank you for your patience and sorry for the delay.
So lately we have relaxed the keys constraint, so anything can be a key (if it's serializable). Would you mind updating the PR to support all the prefixes? :)
Also, the quotes shouldn't be an issue, but let me know how it goes.
Alright, I'll update the PR accordingly, sir. |
Thank you, let me know if you have any questions. 👍 |
|
All good @emarc99 or do you still need some feedback? Let me know if you're blocked. |
Yea, I am running tests. I'll update in less than 24 hours for some feedback if tests keep failing, sir. |
Description
Related issue
Closes #2835
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
sstr:prefixBug Fixes
Improvements