Minor improvements to Utils package (#1206)

* change to for loop

* minor doc fix

* add further in-code docs for snip12

* remove mut from compute_hash param

* fix comment

* add changelog entry

* fix comment

* add domain separator comment

* add tests for average

* add u256 average test

* tidy up test

* Apply suggestions from code review

Co-authored-by: Eric Nordelo <[email protected]>

---------

Co-authored-by: Eric Nordelo <[email protected]>

CHANGELOG.md

@@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - SRC9 (Outside Execution) integration to account presets (#1201)
 
+### Changed
+
+- Remove `mut` from `data` param in `compute_hash_on_elements` (#1206)
+
 ### Changed (Breaking)
 
 - Add `verifying_contract` member to the `Delegation` struct used in Votes `delegate_by_sig` (#1214)

docs/modules/ROOT/pages/api/utilities.adoc

@@ -136,7 +136,7 @@ Returns the contract address when passing the given arguments to {deploy_syscall
 
 [.contract-item]
 [[deployments-compute_hash_on_elements]]
-==== `[.contract-item-name]#++compute_hash_on_elements++#++(mut data: Span<felt252>) → felt252++` [.item-kind]#function#
+==== `[.contract-item-name]#++compute_hash_on_elements++#++(data: Span<felt252>) → felt252++` [.item-kind]#function#
 
 Creates a Pedersen hash chain with the elements of `data` and returns the finalized hash.
 

packages/utils/src/cryptography/snip12.cairo

@@ -16,6 +16,7 @@ use starknet::{ContractAddress, get_tx_info};
 pub const STARKNET_DOMAIN_TYPE_HASH: felt252 =
     0x1ff2f602e42168014d405a94f75e8a93d640751d71d16311266e140d8b0a210;
 
+/// Generic Starknet domain separator representation as defined in SNIP-12.
 #[derive(Drop, Copy, Hash)]
 pub struct StarknetDomain {
     pub name: felt252,
@@ -24,14 +25,17 @@ pub struct StarknetDomain {
     pub revision: felt252,
 }
 
+/// Trait for calculating the hash of a struct.
 pub trait StructHash<T> {
     fn hash_struct(self: @T) -> felt252;
 }
 
+/// Trait for calculating the hash of a message given the passed `signer`.
 pub trait OffchainMessageHash<T> {
     fn get_message_hash(self: @T, signer: ContractAddress) -> felt252;
 }
 
+/// Implementation of `StructHash` that calculates the Poseidon hash of type `StarknetDomain`.
 pub impl StructHashStarknetDomainImpl of StructHash<StarknetDomain> {
     fn hash_struct(self: @StarknetDomain) -> felt252 {
         let hash_state = PoseidonTrait::new();
@@ -47,6 +51,14 @@ pub trait SNIP12Metadata {
     fn version() -> felt252;
 }
 
+/// Implementation of OffchainMessageHash that calculates the Poseidon hash of the message.
+///
+/// The hash state hashes the following in order:
+///
+/// - 'StarkNet Message' short string.
+/// - Starknet domain struct hash.
+/// - `signer` of the message.
+/// - Hashed struct of the message.
 pub(crate) impl OffchainMessageHashImpl<
     T, +StructHash<T>, impl metadata: SNIP12Metadata
 > of OffchainMessageHash<T> {

packages/utils/src/deployments.cairo

@@ -46,20 +46,13 @@ pub fn calculate_contract_address_from_deploy_syscall(
 }
 
 /// Creates a Pedersen hash chain with the elements of `data` and returns the finalized hash.
-fn compute_hash_on_elements(mut data: Span<felt252>) -> felt252 {
-    let data_len = data.len();
+fn compute_hash_on_elements(data: Span<felt252>) -> felt252 {
     let mut state = PedersenTrait::new(0);
-    let mut hash = 0;
-    loop {
-        match data.pop_front() {
-            Option::Some(elem) => { state = state.update_with(*elem); },
-            Option::None => {
-                hash = state.update_with(data_len).finalize();
-                break;
-            },
-        };
+    for elem in data {
+        state = state.update_with(*elem);
     };
-    hash
+
+    state.update_with(data.len()).finalize()
 }
 
 #[derive(Drop)]

packages/utils/src/structs/checkpoint.cairo

@@ -99,12 +99,12 @@ pub impl TraceImpl of TraceTrait {
         }
     }
 
-    /// Returns the number of checkpoints.
+    /// Returns the total number of checkpoints.
     fn length(self: StoragePath<Trace>) -> u64 {
         self.checkpoints.len()
     }
 
-    /// Returns the checkpoint at given position.
+    /// Returns the checkpoint at the given position.
     fn at(self: StoragePath<Trace>, pos: u64) -> Checkpoint {
         assert(pos < self.length(), 'Vec overflow');
         self.checkpoints[pos].read()

packages/utils/src/tests.cairo

@@ -1,3 +1,4 @@
 mod test_checkpoint;
+mod test_math;
 mod test_nonces;
 mod test_snip12;

packages/utils/src/tests/test_math.cairo

@@ -0,0 +1,75 @@
+use core::integer::{u512, u512_safe_div_rem_by_u256};
+use core::num::traits::OverflowingAdd;
+use crate::math::average;
+
+#[test]
+fn test_average_u8(a: u8, b: u8) {
+    let actual = average(a, b);
+
+    let a: u256 = a.into();
+    let b: u256 = b.into();
+    let expected = (a + b) / 2;
+
+    assert_eq!(actual, expected.try_into().unwrap());
+}
+
+#[test]
+fn test_average_u16(a: u16, b: u16) {
+    let actual = average(a, b);
+
+    let a: u256 = a.into();
+    let b: u256 = b.into();
+    let expected = (a + b) / 2;
+
+    assert_eq!(actual, expected.try_into().unwrap());
+}
+
+#[test]
+fn test_average_u32(a: u32, b: u32) {
+    let actual = average(a, b);
+
+    let a: u256 = a.into();
+    let b: u256 = b.into();
+    let expected = (a + b) / 2;
+
+    assert_eq!(actual, expected.try_into().unwrap());
+}
+
+#[test]
+fn test_average_u64(a: u64, b: u64) {
+    let actual = average(a, b);
+
+    let a: u256 = a.into();
+    let b: u256 = b.into();
+    let expected = (a + b) / 2;
+
+    assert_eq!(actual, expected.try_into().unwrap());
+}
+
+#[test]
+fn test_average_u128(a: u128, b: u128) {
+    let actual = average(a, b);
+
+    let a: u256 = a.into();
+    let b: u256 = b.into();
+    let expected = (a + b) / 2;
+
+    assert_eq!(actual, expected.try_into().unwrap());
+}
+
+#[test]
+fn test_average_u256(a: u256, b: u256) {
+    let actual = average(a, b);
+    let mut expected = 0;
+
+    let (sum, overflow) = a.overflowing_add(b);
+    if !overflow {
+        expected = sum / 2;
+    } else {
+        let u512_sum = u512 { limb0: sum.low, limb1: sum.high, limb2: 1, limb3: 0 };
+        let (res, _) = u512_safe_div_rem_by_u256(u512_sum, 2);
+        expected = res.try_into().unwrap();
+    };
+
+    assert_eq!(actual, expected);
+}