refactor: N-02 Code Clarity Suggestions (#588)

* refactor: explicit felt size when reading arrays * refactor: use bounded u8 max impl * refactor zeroable impl for zero address * refactor: remove redundant addition * refactor: param deserialization * refactor: use is non zero impl * refactor: systematic u64s order * refactor already finalized error message in vote * chore: fix test * refactor: remove explicit mut arr casts * refactor: explicit len offsets --------- Co-authored-by: Orlando <[email protected]>
snapshot-labs · Oct 9, 2023 · 16742df · 16742df
1 parent db58293
commit 16742df
Show file tree

Hide file tree

Showing 9 changed files with 22 additions and 32 deletions.
diff --git 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
@@ -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'
@@ -787,7 +786,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

diff --git 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
@@ -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);

diff --git a/starknet/src/types/strategy.cairo b/starknet/src/types/strategy.cairo
@@ -1,5 +1,5 @@
-use starknet::ContractAddress;
-use starknet::{StorageBaseAddress, Store, SyscallResult};
+use integer::BoundedU8;
+use starknet::{ContractAddress, StorageBaseAddress, Store, SyscallResult};
 
 /// A strategy identified by an address
 #[derive(Clone, Drop, Option, Serde, starknet::Store)]
@@ -44,10 +44,10 @@ impl StoreFelt252Array of Store<Array<felt252>> {
         // Read the stored array's length. If the length is superior to 255, the read will fail.
         let len: u8 = Store::<u8>::read_at_offset(address_domain, base, offset)
             .expect('Storage Span too large');
-        offset += 1;
+        offset += Store::<u8>::size();
 
         // Sequentially read all stored elements and append them to the array.
-        let exit = len + offset;
+        let exit = len * Store::<felt252>::size() + offset;
         loop {
             if offset >= exit {
                 break;
@@ -68,7 +68,7 @@ impl StoreFelt252Array of Store<Array<felt252>> {
         // // Store the length of the array in the first storage slot.
         let len: u8 = value.len().try_into().expect('Storage - Span too large');
         Store::<u8>::write_at_offset(address_domain, base, offset, len);
-        offset += 1;
+        offset += Store::<u8>::size();
 
         // Store the array elements sequentially
         loop {
@@ -91,7 +91,7 @@ impl StoreFelt252Array of Store<Array<felt252>> {
 
     fn size() -> u8 {
         /// Since the array is a dynamic type. We use its max size here. 
-        255_u8
+        BoundedU8::max()
     }
 }
 
diff --git 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<ContractAddress> {
     fn default() -> ContractAddress {
-        contract_address_const::<0>()
+        Zeroable::zero()
     }
 }
diff --git 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;

diff --git 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<u256>) -> (Array<u64>, 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');

diff --git 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::<Leaf>::serialize().len()
-
     #[storage]
     struct Storage {}
 
@@ -29,18 +27,12 @@ mod MerkleWhitelistVotingStrategy {
             self: @ContractState,
             timestamp: u32,
             voter: UserAddress,
-            params: Span<felt252>, // [root: felt252]
-            user_params: Span<felt252>, // [leaf: Leaf, proof: Array<felt252>]
+            mut params: Span<felt252>, // [root: felt252]
+            mut user_params: Span<felt252>, // [leaf: Leaf, proof: Array<felt252>]
         ) -> u256 {
-            let cache = user_params; // cache
-
-            let mut leaf_raw = cache.slice(0, LEAF_SIZE);
-            let leaf = Serde::<Leaf>::deserialize(ref leaf_raw).unwrap();
-
-            let mut proofs_raw = cache.slice(LEAF_SIZE, cache.len() - LEAF_SIZE);
-            let proofs = Serde::<Array<felt252>>::deserialize(ref proofs_raw).unwrap();
-
-            let root = *params.at(0); // no need to deserialize because it's a simple value
+            let root = Serde::<felt252>::deserialize(ref params).unwrap();
+            let (leaf, proofs) = Serde::<(Leaf, Array<felt252>)>::deserialize(ref user_params)
+                .unwrap();
 
             assert(leaf.address == voter, 'Leaf and voter mismatch');
             merkle::assert_valid_proof(root, leaf, proofs.span());