fix(sozo): model get not using prefixes #2867
Conversation
WalkthroughOhayo, sensei! The pull request enhances the Changes
Assessment against linked issues
Tip 🌐 Web search-backed reviews and chat
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🔇 Additional comments (33)
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.
|
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:mistyThere was a problem hiding this comment.
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.
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.
|
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. |
|
Quick question sir @glihm. I know the magic is happening in |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bin/sozo/src/commands/model.rs (1)
238-377: Ohayo, sensei! Enhance test coverageWhile the current tests are thorough for happy paths, consider adding these test cases:
- Error cases for invalid prefixes
- Empty input handling
- Flattening edge cases (e.g., nested arrays)
#[test] fn test_model_get_error_cases() { // Test invalid prefix let result = TestCommand::try_parse_from([ "model", "get", "Account", "invalid:value" ]); assert!(result.is_err()); // Test empty input let result = TestCommand::try_parse_from([ "model", "get", "Account", "" ]); assert!(result.is_err()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/model.rs(5 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/model.rs (1)
217-221: Ohayo, sensei! Clean and efficient implementationThe key flattening implementation is elegant and uses the appropriate iterator methods.
bin/sozo/src/commands/model.rs
Outdated
| // Custom parser for model keys | ||
| fn model_key_parser(s: &str) -> Result<Vec<Felt>> { | ||
| let felts = calldata_decoder::decode_calldata(&vec![s.to_string()])?; | ||
| Ok(felts) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Add validation and error handling
The parser implementation could be enhanced with:
- Empty result handling to prevent index out-of-bounds panic
- Prefix validation to ensure only supported prefixes are used
fn model_key_parser(s: &str) -> Result<Vec<Felt>> {
let felts = calldata_decoder::decode_calldata(&vec![s.to_string()])?;
+ if felts.is_empty() {
+ anyhow::bail!("Failed to parse key '{}': no values returned", s);
+ }
+
+ // Validate prefix
+ if s.contains(':') {
+ let prefix = s.split(':').next().unwrap();
+ if !["sstr"].contains(&prefix) {
+ anyhow::bail!("Unsupported prefix '{}' in key '{}'", prefix, s);
+ }
+ }
Ok(felts)
}📝 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.
| // Custom parser for model keys | |
| fn model_key_parser(s: &str) -> Result<Vec<Felt>> { | |
| let felts = calldata_decoder::decode_calldata(&vec![s.to_string()])?; | |
| Ok(felts) | |
| } | |
| fn model_key_parser(s: &str) -> Result<Vec<Felt>> { | |
| let felts = calldata_decoder::decode_calldata(&vec![s.to_string()])?; | |
| if felts.is_empty() { | |
| anyhow::bail!("Failed to parse key '{}': no values returned", s); | |
| } | |
| // Validate prefix | |
| if s.contains(':') { | |
| let prefix = s.split(':').next().unwrap(); | |
| if !["sstr"].contains(&prefix) { | |
| anyhow::bail!("Unsupported prefix '{}' in key '{}'", prefix, s); | |
| } | |
| } | |
| Ok(felts) | |
| } |
bin/sozo/src/commands/model.rs
Outdated
| #[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<Vec<Felt>>, |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! Help text needs updating
The help text for the keys argument only shows a subset of supported prefixes. The codebase fully supports:
- u256: 256-bit unsigned integer
- sstr: Cairo short string
- str: Cairo string (ByteArray)
- no prefix: Cairo felt or any type that fits into one felt
🔗 Analysis chain
Ohayo, sensei! Verify prefix support scope
Based on past discussions, only felt-long serialized types were supported. The help text now suggests support for multiple prefixes. Please confirm if this aligns with the current design decisions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any size constraints or prefix validations in the codebase
# Search for prefix validation logic
rg -A 5 'prefix|serialize' src/
Length of output: 78
Script:
#!/bin/bash
# Search for model key parser and related code
rg -A 10 "model_key_parser|fn parse" bin/sozo/src/
# Search for prefix validation and serialization logic
rg -A 5 "prefix|serialize" bin/sozo/src/
# Look for any type constraints or validation
rg -A 5 "Felt|felt|short string" bin/sozo/src/commands/
Length of output: 61730
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bin/sozo/src/commands/model.rs (1)
133-137: 🛠️ Refactor suggestionOhayo, sensei! Add validation for empty results and prefix support.
The parser implementation should be enhanced with validation to prevent potential issues.
fn model_key_parser(s: &str) -> Result<Vec<Felt>> { let felts = calldata_decoder::decode_calldata(&vec![s.to_string()])?; + if felts.is_empty() { + anyhow::bail!("Failed to parse key '{}': no values returned", s); + } Ok(felts) }
🧹 Nitpick comments (2)
bin/sozo/src/commands/model.rs (2)
220-224: Ohayo, sensei! Consider adding debug logging for key flattening.Adding debug logging would help with troubleshooting key parsing issues.
+ trace!(?keys, "Original keys before flattening"); let flattened_keys: Vec<Felt> = keys.into_iter().flatten().collect(); + trace!(?flattened_keys, "Flattened keys");
241-381: Ohayo, sensei! Comprehensive test coverage looks good!The test suite thoroughly covers various input scenarios including hex values, short strings, u256, int, and str prefixes. Great job on including edge cases and multiple prefix combinations!
However, consider adding negative test cases:
- Empty input
- Invalid prefixes
- Malformed input
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/model.rs(5 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/model.rs (1)
113-119: Ohayo, sensei! Help text needs updating based on recent changes.Based on the PR comments, glihm mentioned that any serializable item can now be a key. The help text should be updated to reflect this change.
- #[arg(help = "Comma separated values, e.g., 0x12345,0x69420,sstr:\"hello\", u256:0x123. - Supporting all prefixes:\n - u256: A 256-bit unsigned integer\n - str: A \ - cairo string (ByteArray)\n - sstr: A cairo short string\n - int: A \ - signed integer\n - no prefix: A cairo felt or any type that fits into \ - one felt")] + #[arg(help = "Comma separated values, e.g., 0x12345,0x69420,sstr:\"hello\", u256:0x123. + Supporting any serializable type with prefixes:\n - u256: A 256-bit unsigned integer\n - str: A \ + cairo string (ByteArray)\n - sstr: A cairo short string\n - int: A \ + signed integer\n - no prefix: A cairo felt\n - <type>: Any serializable type")]
Yes with latest changes keys are not limited. So all prefixes are supported. The important point is having the he correct serialization, which is handled by the prefixes. 👍 |
| let value = if value.starts_with('"') && value.ends_with('"') { | ||
| value.trim_matches('"') | ||
| } else { | ||
| value | ||
| }; | ||
| ShortStrCalldataDecoder.decode(value)? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/sozo/src/commands/model.rs (1)
246-249: Ohayo, sensei! Address the TODO comment about key flattening tests.The test suite is comprehensive, but we should implement the missing tests for key flattening.
Would you like me to help implement the additional test cases for key flattening?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/sozo/src/commands/model.rs(5 hunks)crates/dojo/world/src/config/calldata_decoder.rs(2 hunks)examples/spawn-and-move/manifest_dev.json(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/world/src/config/calldata_decoder.rs
🔇 Additional comments (3)
bin/sozo/src/commands/model.rs (2)
1-14: Ohayo, sensei! The imports look good!The added imports for
calldata_decoderandCALLDATA_DOCalign well with the new key parsing functionality.
113-118: Ohayo, sensei! The Get command argument changes look good!The change from
Vec<Felt>toVec<String>improves usability by allowing more flexible input formats, and the help text now properly references the calldata documentation.examples/spawn-and-move/manifest_dev.json (1)
3-4: Ohayo, sensei! The manifest updates look good!The changes to class hashes and addresses are consistent throughout the manifest, and the new models (Flatbow and RiverSkale) have been properly integrated.
Also applies to: 1255-1255, 1609-1609, 1803-1803, 1979-1979, 2165-2168, 2177-2177, 2183-2183, 2189-2189, 2195-2195, 2201-2208, 2215-2215, 2221-2221
| /// Parses the keys from the command line into a vector of Felt representing the serialized keys of | ||
| /// the model. | ||
| fn parse_keys(keys: &[String]) -> Result<Vec<Felt>> { | ||
| let mut keys_serde = vec![]; | ||
|
|
||
| for key in keys { | ||
| let key_felt = calldata_decoder::decode_single_calldata(key) | ||
| .with_context(|| format!("Failed to decode key: {}", key))?; | ||
| keys_serde.extend(key_felt); | ||
| } | ||
|
|
||
| Ok(keys_serde) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Add validation to the parse_keys function.
The function should validate empty results and supported prefixes to prevent runtime errors.
Apply this diff to add validation:
fn parse_keys(keys: &[String]) -> Result<Vec<Felt>> {
let mut keys_serde = vec![];
for key in keys {
let key_felt = calldata_decoder::decode_single_calldata(key)
.with_context(|| format!("Failed to decode key: {}", key))?;
+ if key_felt.is_empty() {
+ anyhow::bail!("Failed to parse key '{}': no values returned", key);
+ }
+
+ // Validate prefix if present
+ if key.contains(':') {
+ let prefix = key.split(':').next().unwrap();
+ if !["u256", "sstr", "str", "int"].contains(&prefix) {
+ anyhow::bail!("Unsupported prefix '{}' in key '{}'", prefix, key);
+ }
+ }
keys_serde.extend(key_felt);
}
Ok(keys_serde)
}📝 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.
| /// Parses the keys from the command line into a vector of Felt representing the serialized keys of | |
| /// the model. | |
| fn parse_keys(keys: &[String]) -> Result<Vec<Felt>> { | |
| let mut keys_serde = vec![]; | |
| for key in keys { | |
| let key_felt = calldata_decoder::decode_single_calldata(key) | |
| .with_context(|| format!("Failed to decode key: {}", key))?; | |
| keys_serde.extend(key_felt); | |
| } | |
| Ok(keys_serde) | |
| } | |
| fn parse_keys(keys: &[String]) -> Result<Vec<Felt>> { | |
| let mut keys_serde = vec![]; | |
| for key in keys { | |
| let key_felt = calldata_decoder::decode_single_calldata(key) | |
| .with_context(|| format!("Failed to decode key: {}", key))?; | |
| if key_felt.is_empty() { | |
| anyhow::bail!("Failed to parse key '{}': no values returned", key); | |
| } | |
| // Validate prefix if present | |
| if key.contains(':') { | |
| let prefix = key.split(':').next().unwrap(); | |
| if !["u256", "sstr", "str", "int"].contains(&prefix) { | |
| anyhow::bail!("Unsupported prefix '{}' in key '{}'", prefix, key); | |
| } | |
| } | |
| keys_serde.extend(key_felt); | |
| } | |
| Ok(keys_serde) | |
| } |
Thanks for the chance, sir. Would've taken me longer to implement the new changes. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2867 +/- ##
==========================================
+ Coverage 57.04% 57.13% +0.08%
==========================================
Files 427 427
Lines 56561 56686 +125
==========================================
+ Hits 32266 32385 +119
- Misses 24295 24301 +6 ☔ View full report in Codecov by Sentry. |
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:prefix.tagandselectorin the manifest for improved entity categorization.Bug Fixes
Improvements