From 52e7dd43e4a9ce45f27baba6ebbbefdf4343efce Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 11:48:40 +0100 Subject: [PATCH 01/11] refactor: explicit felt size when reading arrays --- starknet/src/types/strategy.cairo | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/starknet/src/types/strategy.cairo b/starknet/src/types/strategy.cairo index 45bc7ea4d..15d94264d 100644 --- a/starknet/src/types/strategy.cairo +++ b/starknet/src/types/strategy.cairo @@ -1,5 +1,4 @@ -use starknet::ContractAddress; -use starknet::{StorageBaseAddress, Store, SyscallResult}; +use starknet::{ContractAddress, StorageBaseAddress, Store, SyscallResult}; /// A strategy identified by an address #[derive(Clone, Drop, Option, Serde, starknet::Store)] @@ -47,7 +46,7 @@ impl StoreFelt252Array of Store> { offset += 1; // Sequentially read all stored elements and append them to the array. - let exit = len + offset; + let exit = len * Store::::size() + offset; loop { if offset >= exit { break; From 710d79df636cdf3a9c6bf44b97528a49ee529e86 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 11:54:15 +0100 Subject: [PATCH 02/11] refactor: use bounded u8 max impl --- starknet/src/types/strategy.cairo | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/starknet/src/types/strategy.cairo b/starknet/src/types/strategy.cairo index 15d94264d..8e1532a1f 100644 --- a/starknet/src/types/strategy.cairo +++ b/starknet/src/types/strategy.cairo @@ -1,3 +1,4 @@ +use integer::BoundedU8; use starknet::{ContractAddress, StorageBaseAddress, Store, SyscallResult}; /// A strategy identified by an address @@ -90,7 +91,7 @@ impl StoreFelt252Array of Store> { fn size() -> u8 { /// Since the array is a dynamic type. We use its max size here. - 255_u8 + BoundedU8::max() } } From 7087210387e323632d97821bacc6470e2a055d59 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 12:09:54 +0100 Subject: [PATCH 03/11] refactor zeroable impl for zero address --- starknet/src/utils/default.cairo | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/starknet/src/utils/default.cairo b/starknet/src/utils/default.cairo index c78b99a11..bdd121bdc 100644 --- a/starknet/src/utils/default.cairo +++ b/starknet/src/utils/default.cairo @@ -1,8 +1,8 @@ -use starknet::{ContractAddress, contract_address_const}; +use starknet::ContractAddress; // Ideally, ContractAddress would impl Default in the corelib. impl ContractAddressDefault of Default { fn default() -> ContractAddress { - contract_address_const::<0>() + Zeroable::zero() } } From 8fc30dbcae29e6c7732336de5c8a609bfe8b7da7 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 12:17:59 +0100 Subject: [PATCH 04/11] refactor: remove redundant addition --- starknet/src/utils/eip712.cairo | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/starknet/src/utils/eip712.cairo b/starknet/src/utils/eip712.cairo index 9f8088963..a702e3219 100644 --- a/starknet/src/utils/eip712.cairo +++ b/starknet/src/utils/eip712.cairo @@ -202,8 +202,7 @@ mod EIP712 { /// Adds a 16 bit prefix to a 128 bit input, returning the result and a carry. fn add_prefix_u128(input: u128, prefix: u128) -> (u128, u128) { - let with_prefix = u256 { low: input, high: 0_u128 } - + u256 { low: 0_u128, high: prefix }; + let with_prefix = u256 { low: input, high: prefix }; let carry = with_prefix & 0xffff; // Removing the carry and shifting back. let out = (with_prefix - carry) / 0x10000; From b0fc678f0962b03e5e994fed05715accf0678626 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 12:42:09 +0100 Subject: [PATCH 05/11] refactor: param deserialization --- .../src/voting_strategies/merkle_whitelist.cairo | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/starknet/src/voting_strategies/merkle_whitelist.cairo b/starknet/src/voting_strategies/merkle_whitelist.cairo index b2633ae18..eb01a9cd0 100644 --- a/starknet/src/voting_strategies/merkle_whitelist.cairo +++ b/starknet/src/voting_strategies/merkle_whitelist.cairo @@ -32,15 +32,12 @@ mod MerkleWhitelistVotingStrategy { params: Span, // [root: felt252] user_params: Span, // [leaf: Leaf, proof: Array] ) -> u256 { - let cache = user_params; // cache + let mut params = params; + let mut user_params = user_params; - let mut leaf_raw = cache.slice(0, LEAF_SIZE); - let leaf = Serde::::deserialize(ref leaf_raw).unwrap(); - - let mut proofs_raw = cache.slice(LEAF_SIZE, cache.len() - LEAF_SIZE); - let proofs = Serde::>::deserialize(ref proofs_raw).unwrap(); - - let root = *params.at(0); // no need to deserialize because it's a simple value + let root = Serde::::deserialize(ref params).unwrap(); + let (leaf, proofs) = Serde::<(Leaf, Array)>::deserialize(ref user_params) + .unwrap(); assert(leaf.address == voter, 'Leaf and voter mismatch'); merkle::assert_valid_proof(root, leaf, proofs.span()); From 8b3b361f5c3e2ddf7f44a8504903db2c5c974f46 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 12:55:56 +0100 Subject: [PATCH 06/11] refactor: use is non zero impl --- starknet/src/authenticators/eth_tx.cairo | 2 +- starknet/src/space/space.cairo | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/starknet/src/authenticators/eth_tx.cairo b/starknet/src/authenticators/eth_tx.cairo index 6a96fb0ee..71ba3e476 100644 --- a/starknet/src/authenticators/eth_tx.cairo +++ b/starknet/src/authenticators/eth_tx.cairo @@ -185,7 +185,7 @@ mod EthTxAuthenticator { impl InternalImpl of InternalTrait { fn consume_commit(ref self: ContractState, hash: felt252, sender_address: EthAddress) { let committer_address = self._commits.read(hash); - assert(!committer_address.is_zero(), 'Commit not found'); + assert(committer_address.is_non_zero(), 'Commit not found'); assert(committer_address == sender_address, 'Invalid sender address'); // Delete the commit to prevent replay attacks. self._commits.write(hash, Zeroable::zero()); diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index b2e6c70f5..f642426e2 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -787,7 +787,7 @@ mod Space { loop { match _voting_strategies.pop_front() { Option::Some(strategy) => { - assert(!(*strategy.address).is_zero(), 'Invalid voting strategy'); + assert((*strategy.address).is_non_zero(), 'Invalid voting strategy'); cachedActiveVotingStrategies.set_bit(cachedNextVotingStrategyIndex, true); self ._voting_strategies From 5039ded6bc1895f44681ca1ce603caa9a1b6e0f4 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 12:56:28 +0100 Subject: [PATCH 07/11] refactor: systematic u64s order --- starknet/src/utils/endian.cairo | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/starknet/src/utils/endian.cairo b/starknet/src/utils/endian.cairo index b2cbb791c..cb70accc1 100644 --- a/starknet/src/utils/endian.cairo +++ b/starknet/src/utils/endian.cairo @@ -9,10 +9,10 @@ fn uint256_into_le_u64s(self: u256) -> (u64, u64, u64, u64) { let high_low = integer::u128_byte_reverse(self.high & MASK_LOW) / SHIFT_64; let high_high = integer::u128_byte_reverse(self.high & MASK_HIGH); ( + high_high.try_into().unwrap(), + high_low.try_into().unwrap(), low_high.try_into().unwrap(), low_low.try_into().unwrap(), - high_high.try_into().unwrap(), - high_low.try_into().unwrap() ) } @@ -25,7 +25,7 @@ fn into_le_u64_array(self: Array) -> (Array, u64) { let overflow = loop { match self.pop_front() { Option::Some(num) => { - let (low_high, low_low, high_high, high_low) = uint256_into_le_u64s(num); + let (high_high, high_low, low_high, low_low) = uint256_into_le_u64s(num); if self.len() == 0 { assert(low_high == 0, 'Final u256 overflows u64'); assert(low_low == 0, 'Final u256 overflows u64'); From 83d824865e1bbc36b30afe7cc1be6ca402d053db Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 12:57:55 +0100 Subject: [PATCH 08/11] refactor already finalized error message in vote --- starknet/src/space/space.cairo | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index f642426e2..74d72b8c3 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -311,8 +311,7 @@ mod Space { assert(timestamp < proposal.max_end_timestamp, 'Voting period has ended'); assert(timestamp >= proposal.start_timestamp, 'Voting period has not started'); assert( - proposal.finalization_status == FinalizationStatus::Pending(()), - 'Proposal has been finalized' + proposal.finalization_status == FinalizationStatus::Pending(()), 'Already finalized' ); assert( self._vote_registry.read((proposal_id, voter)) == false, 'Voter has already voted' From 35d439bbaca42a7ce431a633d1968553b4368884 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 13:06:26 +0100 Subject: [PATCH 09/11] chore: fix test --- starknet/src/tests/space/space.cairo | 2 +- starknet/src/tests/space/vote.cairo | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/starknet/src/tests/space/space.cairo b/starknet/src/tests/space/space.cairo index edf716d0d..da9c9ce29 100644 --- a/starknet/src/tests/space/space.cairo +++ b/starknet/src/tests/space/space.cairo @@ -654,7 +654,7 @@ mod tests { #[test] #[available_gas(10000000000)] - #[should_panic(expected: ('Proposal has been finalized', 'ENTRYPOINT_FAILED'))] + #[should_panic(expected: ('Already finalized', 'ENTRYPOINT_FAILED'))] fn cancel() { let relayer = starknet::contract_address_const::<0x1234>(); let config = setup(); diff --git a/starknet/src/tests/space/vote.cairo b/starknet/src/tests/space/vote.cairo index 5fcf68952..f7505fd86 100644 --- a/starknet/src/tests/space/vote.cairo +++ b/starknet/src/tests/space/vote.cairo @@ -256,7 +256,7 @@ mod tests { #[test] #[available_gas(10000000000)] - #[should_panic(expected: ('Proposal has been finalized', 'ENTRYPOINT_FAILED'))] + #[should_panic(expected: ('Already finalized', 'ENTRYPOINT_FAILED'))] fn vote_finalized_proposal() { let config = setup(); let (factory, space) = deploy(@config); From 1b5cceadb98a5a136bfbae7059f58d658f427676 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 16:49:12 +0100 Subject: [PATCH 10/11] refactor: remove explicit mut arr casts --- starknet/src/voting_strategies/merkle_whitelist.cairo | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/starknet/src/voting_strategies/merkle_whitelist.cairo b/starknet/src/voting_strategies/merkle_whitelist.cairo index eb01a9cd0..310d31463 100644 --- a/starknet/src/voting_strategies/merkle_whitelist.cairo +++ b/starknet/src/voting_strategies/merkle_whitelist.cairo @@ -4,8 +4,6 @@ mod MerkleWhitelistVotingStrategy { use sx::types::UserAddress; use sx::utils::{merkle, Leaf}; - const LEAF_SIZE: usize = 4; // Serde::::serialize().len() - #[storage] struct Storage {} @@ -29,12 +27,9 @@ mod MerkleWhitelistVotingStrategy { self: @ContractState, timestamp: u32, voter: UserAddress, - params: Span, // [root: felt252] - user_params: Span, // [leaf: Leaf, proof: Array] + mut params: Span, // [root: felt252] + mut user_params: Span, // [leaf: Leaf, proof: Array] ) -> u256 { - let mut params = params; - let mut user_params = user_params; - let root = Serde::::deserialize(ref params).unwrap(); let (leaf, proofs) = Serde::<(Leaf, Array)>::deserialize(ref user_params) .unwrap(); From 4a01e008382eacfb8663caa5cdd4c0c7655920d8 Mon Sep 17 00:00:00 2001 From: Orlando Date: Fri, 6 Oct 2023 16:49:50 +0100 Subject: [PATCH 11/11] refactor: explicit len offsets --- starknet/src/types/strategy.cairo | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/starknet/src/types/strategy.cairo b/starknet/src/types/strategy.cairo index 8e1532a1f..a99f4244b 100644 --- a/starknet/src/types/strategy.cairo +++ b/starknet/src/types/strategy.cairo @@ -44,7 +44,7 @@ impl StoreFelt252Array of Store> { // Read the stored array's length. If the length is superior to 255, the read will fail. let len: u8 = Store::::read_at_offset(address_domain, base, offset) .expect('Storage Span too large'); - offset += 1; + offset += Store::::size(); // Sequentially read all stored elements and append them to the array. let exit = len * Store::::size() + offset; @@ -68,7 +68,7 @@ impl StoreFelt252Array of Store> { // // Store the length of the array in the first storage slot. let len: u8 = value.len().try_into().expect('Storage - Span too large'); Store::::write_at_offset(address_domain, base, offset, len); - offset += 1; + offset += Store::::size(); // Store the array elements sequentially loop {