feat: add support for signed integers #2161
Conversation
WalkthroughThe changes introduce support for signed integers across various components. A new Changes
Sequence Diagram(s)The changes are too varied to be represented in a single sequence diagram and do not lend themselves well to such visualization due to their spread across multiple files and functionalities. Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@glihm I've opened a new PR here just as we discussed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2161 +/- ##
==========================================
+ Coverage 67.50% 67.77% +0.26%
==========================================
Files 335 336 +1
Lines 43563 43936 +373
==========================================
+ Hits 29409 29779 +370
- Misses 14154 14157 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looking great so far, thank you for the new PR!
So now, to continue on this path, you have the following things to check:
- Torii has some tests with a
types-testcairo project. You must add some signed integers here to check how Torii indexes it. - After that, we also need
sozoto be able to parse a signed integer, which should be now easy with the new Felt type.
Great progress!
Got it! |
There was a problem hiding this comment.
In good way!
Also, to easily test something I strongly recommend:
- Adding a function to the
spawn-and-moveexample to deal with integers, + a new model / modify an existing model with an integer. - Use the
execute actions <FUNC_NAME>to set some values. - Use
sozo model get <MODEL_NAME> [<KEYS>]to actually inspect the world's storage during debug phase.
| match input.parse::<i128>() { | ||
| Ok(value) if value <= i8::MAX as i128 && value >= i8::MIN as i128 => { | ||
| Ok(vec![Felt::from_i8(value as i8).expect("Invalid numeric string")]) | ||
| } | ||
| Ok(value) if value <= i16::MAX as i128 && value >= i16::MIN as i128 => { | ||
| Ok(vec![Felt::from_i16(value as i16).expect("Invalid numeric string")]) | ||
| } | ||
| Ok(value) if value <= i32::MAX as i128 && value >= i32::MIN as i128 => { | ||
| Ok(vec![Felt::from_i32(value as i32).expect("Invalid numeric string")]) | ||
| } | ||
| Ok(value) if value <= i64::MAX as i128 && value >= i64::MIN as i128 => { | ||
| Ok(vec![Felt::from_i64(value as i64).expect("Invalid numeric string")]) | ||
| } | ||
| Ok(value) => Ok(vec![Felt::from_i128(value as i128).expect("Invalid numeric string")]), | ||
| Err(_) => Err(CalldataDecoderError::ParseError("Invalid numeric string".to_string())), | ||
| } |
There was a problem hiding this comment.
Here this can be refactoring to something more simple, something like this should work:
fn decode(&self, input: &str) -> DecoderResult<Vec<Felt>> {
if let Ok(value) = input.parse::<i128>() {
Ok(vec![value.into()])
} else {
Err(CalldataDecoderError::ParseError("Invalid numeric string".to_string()))
}
}The reason is that, no matter which signed integer we have, they are all 1 felt long when serialized. 👍
| @@ -1,4 +1,5 @@ | |||
| use anyhow::{self, Result}; | |||
| use bigdecimal::FromPrimitive; | |||
There was a problem hiding this comment.
This may not be needed with the suggested refactoring below.
| @@ -150,6 +174,7 @@ fn decode_inner(item: &str) -> DecoderResult<Vec<Felt>> { | |||
| "u256" => U256CalldataDecoder.decode(value)?, | |||
| "str" => StrCalldataDecoder.decode(value)?, | |||
| "sstr" => ShortStrCalldataDecoder.decode(value)?, | |||
| "-" => SignedIntegerCalldataDecoder.decode(value)?, | |||
There was a problem hiding this comment.
| "-" => SignedIntegerCalldataDecoder.decode(value)?, | |
| "int" => SignedIntegerCalldataDecoder.decode(value)?, |
Use int instead, I didn't think about clap that will try to consume the -, hence not working.
bin/sozo/src/commands/execute.rs
Outdated
| @@ -31,6 +31,7 @@ pub struct ExecuteArgs { | |||
| - u256: A 256-bit unsigned integer. | |||
| - sstr: A cairo short string. | |||
| - str: A cairo string (ByteArray). | |||
| - -: A signed integer. | |||
There was a problem hiding this comment.
| - -: A signed integer. | |
| - int: A signed integer. |
@glihm I'm a little lost on what to do here. Could you please explain a bit more? |
|
We're actually blocked by starknet-io/types-rs#74. Seems like in the version we have, the high end of the field is not converted to negative, but |
|
I was giving you some other way to end-to-end test a signed integer in a real contract to verify the workflow works correctly. Feel free to inc inside Dojo discord I can show you in a call what I meant. But basically, deploying a dojo project and running Torii by hand gives you a good understand on how this value is actually processed, from the command line to the indexing. |
Ok I'll give it a try. I'll let you know if I have questions |
|
@glihm, I was able to spin up a fresh Katana instance to test the feature I added. However, when I execute commands with Sozo, like: the operation doesn't perform as expected. From my understanding, isn't the spawn command supposed to update the Moves model? You can see from the screenshot below that the models aren't being updated. Could you help me identify what might be going wrong?
|
|
@EjembiEmmanuel so:
|
|
Yea, that's exactly what I did. |
@glihm I was able to get it to work thank you! |
We would have to wait for this to continue right? |
|
@glihm I modified the PlayerItem model to include a field for signed integers. While setting and getting positive signed integers works fine, I encounter an error when I try to get a negative signed integer after setting it.
|
Nice, yes exactly this is the reason why we depend on the fix of the Felt type. Because currently only positive ones are supported. :) |
|
Arg.. the issue seems a bit stalled for now. We can do a workaround @EjembiEmmanuel. We can take the modification they are doing in their PR, only the file we are interested in, and we use a function instead of deriving Patching will not be that good I guess because we already patch the starknet-core-types library. |
Sounds good. |
|
@glihm I successfully implemented and tested the conversion of Felt for signed integers, which is working perfectly. However, when I assign a negative integer to a field in a model, such as in this example: and query it using: The result is returned in
|
There was a problem hiding this comment.
Nice @EjembiEmmanuel!
The reason why you have such a big value it's due to how the negative sign is encoded into the felt value.
So even for small values, if they are negative, some bits are set which cause the value to be huge.
To solve the print to not have the raw value but the integer value instead, you'll have to convert back the value to integer when using the model get. 👍
As you have access to the layout when you get the value, you should be able to know when to apply such conversion.
scripts/prettier.sh
Outdated
There was a problem hiding this comment.
This file shouldn't have changed I guess, would you mind reverting?
There was a problem hiding this comment.
Good job on that! Would you mind please adding a comment for the reason having this file + a link to the PR, to ensure we can track it easily and remove the code once merged?
Please can you explain how I can use |
So, Torii has the same issue: The idea here: I've just pushed the fix + some formatting to include this into the next alpha. |
Ahh I see, looking good! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
crates/dojo-types/src/primitive_conversion.rs (1)
34-67: Insufficient test coverage for conversion logic.The current test function
test_try_from_feltonly covers the conversion for thei8type and a single negative value. To ensure robustness, please add comprehensive tests for all integer types (i16,i32,i64,i128) and various edge cases, including positive values and boundary values.
- Add tests for
i16,i32,i64, andi128.- Include edge cases such as maximum and minimum values for each type.
- Test positive and boundary values.
Analysis chain
Approved conversion logic for signed integers.
The implementation of conversion logic from
Feltto signed integers (i8,i16,i32,i64,i128) is crucial for the support of these types. Please ensure comprehensive tests are added to verify the robustness of this conversion, especially for edge cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the robustness of the conversion logic through comprehensive testing. # Test: Search for tests related to the conversion logic. Expect: Comprehensive coverage of edge cases. rg --type rust -A 5 $'test_try_from_felt'Length of output: 516
Script:
#!/bin/bash # Description: Verify the robustness of the conversion logic through comprehensive testing. # Test: Search for additional tests related to the conversion logic for other integer types and edge cases. rg --type rust -A 5 'fn test_try_from_felt'Length of output: 521
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (23)
- bin/sozo/src/commands/calldata_decoder.rs (3 hunks)
- bin/sozo/src/commands/execute.rs (1 hunks)
- crates/dojo-core/src/database/introspect.cairo (1 hunks)
- crates/dojo-types/src/lib.rs (1 hunks)
- crates/dojo-types/src/primitive.rs (14 hunks)
- crates/dojo-types/src/primitive_conversion.rs (1 hunks)
- crates/dojo-types/src/schema.rs (1 hunks)
- crates/sozo/ops/src/model.rs (1 hunks)
- crates/torii/core/src/model.rs (2 hunks)
- crates/torii/grpc/src/types/schema.rs (1 hunks)
- crates/torii/types-test/src/contracts.cairo (1 hunks)
- crates/torii/types-test/src/models.cairo (3 hunks)
- examples/spawn-and-move/manifests/dev/base/abis/models/dojo_examples-PlayerConfig-3adad785.json (1 hunks)
- examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-actions-40b6994c.toml (1 hunks)
- examples/spawn-and-move/manifests/dev/base/models/dojo_examples-PlayerConfig-3adad785.toml (1 hunks)
- examples/spawn-and-move/manifests/dev/deployment/abis/models/dojo_examples-PlayerConfig-3adad785.json (1 hunks)
- examples/spawn-and-move/manifests/dev/deployment/manifest.json (3 hunks)
- examples/spawn-and-move/manifests/dev/deployment/manifest.toml (2 hunks)
- examples/spawn-and-move/manifests/release/base/abis/models/dojo_examples-PlayerConfig-3adad785.json (1 hunks)
- examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-actions-40b6994c.toml (1 hunks)
- examples/spawn-and-move/manifests/release/base/models/dojo_examples-PlayerConfig-3adad785.toml (1 hunks)
- examples/spawn-and-move/src/actions.cairo (1 hunks)
- examples/spawn-and-move/src/models.cairo (1 hunks)
Files skipped from review due to trivial changes (6)
- crates/dojo-types/src/lib.rs
- examples/spawn-and-move/manifests/dev/base/abis/models/dojo_examples-PlayerConfig-3adad785.json
- examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-actions-40b6994c.toml
- examples/spawn-and-move/manifests/dev/base/models/dojo_examples-PlayerConfig-3adad785.toml
- examples/spawn-and-move/manifests/dev/deployment/abis/models/dojo_examples-PlayerConfig-3adad785.json
- examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-actions-40b6994c.toml
Additional comments not posted (14)
examples/spawn-and-move/manifests/release/base/models/dojo_examples-PlayerConfig-3adad785.toml (1)
2-3: Verify updated class hashes.The
class_hashandoriginal_class_hashhave been updated. Please ensure these hashes correctly correspond to the updated class definitions in the associated contract.examples/spawn-and-move/src/models.cairo (1)
77-77: Approved addition ofscorefield.The addition of the
scorefield of typei32to thePlayerItemstruct aligns with the PR's objective to support signed integers. Please verify its integration with related functionalities, such as database interactions and UI displays.Verification successful
Approved addition of
scorefield.The addition of the
scorefield of typei32to thePlayerItemstruct aligns with the PR's objective to support signed integers. Thescorefield is properly integrated and utilized in the codebase, particularly inactions.cairo.
examples/spawn-and-move/src/actions.cairo: Instances ofPlayerItemwith thescorefield are being created and used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the `score` field with related functionalities. # Test: Search for usages of the `PlayerItem` struct. Expect: Proper handling of the `score` field. rg --type cairo -A 5 $'PlayerItem'Length of output: 63
Script:
#!/bin/bash # Description: Verify the integration of the `score` field with related functionalities. # Test: Search for the definition and usages of the `PlayerItem` struct. Expect: Proper handling of the `score` field. rg 'struct PlayerItem' -A 5Length of output: 351
Script:
#!/bin/bash # Description: Verify the integration of the `score` field with related functionalities. # Test: Search for all occurrences of `PlayerItem` in the codebase. Expect: Proper handling of the `score` field. rg 'PlayerItem'Length of output: 3360
Script:
#!/bin/bash # Description: Verify the integration of the `score` field with related functionalities. # Test: Search for functions and methods in `actions.cairo` that involve `PlayerItem`. Expect: Proper handling of the `score` field. rg 'PlayerItem' examples/spawn-and-move/src/actions.cairo -A 10Length of output: 965
bin/sozo/src/commands/execute.rs (1)
34-34: Approved addition ofintcalldata type.The addition of the
intcalldata type for signed integers is a crucial part of supporting these types in system executions. Please verify its correct parsing and handling during execution.crates/torii/types-test/src/models.cairo (1)
10-14: New signed integer fields added to the Record model are well-defined.The addition of signed integer fields (
type_i8,type_i16,type_i32,type_i64,type_i128) to theRecordmodel aligns with the PR's objectives to support signed integers in the Dojo engine.crates/dojo-core/src/database/introspect.cairo (1)
159-217: Implementation of the Introspect trait for signed integers is correctly done.The implementations for
Introspect<i8>,Introspect<i16>,Introspect<i32>,Introspect<i64>,Introspect<i128>are consistent with the requirements to support signed integers with a fixed layout of 251 bytes. This aligns well with the PR's objectives and the linked issue's requirements.examples/spawn-and-move/manifests/dev/deployment/manifest.toml (1)
Line range hint
27-200: Updated class hashes in deployment configurations reflect the latest changes.The updates to
class_hashandoriginal_class_hashvalues across various contracts and models in the deployment configurations ensure that they are in sync with the latest codebase changes, including the support for signed integers.examples/spawn-and-move/manifests/release/base/abis/models/dojo_examples-PlayerConfig-3adad785.json (1)
382-383: Approved: Addition ofscorefield toPlayerItemstruct.The addition of the
scorefield of typecore::integer::i32is correctly implemented and aligns with the PR's objective to support signed integers.examples/spawn-and-move/src/actions.cairo (1)
119-120: Approved: Addition ofscorefield inPlayerIteminstances withinset_player_config.The addition of the
scorefield with both positive and negative values in thePlayerIteminstances is correctly implemented. This change is essential for testing the handling of signed integers in the system.crates/torii/grpc/src/types/schema.rs (1)
192-198: Approved: Updates to handle signed integer types in GRPC schema.The implementation for handling new signed integer types (
i8,i16,i32,i64,i128) in the GRPC schema is correctly done. The updates ensure that these types are appropriately serialized and deserialized, facilitating their use across different parts of the system.Also applies to: 209-210
crates/dojo-types/src/primitive.rs (4)
30-34: Addition of signed integer types to thePrimitiveenum.The addition of signed integer types (
I8,I16,I32,I64,I128) is consistent with the PR's objectives to support signed integers. This change looks good.
67-67: Addition ofFeltFromFeltErrorto handle conversion errors.The introduction of
FeltFromFeltErrorto handle conversion errors for the new signed integer types is a good practice, ensuring robust error handling.
108-112: Getter methods for signed integer types.The implementation of getter methods for the new signed integer types (
as_i8,as_i16,as_i32,as_i64,as_i128) is correctly done and follows the established pattern for other types.
125-129: Setter methods for signed integer types.The setter methods for the new signed integer types are correctly implemented, following the established pattern and ensuring type safety.
crates/sozo/ops/src/model.rs (1)
176-180: Handling of signed integer types inget_name_from_schema.The update to
get_name_from_schemato include the new signed integer types (i8,i16,i32,i64,i128) ensures that these types are correctly recognized and handled in model schemas.
| Primitive::I8(_) => { | ||
| let value = row.try_get::<i8, &str>(&column_name)?; | ||
| primitive.set_i8(Some(value))?; | ||
| } | ||
| Primitive::USize(_) => { | ||
| let value = row.try_get::<u32, &str>(&column_name)?; | ||
| primitive.set_usize(Some(value))?; | ||
| Primitive::I16(_) => { | ||
| let value = row.try_get::<i16, &str>(&column_name)?; | ||
| primitive.set_i16(Some(value))?; | ||
| } | ||
| Primitive::I32(_) => { | ||
| let value = row.try_get::<i32, &str>(&column_name)?; | ||
| primitive.set_i32(Some(value))?; | ||
| } | ||
| Primitive::I64(_) => { | ||
| let value = row.try_get::<String, &str>(&column_name)?; | ||
| let hex_str = value.trim_start_matches("0x"); | ||
| primitive.set_i64(Some( | ||
| i64::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError)?, | ||
| ))?; | ||
| } | ||
| Primitive::I128(_) => { | ||
| let value = row.try_get::<String, &str>(&column_name)?; | ||
| let hex_str = value.trim_start_matches("0x"); | ||
| primitive.set_i128(Some( | ||
| i128::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError)?, | ||
| ))?; |
There was a problem hiding this comment.
Review of signed integer handling in map_row_to_ty function.
The implementation for handling signed integers (i8, i16, i32, i64, i128) has been added. The handling for i64 and i128 involves converting from a hexadecimal string, which is appropriate given the context of handling large integers in systems like StarkNet.
However, there are a couple of points to consider:
- Error Handling: The use of
map_err(ParseError::ParseIntError)?is consistent and ensures that parsing errors are handled gracefully. - Code Duplication: The pattern for handling
i64andi128is almost identical, suggesting that a helper function could be used to reduce duplication and improve maintainability.
Consider refactoring to reduce code duplication:
+ fn parse_hex_to_i64(column_value: &str) -> Result<i64, ParseError> {
+ let hex_str = column_value.trim_start_matches("0x");
+ i64::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError)
+ }
+
+ fn parse_hex_to_i128(column_value: &str) -> Result<i128, ParseError> {
+ let hex_str = column_value.trim_start_matches("0x");
+ i128::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError)
+ }
- let hex_str = value.trim_start_matches("0x");
- primitive.set_i64(Some(
- i64::from_str_radic(hex_str, 16).map_err(ParseError::ParseIntError)?,
- ))?;
+ let parsed_value = parse_hex_to_i64(&value)?;
+ primitive.set_i64(Some(parsed_value))?;This refactoring introduces helper functions to handle the parsing of hexadecimal strings to i64 and i128, reducing code duplication and centralizing the parsing logic.
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.
| Primitive::I8(_) => { | |
| let value = row.try_get::<i8, &str>(&column_name)?; | |
| primitive.set_i8(Some(value))?; | |
| } | |
| Primitive::USize(_) => { | |
| let value = row.try_get::<u32, &str>(&column_name)?; | |
| primitive.set_usize(Some(value))?; | |
| Primitive::I16(_) => { | |
| let value = row.try_get::<i16, &str>(&column_name)?; | |
| primitive.set_i16(Some(value))?; | |
| } | |
| Primitive::I32(_) => { | |
| let value = row.try_get::<i32, &str>(&column_name)?; | |
| primitive.set_i32(Some(value))?; | |
| } | |
| Primitive::I64(_) => { | |
| let value = row.try_get::<String, &str>(&column_name)?; | |
| let hex_str = value.trim_start_matches("0x"); | |
| primitive.set_i64(Some( | |
| i64::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError)?, | |
| ))?; | |
| } | |
| Primitive::I128(_) => { | |
| let value = row.try_get::<String, &str>(&column_name)?; | |
| let hex_str = value.trim_start_matches("0x"); | |
| primitive.set_i128(Some( | |
| i128::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError)?, | |
| ))?; | |
| Primitive::I8(_) => { | |
| let value = row.try_get::<i8, &str>(&column_name)?; | |
| primitive.set_i8(Some(value))?; | |
| } | |
| Primitive::I16(_) => { | |
| let value = row.try_get::<i16, &str>(&column_name)?; | |
| primitive.set_i16(Some(value))?; | |
| } | |
| Primitive::I32(_) => { | |
| let value = row.try_get::<i32, &str>(&column_name)?; | |
| primitive.set_i32(Some(value))?; | |
| } | |
| Primitive::I64(_) => { | |
| let value = row.try_get::<String, &str>(&column_name)?; | |
| let parsed_value = parse_hex_to_i64(&value)?; | |
| primitive.set_i64(Some(parsed_value))?; | |
| } | |
| Primitive::I128(_) => { | |
| let value = row.try_get::<String, &str>(&column_name)?; | |
| let parsed_value = parse_hex_to_i128(&value)?; | |
| primitive.set_i128(Some(parsed_value))?; | |
| } | |
| + fn parse_hex_to_i64(column_value: &str) -> Result<i64, ParseError> { | |
| + let hex_str = column_value.trim_start_matches("0x"); | |
| + i64::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError) | |
| + } | |
| + | |
| + fn parse_hex_to_i128(column_value: &str) -> Result<i128, ParseError> { | |
| + let hex_str = column_value.trim_start_matches("0x"); | |
| + i128::from_str_radix(hex_str, 16).map_err(ParseError::ParseIntError) | |
| + } |
| type_i8: type_felt.try_into().unwrap(), | ||
| type_i16: type_felt.try_into().unwrap(), | ||
| type_i32: type_felt.try_into().unwrap(), | ||
| type_i64: type_felt.try_into().unwrap(), | ||
| type_i128: type_felt.try_into().unwrap(), |
There was a problem hiding this comment.
Consider safer error handling when converting types.
The current implementation uses unwrap() for converting type_felt into various signed integer types. This could lead to panics if the conversion fails. Consider using a safer approach to handle potential errors gracefully.
- type_i8: type_felt.try_into().unwrap(),
+ type_i8: type_felt.try_into().unwrap_or_default(),Repeat similar changes for type_i16, type_i32, type_i64, type_i128.
Committable suggestion was skipped due to low confidence.
| Primitive::I8(value) => { | ||
| if let Some(value) = value { | ||
| str.push_str(&format!(" = {}", value)); | ||
| } | ||
| } | ||
| Primitive::I16(value) => { | ||
| if let Some(value) = value { | ||
| str.push_str(&format!(" = {}", value)); | ||
| } | ||
| } | ||
| Primitive::I32(value) => { | ||
| if let Some(value) = value { | ||
| str.push_str(&format!(" = {}", value)); | ||
| } | ||
| } | ||
| Primitive::I64(value) => { | ||
| if let Some(value) = value { | ||
| str.push_str(&format!(" = {}", value)); | ||
| } | ||
| } | ||
| Primitive::I128(value) => { | ||
| if let Some(value) = value { | ||
| str.push_str(&format!(" = {}", value)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring to reduce redundancy in value formatting.
The code for formatting values for different types of primitives is repetitive. Consider using a more generic approach to reduce redundancy and improve maintainability.
- Primitive::I8(value) => {
- if let Some(value) = value {
- str.push_str(&format!(" = {}", value));
- }
- }
- // Repeat for I16, I32, etc.
+ Primitive::I8(value) | Primitive::I16(value) | Primitive::I32(value) | Primitive::I64(value) | Primitive::I128(value) => {
+ if let Some(value) = value {
+ str.push_str(&format!(" = {}", value));
+ }
+ }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.
| Primitive::I8(value) => { | |
| if let Some(value) = value { | |
| str.push_str(&format!(" = {}", value)); | |
| } | |
| } | |
| Primitive::I16(value) => { | |
| if let Some(value) = value { | |
| str.push_str(&format!(" = {}", value)); | |
| } | |
| } | |
| Primitive::I32(value) => { | |
| if let Some(value) = value { | |
| str.push_str(&format!(" = {}", value)); | |
| } | |
| } | |
| Primitive::I64(value) => { | |
| if let Some(value) = value { | |
| str.push_str(&format!(" = {}", value)); | |
| } | |
| } | |
| Primitive::I128(value) => { | |
| if let Some(value) = value { | |
| str.push_str(&format!(" = {}", value)); | |
| } | |
| } | |
| Primitive::I8(value) | Primitive::I16(value) | Primitive::I32(value) | Primitive::I64(value) | Primitive::I128(value) => { | |
| if let Some(value) = value { | |
| str.push_str(&format!(" = {}", value)); | |
| } | |
| } |
| /// Decodes a signed integer into a [`Felt`] | ||
| struct SignedIntegerCalldataDecoder; | ||
| impl CalldataDecoder for SignedIntegerCalldataDecoder { | ||
| fn decode(&self, input: &str) -> DecoderResult<Vec<Felt>> { | ||
| if let Ok(value) = input.parse::<i128>() { | ||
| Ok(vec![value.into()]) | ||
| } else { | ||
| Err(CalldataDecoderError::ParseError("Invalid numeric string".to_string())) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Approved: Implementation of SignedIntegerCalldataDecoder and its tests.
The implementation of SignedIntegerCalldataDecoder for decoding signed integers from calldata is correctly done. The addition of comprehensive tests for various signed integer types (i8, i16, i32, i64, i128) ensures the correctness of the decoder.
Consider adopting the suggested refactoring to treat all signed integers as a single Felt regardless of their size, which could simplify the code and improve efficiency.
fn decode(&self, input: &str) -> DecoderResult<Vec<Felt>> {
if let Ok(value) = input.parse::<i128>() {
Ok(vec![value.into()])
} else {
Err(CalldataDecoderError::ParseError("Invalid numeric string".to_string()))
}
}Also applies to: 252-295
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- bin/sozo/src/commands/calldata_decoder.rs (4 hunks)
- crates/dojo-types/src/primitive.rs (15 hunks)
- crates/dojo-types/src/schema.rs (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- bin/sozo/src/commands/calldata_decoder.rs
- crates/dojo-types/src/primitive.rs
Additional comments not posted (2)
crates/dojo-types/src/schema.rs (2)
372-396: Refactor to reduce redundancy in value formatting.The changes to handle new primitive types (I8, I16, I32, I64, I128) are correct and align with the PR's objectives. However, the code is repetitive and could benefit from a more generic approach to reduce redundancy and improve maintainability.
- Primitive::I8(value) => { - if let Some(value) = value { - str.push_str(&format!(" = {}", value)); - } - } - // Repeat for I16, I32, etc. + Primitive::I8(value) | Primitive::I16(value) | Primitive::I32(value) | Primitive::I64(value) | Primitive::I128(value) => { + if let Some(value) = value { + str.push_str(&format!(" = {}", value)); + } + }
471-603: Approve the test cases for signed integer formatting.The test cases for new signed integer types (I8, I16, I32, I64, I128) are well-implemented and cover basic scenarios. However, consider adding more comprehensive tests to cover edge cases and potential error scenarios to ensure robustness.
There was a problem hiding this comment.
@EjembiEmmanuel congrats for this work here and your contribution! 🚀
@glihm thank you for the opportunity to contribute to this project. I just picked up Rust and I must say I learnt about working on this issue. Looking forward to more collaborations! |
* feat: add support for signed integers * chore: run scripts/rust_fmt.sh * feat: add support for signed integers * chore: run scripts/rust_fmt.sh * feat: decode signed integer * test: test signed integers decoder * refac: remove redundant type casting * refac: modify signed integer decoder * feat: implement try from felt for signed integers * Delete scripts/prettier.sh * chore: add comments * fix: ensure signed integers are correctly printed * fix: add more tests --------- Co-authored-by: glihm <[email protected]>
Description
This PR adds support for signed integers
Related issue
Fixes #2120
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
i8,i16,i32,i64,i128) for introspection and conversion.Primitiveenum to include signed integer variants.format_memberfunction to handle new integer types.