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

feat: add support for signed integers #2161

Merged
merged 16 commits into from
Jul 16, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
feat: implement try from felt for signed integers
  • Loading branch information
EjembiEmmanuel committed Jul 14, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 4d0f5f370bc3009d94bfbc9a2b87a97f401c7e10
1 change: 1 addition & 0 deletions crates/dojo-types/src/lib.rs
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ use starknet::core::types::Felt;
pub mod event;
pub mod packing;
pub mod primitive;
pub mod primitive_conversion;
pub mod schema;
pub mod storage;
pub mod system;
57 changes: 42 additions & 15 deletions crates/dojo-types/src/primitive.rs
Original file line number Diff line number Diff line change
@@ -7,6 +7,8 @@
use strum::IntoEnumIterator;
use strum_macros::{AsRefStr, Display, EnumIter, EnumString};

use super::primitive_conversion::try_from_felt;

#[derive(
AsRefStr,
Display,
@@ -148,11 +150,11 @@
Primitive::Felt252(_) => 8,
Primitive::ClassHash(_) => 9,
Primitive::ContractAddress(_) => 10,
Primitive::I8(_) => 11,
Primitive::I16(_) => 12,
Primitive::I32(_) => 13,
Primitive::I64(_) => 14,
Primitive::I128(_) => 15,

Check warning on line 157 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L153-L157

Added lines #L153 - L157 were not covered by tests
}
}

@@ -230,41 +232,36 @@
match self {
Primitive::I8(ref mut value) => {
let felt = felts.remove(0);
*value = Some(felt.to_i8().ok_or_else(|| PrimitiveError::ValueOutOfRange {
r#type: type_name::<i8>(),
value: felt,
*value = Some(try_from_felt::<i8>(felt).map_err(|_| {
PrimitiveError::ValueOutOfRange { r#type: type_name::<i8>(), value: felt }

Check warning on line 236 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L236

Added line #L236 was not covered by tests
})?);
}

Primitive::I16(ref mut value) => {
let felt = felts.remove(0);
*value = Some(felt.to_i16().ok_or_else(|| PrimitiveError::ValueOutOfRange {
r#type: type_name::<i16>(),
value: felt,
*value = Some(try_from_felt::<i16>(felt).map_err(|_| {
PrimitiveError::ValueOutOfRange { r#type: type_name::<i16>(), value: felt }

Check warning on line 243 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L243

Added line #L243 was not covered by tests
})?);
}

Primitive::I32(ref mut value) => {
let felt = felts.remove(0);
*value = Some(felt.to_i32().ok_or_else(|| PrimitiveError::ValueOutOfRange {
r#type: type_name::<i32>(),
value: felt,
*value = Some(try_from_felt::<i32>(felt).map_err(|_| {
PrimitiveError::ValueOutOfRange { r#type: type_name::<i32>(), value: felt }

Check warning on line 250 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L250

Added line #L250 was not covered by tests
})?);
}

Primitive::I64(ref mut value) => {
let felt = felts.remove(0);
*value = Some(felt.to_i64().ok_or_else(|| PrimitiveError::ValueOutOfRange {
r#type: type_name::<i64>(),
value: felt,
*value = Some(try_from_felt::<i64>(felt).map_err(|_| {
PrimitiveError::ValueOutOfRange { r#type: type_name::<i64>(), value: felt }

Check warning on line 257 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L257

Added line #L257 was not covered by tests
})?);
}

Primitive::I128(ref mut value) => {
let felt = felts.remove(0);
*value = Some(felt.to_i128().ok_or_else(|| PrimitiveError::ValueOutOfRange {
r#type: type_name::<i128>(),
value: felt,
*value = Some(try_from_felt::<i128>(felt).map_err(|_| {
PrimitiveError::ValueOutOfRange { r#type: type_name::<i128>(), value: felt }

Check warning on line 264 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L264

Added line #L264 was not covered by tests
})?);
}

@@ -322,12 +319,12 @@
*value = Some(U256::from_be_bytes(bytes));
}

Primitive::USize(ref mut value) => {
let felt = felts.remove(0);
*value = Some(felt.to_u32().ok_or_else(|| PrimitiveError::ValueOutOfRange {
r#type: type_name::<u32>(),
value: felt,
})?);

Check warning on line 327 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L322-L327

Added lines #L322 - L327 were not covered by tests
}

Primitive::Bool(ref mut value) => {
@@ -397,9 +394,9 @@
Ok(vec![value0, value1])
})
.unwrap_or(Err(PrimitiveError::MissingFieldElement)),
Primitive::USize(value) => value
.map(|v| Ok(vec![Felt::from(v)]))
.unwrap_or(Err(PrimitiveError::MissingFieldElement)),

Check warning on line 399 in crates/dojo-types/src/primitive.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive.rs#L397-L399

Added lines #L397 - L399 were not covered by tests
Primitive::Bool(value) => value
.map(|v| Ok(vec![if v { Felt::ONE } else { Felt::ZERO }]))
.unwrap_or(Err(PrimitiveError::MissingFieldElement)),
@@ -425,6 +422,8 @@

use super::Primitive;

use super::try_from_felt;

#[test]
fn test_u256() {
let primitive = Primitive::U256(Some(U256::from_be_hex(
@@ -498,4 +497,32 @@
primitive.set_contract_address(Some(Felt::from(1u128))).unwrap();
assert_eq!(primitive.as_contract_address(), Some(Felt::from(1u128)));
}

#[test]
fn test_try_from_felt() {
let i_8: i8 = -64;
let felt = Felt::from(i_8);
let signed_integer = try_from_felt::<i8>(felt).unwrap();
assert_eq!(i_8, signed_integer);

let i_16: i16 = -14293;
let felt = Felt::from(i_16);
let signed_integer = try_from_felt::<i16>(felt).unwrap();
assert_eq!(i_16, signed_integer);

let i_32: i32 = -194875;
let felt = Felt::from(i_32);
let signed_integer = try_from_felt::<i32>(felt).unwrap();
assert_eq!(i_32, signed_integer);

let i_64: i64 = -3147483648;
let felt = Felt::from(i_64);
let signed_integer = try_from_felt::<i64>(felt).unwrap();
assert_eq!(i_64, signed_integer);

let i_128: i128 = -170141183460469231731687303715884105728;
let felt = Felt::from(i_128);
let signed_integer = try_from_felt::<i128>(felt).unwrap();
assert_eq!(i_128, signed_integer);
}
}
58 changes: 58 additions & 0 deletions crates/dojo-types/src/primitive_conversion.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
use core::convert::TryInto;
use starknet::core::types::Felt;

#[derive(Debug, Copy, Clone)]
pub struct PrimitiveFromFeltError;

impl core::fmt::Display for PrimitiveFromFeltError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
write!(f, "Failed to convert `Felt` into primitive type")
}

Check warning on line 10 in crates/dojo-types/src/primitive_conversion.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive_conversion.rs#L8-L10

Added lines #L8 - L10 were not covered by tests
}

const MINUS_TWO_BYTES_REPR: [u8; 32] = [
255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
255, 255, 255, 255, 255, 16, 0, 0, 0, 0, 0, 0, 8,
];

pub trait FromFelt: Sized {
fn try_from_felt(value: Felt) -> Result<Self, PrimitiveFromFeltError>;
}

macro_rules! impl_from_felt {
($into:ty) => {
impl FromFelt for $into {
fn try_from_felt(value: Felt) -> Result<Self, PrimitiveFromFeltError> {
let size_of_type = core::mem::size_of::<$into>();
let bytes_le = value.to_bytes_le();

if bytes_le[size_of_type..].iter().all(|&v| v == 0)
&& bytes_le[size_of_type - 1] <= 0b01111111
{
Ok(<$into>::from_le_bytes(bytes_le[..size_of_type].try_into().unwrap()))
} else if bytes_le[size_of_type..] == MINUS_TWO_BYTES_REPR[size_of_type..]
&& bytes_le[size_of_type - 1] >= 0b10000000
{
let offsetted_value =
<$into>::from_le_bytes(bytes_le[..size_of_type].try_into().unwrap());

offsetted_value.checked_sub(1).ok_or(PrimitiveFromFeltError)
} else if bytes_le[24..] == [17, 0, 0, 0, 0, 0, 0, 8] {
return Ok(-1);

Check warning on line 41 in crates/dojo-types/src/primitive_conversion.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive_conversion.rs#L40-L41

Added lines #L40 - L41 were not covered by tests
} else {
Err(PrimitiveFromFeltError)

Check warning on line 43 in crates/dojo-types/src/primitive_conversion.rs

Codecov / codecov/patch

crates/dojo-types/src/primitive_conversion.rs#L43

Added line #L43 was not covered by tests
}
}
}
};
}

impl_from_felt!(i8);
impl_from_felt!(i16);
impl_from_felt!(i32);
impl_from_felt!(i64);
impl_from_felt!(i128);

pub fn try_from_felt<T: FromFelt>(value: Felt) -> Result<T, PrimitiveFromFeltError> {
T::try_from_felt(value)
}
10 changes: 5 additions & 5 deletions crates/torii/types-test/src/contracts.cairo
Original file line number Diff line number Diff line change
@@ -61,11 +61,11 @@ mod records {
Record {
record_id,
depth: Depth::Zero,
type_i8: record_idx.into(),
type_i16: record_idx.into(),
type_i32: record_idx.into(),
type_i64: record_idx.into(),
type_i128: record_idx.into(),
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(),
Comment on lines +64 to +68
Copy link

Choose a reason for hiding this comment

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

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.

type_u8: record_idx.into(),
type_u16: record_idx.into(),
type_u32: record_idx.into(),
3 changes: 2 additions & 1 deletion examples/spawn-and-move/src/actions.cairo
Original file line number Diff line number Diff line change
@@ -85,7 +85,8 @@ mod actions {
let player = get_caller_address();

let items = array![
PlayerItem { item_id: 1, quantity: 100 }, PlayerItem { item_id: 2, quantity: 50 }
PlayerItem { item_id: 1, quantity: 100, score: 10 },
PlayerItem { item_id: 2, quantity: 50, score: -32 }
];

let config = PlayerConfig { player, name, items, favorite_item: Option::Some(1), };
1 change: 1 addition & 0 deletions examples/spawn-and-move/src/models.cairo
Original file line number Diff line number Diff line change
@@ -74,6 +74,7 @@ struct Position {
struct PlayerItem {
item_id: u32,
quantity: u32,
score: i32,
}

#[derive(Drop, Serde)]
Loading