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

refactor: N-02 Code Clarity Suggestions #588

Merged
merged 11 commits into from
Oct 9, 2023
2 changes: 1 addition & 1 deletion starknet/src/authenticators/eth_tx.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
5 changes: 2 additions & 3 deletions starknet/src/space/space.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions starknet/src/types/strategy.cairo
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down Expand Up @@ -47,7 +47,7 @@ impl StoreFelt252Array of Store<Array<felt252>> {
offset += 1;

// Sequentially read all stored elements and append them to the array.
let exit = len + offset;
let exit = len * Store::<felt252>::size() + offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

I did this, what do you think 73a9269 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah thanks, that makes a lot of sense.

loop {
if offset >= exit {
break;
Expand Down Expand Up @@ -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()
}
}

4 changes: 2 additions & 2 deletions starknet/src/utils/default.cairo
Original file line number Diff line number Diff line change
@@ -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()
}
}
3 changes: 1 addition & 2 deletions starknet/src/utils/eip712.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions starknet/src/utils/endian.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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()
)
}

Expand All @@ -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');
Expand Down
13 changes: 5 additions & 8 deletions starknet/src/voting_strategies/merkle_whitelist.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,12 @@ mod MerkleWhitelistVotingStrategy {
params: Span<felt252>, // [root: felt252]
user_params: Span<felt252>, // [leaf: Leaf, proof: Array<felt252>]
) -> u256 {
let cache = user_params; // cache
let mut params = params;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add mut keywrods to the params and user_params arguments directly, no need to re-bind :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice, updated. think there some other situations where we can do this. will have a look

let mut user_params = user_params;

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());
Expand Down