fix(sozo): model get not using prefixes #2867
Conversation
WalkthroughOhayo, sensei! The pull request introduces enhancements to the Changes
Assessment against linked issues
The changes directly address the requirements outlined in issue #2835, implementing prefix parsing and decoding for the 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.
| #[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.
|
Don't forget to run the |
Noted, 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
Documentation
Tests