From 172feb9079cd162a5d23b03a9ef172ec65e5a812 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Delabrouille?= Date: Fri, 21 Jun 2024 15:07:45 +0200 Subject: [PATCH] lucas review --- .../src/felt/primitive_conversions.rs | 78 ++++++++++--------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/crates/starknet-types-core/src/felt/primitive_conversions.rs b/crates/starknet-types-core/src/felt/primitive_conversions.rs index bf80d7e..94ea24e 100644 --- a/crates/starknet-types-core/src/felt/primitive_conversions.rs +++ b/crates/starknet-types-core/src/felt/primitive_conversions.rs @@ -13,12 +13,6 @@ impl From for Felt { } } -impl From for bool { - fn from(value: Felt) -> Self { - value != Felt::ZERO - } -} - // From for Felt macro_rules! impl_from { @@ -122,28 +116,28 @@ macro_rules! impl_try_felt_into_signed { // Bytes are those of Felt::MAX fn try_from(value: Felt) -> Result { let size_of_type = core::mem::size_of::<$into>(); - let bytes_be = value.to_bytes_le(); + let bytes_le = value.to_bytes_le(); // Positive numbers - if bytes_be[size_of_type..].iter().all(|&v| v == 0) - && bytes_be[size_of_type - 1] <= 0x7f + if bytes_le[size_of_type..].iter().all(|&v| v == 0) + && bytes_le[size_of_type - 1] <= 0x7f { Ok(<$into>::from_le_bytes( - bytes_be[..size_of_type].try_into().unwrap(), + bytes_le[..size_of_type].try_into().unwrap(), )) } // Negative numbers - else if bytes_be[size_of_type..] == MINUS_TWO_BYTES_REPR[size_of_type..] - && bytes_be[size_of_type - 1] >= 80 + else if bytes_le[size_of_type..] == MINUS_TWO_BYTES_REPR[size_of_type..] + && bytes_le[size_of_type - 1] >= 0x80 { let offsetted_value = - <$into>::from_le_bytes(bytes_be[..size_of_type].try_into().unwrap()); + <$into>::from_le_bytes(bytes_le[..size_of_type].try_into().unwrap()); // Quite conveniently the byte representation of the `s` least significant bytes // is the one of the negative value we are looking for +1. offsetted_value.checked_sub(1).ok_or(PrimitiveFromFeltError) // -1 - } else if bytes_be[24..] == [17, 0, 0, 0, 0, 0, 0, 8] { + } else if bytes_le[24..] == [17, 0, 0, 0, 0, 0, 0, 8] { // The only valid Felt with those most significant bytes is Felt::MAX. // All other arrays with those most significant bytes are invalid Felts. return Ok(-1); @@ -165,7 +159,6 @@ impl_try_felt_into_signed!(i128); #[cfg(test)] mod tests { use crate::felt::Felt; - #[test] fn from_and_try_from_works_both_way_for_valid_unsigned_values() { // u8 @@ -237,25 +230,21 @@ mod tests { } #[test] - fn from_and_try_from_works_both_way_for_valid_signed_values() { + fn from_and_try_from_works_both_way_for_all_valid_i8_and_i16_values() { // i8 - let i8_max = i8::MAX; - assert_eq!(i8_max, Felt::from(i8_max).try_into().unwrap()); - let i8_min = i8::MIN; - assert_eq!(i8_min, Felt::from(i8_min).try_into().unwrap()); - let i8_zero = 0i8; - assert_eq!(i8_zero, Felt::from(i8_zero).try_into().unwrap()); - let i8_minus_one = -1i8; - assert_eq!(i8_minus_one, Felt::from(i8_minus_one).try_into().unwrap()); + for i in i8::MIN..i8::MAX { + assert_eq!(i, Felt::from(i).try_into().unwrap()); + } // i16 - let i16_max = i16::MAX; - assert_eq!(i16_max, Felt::from(i16_max).try_into().unwrap()); - let i16_min = i16::MIN; - assert_eq!(i16_min, Felt::from(i16_min).try_into().unwrap()); - let i16_zero = 0i16; - assert_eq!(i16_zero, Felt::from(i16_zero).try_into().unwrap()); - let i16_minus_one = -1i16; - assert_eq!(i16_minus_one, Felt::from(i16_minus_one).try_into().unwrap()); + for i in i16::MIN..i16::MAX { + assert_eq!(i, Felt::from(i).try_into().unwrap()); + } + // For the others types it would be too long to test every value exhaustively, + // so we only check keys values in `from_and_try_from_works_both_way_for_valid_signed_values` + } + + #[test] + fn from_and_try_from_works_both_way_for_valid_signed_values() { // i32 let i32_max = i32::MAX; assert_eq!(i32_max, Felt::from(i32_max).try_into().unwrap()); @@ -302,6 +291,17 @@ mod tests { #[test] fn try_from_fail_for_out_of_boud_values() { + for i in (i8::MAX as i16 + 1)..i16::MAX { + let f = Felt::from(i); + assert!(i8::try_from(f).is_err()); + } + for i in i16::MIN..(i8::MIN as i16 - 1) { + let f = Felt::from(i); + assert!(i8::try_from(f).is_err()); + } + // It would be too expansive to check all values of larger types, + // so we just check some key ones + let over_i128_max = Felt::from(i128::MAX) + 1; assert!(i8::try_from(over_i128_max).is_err()); assert!(i16::try_from(over_i128_max).is_err()); @@ -317,14 +317,18 @@ mod tests { assert!(i64::try_from(under_i128_min).is_err()); assert!(isize::try_from(under_i128_min).is_err()); assert!(i128::try_from(under_i128_min).is_err()); + let under_i128_min = Felt::from(i128::MIN) - 2; + assert!(i8::try_from(under_i128_min).is_err()); + assert!(i16::try_from(under_i128_min).is_err()); + assert!(i32::try_from(under_i128_min).is_err()); + assert!(i64::try_from(under_i128_min).is_err()); + assert!(isize::try_from(under_i128_min).is_err()); + assert!(i128::try_from(under_i128_min).is_err()); } #[test] fn felt_from_into_bool() { - assert!(bool::from(Felt::from(true))); - assert!(!bool::from(Felt::from(false))); - - assert!(bool::from(Felt::MAX)); - assert!(!bool::from(Felt::ZERO)); + assert!(Felt::from(true) == Felt::ONE); + assert!(Felt::from(false) == Felt::ZERO); } }