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

Minor improvements to Utils package #1206

Merged
merged 13 commits into from
Nov 19, 2024
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion docs/modules/ROOT/pages/api/utilities.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
12 changes: 12 additions & 0 deletions packages/utils/src/cryptography/snip12.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();
Expand All @@ -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> {
Expand Down
17 changes: 5 additions & 12 deletions packages/utils/src/deployments.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
4 changes: 2 additions & 2 deletions packages/utils/src/structs/checkpoint.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

I think position is easier to read here, maybe we can update the param name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm I honestly like pos as the param name—unless you prefer a new name? I'll switch the comment back. Looking at it now, I agree position is easier to read

/// 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()
Expand Down
1 change: 1 addition & 0 deletions packages/utils/src/tests.cairo
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod test_checkpoint;
mod test_math;
mod test_nonces;
mod test_snip12;
75 changes: 75 additions & 0 deletions packages/utils/src/tests/test_math.cairo
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that fuzzing actually takes a significant amount of time when running tests, not only in compilation. Did you noticed the same thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did...it's only a few tests though so relative to the whole repo, IMO it's not a huge deal. If we want to improve performance, we can configure it to a smaller amount either in the tests themselves or globally when running the test cmd i.e. snforge test ... --fuzzer-runs 25

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);
}