From 6f024459ec489a3793e946e5dc30e658e643a87b Mon Sep 17 00:00:00 2001 From: Orland0x <37511817+Orland0x@users.noreply.github.com> Date: Wed, 23 Aug 2023 11:35:07 -0400 Subject: [PATCH 1/8] feat: prevent zero address for author/voter (#492) * revert if autauthor/voter is zero address * chore: test propose with zero starknet and ethereum address * chore: test propose with zero custom address * chore: UserAddress zeroable tests * chore: zero address untegration tests vote and update proposal * chore: updated scarb version to 0.6.2 in CI * chore: attempt fix CI * chore: formatting * chore: fix CI 2 * chore: formatting * chore: fixed space test * chore: add code coverage for zero/non_zero --------- Co-authored-by: Orlando Co-authored-by: Scott Piriou <30843220+pscott@users.noreply.github.com> --- .github/workflows/test.yml | 2 +- starknet/src/space/space.cairo | 3 + starknet/src/tests/test_space.cairo | 164 ++++++++++++++++++++++++++ starknet/src/types/user_address.cairo | 64 ++++++++++ 4 files changed, 232 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a2b69666..ca69722d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -54,7 +54,7 @@ jobs: - name: Step 2 - Install Scarb uses: software-mansion/setup-scarb@v1 with: - scarb-version: 0.6.0-alpha.1 + scarb-version: 0.6.2 - name: Step 3 - Check formatting working-directory: ./starknet diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 0e9ee514..3e8732e4 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -257,6 +257,7 @@ mod Space { metadata_URI: Array, ) { assert_only_authenticator(@self); + assert(author.is_non_zero(), 'Zero Address'); let proposal_id = self._next_proposal_id.read(); // Proposal Validation @@ -312,6 +313,7 @@ mod Space { metadata_URI: Array ) { assert_only_authenticator(@self); + assert(voter.is_non_zero(), 'Zero Address'); let proposal = self._proposals.read(proposal_id); assert_proposal_exists(@proposal); @@ -377,6 +379,7 @@ mod Space { metadata_URI: Array, ) { assert_only_authenticator(@self); + assert(author.is_non_zero(), 'Zero Address'); let mut proposal = self._proposals.read(proposal_id); assert_proposal_exists(@proposal); assert(proposal.author == author, 'Only Author'); diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 1c36f42a..7e66afc0 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -405,4 +405,168 @@ mod tests { ArrayTrait::::new().serialize(ref vote_calldata); authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); } + + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))] + fn test_propose_zero_address() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let quorum = u256_from_felt252(1); + let mut constructor_calldata = ArrayTrait::::new(); + quorum.serialize(ref constructor_calldata); + + let (vanilla_execution_strategy_address, _) = deploy_syscall( + VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + constructor_calldata.span(), + false + ) + .unwrap(); + let vanilla_execution_strategy = StrategyImpl::from_address( + vanilla_execution_strategy_address + ); + // author is the zero address + let author = UserAddress::Starknet(contract_address_const::<0x0>()); + let mut propose_calldata = array::ArrayTrait::::new(); + author.serialize(ref propose_calldata); + vanilla_execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + } + + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))] + fn test_update_zero_address() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let quorum = u256_from_felt252(1); + let mut constructor_calldata = ArrayTrait::::new(); + quorum.serialize(ref constructor_calldata); + + let (vanilla_execution_strategy_address, _) = deploy_syscall( + VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + constructor_calldata.span(), + false + ) + .unwrap(); + let vanilla_execution_strategy = StrategyImpl::from_address( + vanilla_execution_strategy_address + ); + // author is the zero address + let author = UserAddress::Starknet(contract_address_const::<0x0>()); + let mut propose_calldata = array::ArrayTrait::::new(); + author.serialize(ref propose_calldata); + vanilla_execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + + // Update Proposal + let mut update_calldata = array::ArrayTrait::::new(); + author.serialize(ref update_calldata); + let proposal_id = u256_from_felt252(1); + proposal_id.serialize(ref update_calldata); + // Keeping the same execution strategy contract but changing the payload + let mut new_payload = ArrayTrait::::new(); + new_payload.append(1); + let execution_strategy = Strategy { + address: vanilla_execution_strategy.address, params: new_payload + }; + execution_strategy.serialize(ref update_calldata); + ArrayTrait::::new().serialize(ref update_calldata); + + authenticator + .authenticate(space.contract_address, UPDATE_PROPOSAL_SELECTOR, update_calldata); + } + + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))] + fn test_vote_zero_address() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let quorum = u256_from_felt252(1); + let mut constructor_calldata = ArrayTrait::::new(); + quorum.serialize(ref constructor_calldata); + + let (vanilla_execution_strategy_address, _) = deploy_syscall( + VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + constructor_calldata.span(), + false + ) + .unwrap(); + let vanilla_execution_strategy = StrategyImpl::from_address( + vanilla_execution_strategy_address + ); + let author = UserAddress::Starknet(contract_address_const::<0x5678>()); + let mut propose_calldata = array::ArrayTrait::::new(); + author.serialize(ref propose_calldata); + vanilla_execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + + // Update Proposal + let mut update_calldata = array::ArrayTrait::::new(); + author.serialize(ref update_calldata); + let proposal_id = u256_from_felt252(1); + proposal_id.serialize(ref update_calldata); + // Keeping the same execution strategy contract but changing the payload + let mut new_payload = ArrayTrait::::new(); + new_payload.append(1); + let execution_strategy = Strategy { + address: vanilla_execution_strategy.address, params: new_payload + }; + execution_strategy.serialize(ref update_calldata); + ArrayTrait::::new().serialize(ref update_calldata); + + authenticator + .authenticate(space.contract_address, UPDATE_PROPOSAL_SELECTOR, update_calldata); + + // Increasing block block_number by 1 to pass voting delay + testing::set_block_number(1_u64); + + let mut vote_calldata = array::ArrayTrait::::new(); + // Voter is the zero address + let voter = UserAddress::Starknet(contract_address_const::<0x0>()); + voter.serialize(ref vote_calldata); + let proposal_id = u256_from_felt252(1); + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = ArrayTrait::::new(); + user_voting_strategies + .append(IndexedStrategy { index: 0_u8, params: ArrayTrait::::new() }); + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + // Vote on Proposal + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + } } diff --git a/starknet/src/types/user_address.cairo b/starknet/src/types/user_address.cairo index 1066eb96..69bfb177 100644 --- a/starknet/src/types/user_address.cairo +++ b/starknet/src/types/user_address.cairo @@ -1,5 +1,6 @@ use starknet::{ContractAddress, EthAddress}; use traits::{PartialEq, TryInto, Into}; +use zeroable::Zeroable; use serde::Serde; use array::ArrayTrait; use sx::utils::legacy_hash::LegacyHashUserAddress; @@ -57,3 +58,66 @@ impl UserAddressImpl of UserAddressTrait { } } } + +impl UserAddressZeroable of Zeroable { + fn zero() -> UserAddress { + panic_with_felt252('Undefined') + } + fn is_zero(self: UserAddress) -> bool { + match self { + UserAddress::Starknet(address) => address.is_zero(), + UserAddress::Ethereum(address) => address.is_zero(), + UserAddress::Custom(address) => address.is_zero(), + } + } + fn is_non_zero(self: UserAddress) -> bool { + !self.is_zero() + } +} + +#[cfg(test)] +mod tests { + use zeroable::Zeroable; + use super::{UserAddress, UserAddressZeroable}; + use starknet::{EthAddress, contract_address_const}; + + #[test] + fn test_is_zero() { + assert(UserAddress::Starknet(contract_address_const::<0>()).is_zero(), 'is not zero'); + assert(UserAddress::Ethereum(EthAddress { address: 0 }).is_zero(), 'is not zero'); + assert(UserAddress::Custom(0_u256).is_zero(), 'is not zero'); + } + + #[test] + fn test_is_zero_false_positive() { + assert( + UserAddress::Starknet(contract_address_const::<1>()).is_zero() == false, + 'false positive not zero' + ); + assert( + UserAddress::Ethereum(EthAddress { address: 1 }).is_zero() == false, + 'false positive not zero' + ); + assert(UserAddress::Custom(1_u256).is_zero() == false, 'false positive not zero'); + } + + #[test] + fn test_is_non_zero() { + assert(UserAddress::Starknet(contract_address_const::<1>()).is_non_zero(), 'is zero'); + assert(UserAddress::Ethereum(EthAddress { address: 1 }).is_non_zero(), 'is zero'); + assert(UserAddress::Custom(1_u256).is_non_zero(), 'is zero'); + } + + #[test] + fn test_is_non_zero_false_positive() { + assert( + UserAddress::Starknet(contract_address_const::<0>()).is_non_zero() == false, + 'false positive is zero' + ); + assert( + UserAddress::Ethereum(EthAddress { address: 0 }).is_non_zero() == false, + 'false positive is zero' + ); + assert(UserAddress::Custom(0_u256).is_non_zero() == false, 'false positve not zero'); + } +} From ab5657b0526683a6cceac5642e10bad80bb81e6f Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Wed, 23 Aug 2023 17:36:47 +0200 Subject: [PATCH 2/8] ignore VSCodeCounter (#507) --- .gitignore | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 6f806f12..8ea44149 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,6 @@ out/ !/broadcast /broadcast/*/31337/ -/broadcast/**/dry-run/ \ No newline at end of file +/broadcast/**/dry-run/ + +.VSCodeCounter \ No newline at end of file From d243d3c8db3a51fa4de433905eaf1af7eebec7ed Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Wed, 23 Aug 2023 17:41:54 +0200 Subject: [PATCH 3/8] remove needless type annotation (#512) --- starknet/src/execution_strategies/vanilla.cairo | 3 +-- starknet/src/space/space.cairo | 12 ++++++------ starknet/src/tests/mocks/space_v2.cairo | 3 +-- starknet/src/voting_strategies/eth_balance_of.cairo | 6 ++---- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/starknet/src/execution_strategies/vanilla.cairo b/starknet/src/execution_strategies/vanilla.cairo index 0766a6a9..8a52616c 100644 --- a/starknet/src/execution_strategies/vanilla.cairo +++ b/starknet/src/execution_strategies/vanilla.cairo @@ -38,8 +38,7 @@ mod VanillaExecutionStrategy { #[constructor] fn constructor(ref self: ContractState, quorum: u256) { // TODO: temporary until components are released - let mut state: SimpleQuorumExecutionStrategy::ContractState = - SimpleQuorumExecutionStrategy::unsafe_new_contract_state(); + let mut state = SimpleQuorumExecutionStrategy::unsafe_new_contract_state(); SimpleQuorumExecutionStrategy::initializer(ref state, quorum); } diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 3e8732e4..0807045e 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -238,7 +238,7 @@ mod Space { ReinitializableImpl::initialize(ref state); //TODO: temporary component syntax - let mut state: Ownable::ContractState = Ownable::unsafe_new_contract_state(); + let mut state = Ownable::unsafe_new_contract_state(); Ownable::initializer(ref state); Ownable::transfer_ownership(ref state, owner); _set_min_voting_duration(ref self, min_voting_duration); @@ -401,7 +401,7 @@ mod Space { fn cancel_proposal(ref self: ContractState, proposal_id: u256) { //TODO: temporary component syntax - let state: Ownable::ContractState = Ownable::unsafe_new_contract_state(); + let state = Ownable::unsafe_new_contract_state(); Ownable::assert_only_owner(@state); let mut proposal = self._proposals.read(proposal_id); assert_proposal_exists(@proposal); @@ -437,7 +437,7 @@ mod Space { fn owner(self: @ContractState) -> ContractAddress { //TODO: temporary component syntax - let state: Ownable::ContractState = Ownable::unsafe_new_contract_state(); + let state = Ownable::unsafe_new_contract_state(); Ownable::owner(@state) } @@ -484,7 +484,7 @@ mod Space { fn update_settings(ref self: ContractState, input: UpdateSettingsCalldata) { //TODO: temporary component syntax - let state: Ownable::ContractState = Ownable::unsafe_new_contract_state(); + let state = Ownable::unsafe_new_contract_state(); Ownable::assert_only_owner(@state); // if not NO_UPDATE @@ -552,14 +552,14 @@ mod Space { fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { //TODO: temporary component syntax - let mut state: Ownable::ContractState = Ownable::unsafe_new_contract_state(); + let mut state = Ownable::unsafe_new_contract_state(); Ownable::assert_only_owner(@state); Ownable::transfer_ownership(ref state, new_owner); } fn renounce_ownership(ref self: ContractState) { //TODO: temporary component syntax - let mut state: Ownable::ContractState = Ownable::unsafe_new_contract_state(); + let mut state = Ownable::unsafe_new_contract_state(); Ownable::assert_only_owner(@state); Ownable::renounce_ownership(ref state); } diff --git a/starknet/src/tests/mocks/space_v2.cairo b/starknet/src/tests/mocks/space_v2.cairo index 702d1b50..2d40018b 100644 --- a/starknet/src/tests/mocks/space_v2.cairo +++ b/starknet/src/tests/mocks/space_v2.cairo @@ -18,8 +18,7 @@ mod SpaceV2 { impl SpaceV2 of ISpaceV2 { fn initialize(ref self: ContractState, var: felt252) { // TODO: Temp component syntax - let mut state: Reinitializable::ContractState = - Reinitializable::unsafe_new_contract_state(); + let mut state = Reinitializable::unsafe_new_contract_state(); ReinitializableImpl::initialize(ref state); self._var.write(var); } diff --git a/starknet/src/voting_strategies/eth_balance_of.cairo b/starknet/src/voting_strategies/eth_balance_of.cairo index a8138e8d..e060e0c1 100644 --- a/starknet/src/voting_strategies/eth_balance_of.cairo +++ b/starknet/src/voting_strategies/eth_balance_of.cairo @@ -27,8 +27,7 @@ mod EthBalanceOfVotingStrategy { let slot_index = (*params[1]).into(); // TODO: temporary until components are released - let state: SingleSlotProof::ContractState = - SingleSlotProof::unsafe_new_contract_state(); + let state = SingleSlotProof::unsafe_new_contract_state(); // Get the balance of the voter at the given block timestamp let balance = SingleSlotProof::get_storage_slot( @@ -43,8 +42,7 @@ mod EthBalanceOfVotingStrategy { ref self: ContractState, facts_registry: ContractAddress, l1_headers_store: ContractAddress ) { // TODO: temporary until components are released - let mut state: SingleSlotProof::ContractState = - SingleSlotProof::unsafe_new_contract_state(); + let mut state = SingleSlotProof::unsafe_new_contract_state(); SingleSlotProof::initializer(ref state, facts_registry); } } From 1174181cc3a9e7d2da2c8b407b7f63254788f5fe Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Fri, 25 Aug 2023 09:48:26 +0200 Subject: [PATCH 4/8] Fix erc20 proposition strategy (#511) * rename block_number to timestamp in proposition_power.cairo * use timestamp - 1 for proposing power validation strategy --- .../proposing_power.cairo | 96 ++++++++++++++++++- starknet/src/types/proposal.cairo | 1 - starknet/src/utils/proposition_power.cairo | 6 +- 3 files changed, 97 insertions(+), 6 deletions(-) diff --git a/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo b/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo index d73600f5..b587e4a6 100644 --- a/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo +++ b/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo @@ -23,10 +23,20 @@ mod tests { use sx::tests::test_merkle_whitelist::merkle_utils::{ generate_merkle_data, generate_merkle_root, generate_proof }; + use sx::tests::mocks::erc20_votes_preset::{ERC20VotesPreset}; + use openzeppelin::token::erc20::interface::{IERC20Dispatcher, IERC20DispatcherTrait}; + use openzeppelin::governance::utils::interfaces::votes::{ + IVotes, IVotesDispatcher, IVotesDispatcherTrait + }; + use traits::Into; + use sx::voting_strategies::erc20_votes::ERC20VotesVotingStrategy; + use starknet::ContractAddress; - // #[test] - // #[available_gas(10000000000)] + #[test] + #[available_gas(10000000000)] fn test_vanilla_works() { + starknet::testing::set_block_timestamp(1); + // deploy vanilla voting strategy let (vanilla_contract, _) = deploy_syscall( VanillaVotingStrategy::TEST_CLASS_HASH.try_into().unwrap(), 0, array![].span(), false @@ -98,6 +108,8 @@ mod tests { #[test] #[available_gas(10000000000)] fn test_merkle_whitelist_works() { + starknet::testing::set_block_timestamp(1); + // deploy merkle whitelist contract let (merkle_contract, _) = deploy_syscall( MerkleWhitelistVotingStrategy::TEST_CLASS_HASH.try_into().unwrap(), @@ -233,4 +245,84 @@ mod tests { let is_validated = contract.validate(author, params.clone(), user_params.clone()); assert(!is_validated, 'should not have enough VP'); } + + fn strategy_from_contract(token_contract: ContractAddress) -> Strategy { + let (contract, _) = deploy_syscall( + ERC20VotesVotingStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + array![].span(), + false, + ) + .unwrap(); + + let params: Array = array![token_contract.into()]; + + Strategy { address: contract, params, } + } + + #[test] + #[available_gas(10000000000)] + fn test_erc20votes_works() { + let SUPPLY = 100_u256; + + // deploy erc20 voting strategy + let owner = contract_address_const::<'owner'>(); + let mut constructor = array!['TEST', 'TST']; + SUPPLY.serialize(ref constructor); + owner.serialize(ref constructor); + + let (erc20_contract, _) = deploy_syscall( + ERC20VotesPreset::TEST_CLASS_HASH.try_into().unwrap(), 0, constructor.span(), false + ) + .unwrap(); + + let erc20 = IVotesDispatcher { contract_address: erc20_contract, }; + + let erc20_strategy = strategy_from_contract(erc20_contract); + + starknet::testing::set_contract_address(owner); + erc20.delegate(owner); + + // advance block timestamp + starknet::testing::set_block_timestamp(1); + + // create a proposal validation strategy + let (proposal_validation_contract, _) = deploy_syscall( + ProposingPowerProposalValidationStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + array![].span(), + false + ) + .unwrap(); + + let allowed_strategies = array![erc20_strategy.clone()]; + let mut params = array![]; + let proposal_threshold = SUPPLY; + proposal_threshold.serialize(ref params); + allowed_strategies.serialize(ref params); + + // used strategies + let used_strategy = IndexedStrategy { index: 0, params: array![], }; + let used_strategies = array![used_strategy.clone()]; + let mut user_params = array![]; + used_strategies.serialize(ref user_params); + + let contract = IProposalValidationStrategyDispatcher { + contract_address: proposal_validation_contract, + }; + + let author = UserAddress::Starknet(owner); + + let is_validated = contract.validate(author, params, user_params.clone()); + assert(is_validated, 'not enough VP'); + + // Now increase threshold + let proposal_threshold = SUPPLY + 1; + let mut params = array![]; + proposal_threshold.serialize(ref params); + allowed_strategies.serialize(ref params); + + let is_validated = contract.validate(author, params.clone(), user_params); + assert(!is_validated, 'Threshold should not be reached'); + } } diff --git a/starknet/src/types/proposal.cairo b/starknet/src/types/proposal.cairo index d6ce917c..9747b451 100644 --- a/starknet/src/types/proposal.cairo +++ b/starknet/src/types/proposal.cairo @@ -8,7 +8,6 @@ use sx::utils::math::pow; use traits::{Into, TryInto}; use sx::types::{FinalizationStatus, UserAddress}; use option::OptionTrait; -use debug::PrintTrait; const BITMASK_32: u128 = 0xffffffff; const BITMASK_64: u128 = 0xffffffffffffffff; diff --git a/starknet/src/utils/proposition_power.cairo b/starknet/src/utils/proposition_power.cairo index e7361d4d..4da92af3 100644 --- a/starknet/src/utils/proposition_power.cairo +++ b/starknet/src/utils/proposition_power.cairo @@ -14,7 +14,7 @@ use clone::Clone; fn _get_cumulative_power( voter: UserAddress, - block_number: u32, + timestamp: u32, mut user_strategies: Array, allowed_strategies: Array, ) -> u256 { @@ -30,7 +30,7 @@ fn _get_cumulative_power( contract_address: strategy.address } .get_voting_power( - block_number, voter, strategy.params, indexed_strategy.params, + timestamp, voter, strategy.params, indexed_strategy.params, ); }, Option::None => { @@ -60,7 +60,7 @@ fn _validate( let user_strategies = Serde::>::deserialize(ref user_params_span) .unwrap(); - let timestamp: u32 = info::get_block_timestamp().try_into().unwrap(); + let timestamp: u32 = info::get_block_timestamp().try_into().unwrap() - 1; let voting_power = _get_cumulative_power( author, timestamp, user_strategies, allowed_strategies ); From af2c1e61a7d0b2b992839e2c927e5dd1cf3a33da Mon Sep 17 00:00:00 2001 From: Orland0x <37511817+Orland0x@users.noreply.github.com> Date: Fri, 25 Aug 2023 06:14:16 -0400 Subject: [PATCH 5/8] fix: event syntax (#517) * refactor: updated events syntax in space * refactor: updated events syntax in factory --------- Co-authored-by: Orlando Co-authored-by: Scott Piriou <30843220+pscott@users.noreply.github.com> --- starknet/src/factory/factory.cairo | 17 +- starknet/src/space/space.cairo | 369 ++++++++++++++++++++--------- 2 files changed, 275 insertions(+), 111 deletions(-) diff --git a/starknet/src/factory/factory.cairo b/starknet/src/factory/factory.cairo index 855327aa..a3d9ceb0 100644 --- a/starknet/src/factory/factory.cairo +++ b/starknet/src/factory/factory.cairo @@ -4,7 +4,7 @@ use starknet::ClassHash; #[starknet::interface] trait IFactory { fn deploy( - self: @TContractState, + ref self: TContractState, class_hash: ClassHash, contract_address_salt: felt252, initialize_calldata: Span @@ -24,7 +24,16 @@ mod Factory { use sx::utils::constants::INITIALIZE_SELECTOR; #[event] - fn SpaceDeployed(class_hash: ClassHash, space_address: ContractAddress) {} + #[derive(Drop, starknet::Event)] + enum Event { + SpaceDeployed: SpaceDeployed + } + + #[derive(Drop, starknet::Event)] + struct SpaceDeployed { + class_hash: ClassHash, + space_address: ContractAddress + } #[storage] struct Storage {} @@ -32,7 +41,7 @@ mod Factory { #[external(v0)] impl Factory of IFactory { fn deploy( - self: @ContractState, + ref self: ContractState, class_hash: ClassHash, contract_address_salt: felt252, initialize_calldata: Span @@ -46,7 +55,7 @@ mod Factory { call_contract_syscall(space_address, INITIALIZE_SELECTOR, initialize_calldata) .unwrap_syscall(); - SpaceDeployed(class_hash, space_address); + self.emit(Event::SpaceDeployed(SpaceDeployed { class_hash, space_address })); space_address } diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 0807045e..baaf6da6 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -83,6 +83,7 @@ mod Space { use option::OptionTrait; use hash::LegacyHash; use traits::{Into, TryInto}; + use serde::Serde; use sx::interfaces::{ IProposalValidationStrategyDispatcher, IProposalValidationStrategyDispatcherTrait, @@ -118,88 +119,135 @@ mod Space { } #[event] - fn SpaceCreated( - _space: ContractAddress, - _owner: ContractAddress, - _min_voting_duration: u32, - _max_voting_duration: u32, - _voting_delay: u32, - _proposal_validation_strategy: @Strategy, - _proposal_validation_strategy_metadata_URI: @Array, - _voting_strategies: @Array, - _voting_strategy_metadata_URIs: @Array>, - _authenticators: @Array, - _metadata_URI: @Array, - _dao_URI: @Array, - ) {} + #[derive(Drop, starknet::Event)] + enum Event { + SpaceCreated: SpaceCreated, + ProposalCreated: ProposalCreated, + VoteCast: VoteCast, + ProposalExecuted: ProposalExecuted, + ProposalUpdated: ProposalUpdated, + ProposalCancelled: ProposalCancelled, + VotingStrategiesAdded: VotingStrategiesAdded, + VotingStrategiesRemoved: VotingStrategiesRemoved, + AuthenticatorsAdded: AuthenticatorsAdded, + AuthenticatorsRemoved: AuthenticatorsRemoved, + MetadataUriUpdated: MetadataUriUpdated, + DaoUriUpdated: DaoUriUpdated, + MaxVotingDurationUpdated: MaxVotingDurationUpdated, + MinVotingDurationUpdated: MinVotingDurationUpdated, + ProposalValidationStrategyUpdated: ProposalValidationStrategyUpdated, + VotingDelayUpdated: VotingDelayUpdated, + Upgraded: Upgraded, + } - #[event] - fn ProposalCreated( - _proposal_id: u256, - _author: UserAddress, - _proposal: @Proposal, - _payload: @Array, - _metadata_URI: @Array - ) {} + #[derive(Drop, starknet::Event)] + struct SpaceCreated { + space: ContractAddress, + owner: ContractAddress, + min_voting_duration: u32, + max_voting_duration: u32, + voting_delay: u32, + proposal_validation_strategy: Strategy, + proposal_validation_strategy_metadata_URI: Span, + voting_strategies: Span, + voting_strategy_metadata_URIs: Span>, + authenticators: Span, + metadata_URI: Span, + dao_URI: Span, + } - #[event] - fn VoteCast( - _proposal_id: u256, - _voter: UserAddress, - _choice: Choice, - _voting_power: u256, - _metadata_URI: @Array - ) {} + #[derive(Drop, starknet::Event)] + struct ProposalCreated { + proposal_id: u256, + author: UserAddress, + proposal: Proposal, + payload: Span, + metadata_URI: Span, + } - #[event] - fn ProposalExecuted(_proposal_id: u256) {} + #[derive(Drop, starknet::Event)] + struct VoteCast { + proposal_id: u256, + voter: UserAddress, + choice: Choice, + voting_power: u256, + metadata_URI: Span, + } - #[event] - fn ProposalUpdated( - _proposal_id: u256, _execution_stategy: @Strategy, _metadata_URI: @Array - ) {} + #[derive(Drop, starknet::Event)] + struct ProposalExecuted { + proposal_id: u256, + } - #[event] - fn ProposalCancelled(_proposal_id: u256) {} + #[derive(Drop, starknet::Event)] + struct ProposalUpdated { + proposal_id: u256, + execution_strategy: Strategy, + metadata_URI: Span, + } - #[event] - fn VotingStrategiesAdded( - _new_voting_strategies: @Array, - _new_voting_strategy_metadata_URIs: @Array> - ) {} + #[derive(Drop, starknet::Event)] + struct ProposalCancelled { + proposal_id: u256, + } - #[event] - fn VotingStrategiesRemoved(_voting_strategy_indices: @Array) {} + #[derive(Drop, starknet::Event)] + struct VotingStrategiesAdded { + voting_strategies: Span, + voting_strategy_metadata_URIs: Span>, + } - #[event] - fn AuthenticatorsAdded(_new_authenticators: @Array) {} + #[derive(Drop, starknet::Event)] + struct VotingStrategiesRemoved { + voting_strategy_indices: Span, + } - #[event] - fn AuthenticatorsRemoved(_authenticators: @Array) {} + #[derive(Drop, starknet::Event)] + struct AuthenticatorsAdded { + authenticators: Span, + } - #[event] - fn MetadataURIUpdated(_new_metadata_URI: @Array) {} + #[derive(Drop, starknet::Event)] + struct AuthenticatorsRemoved { + authenticators: Span, + } - #[event] - fn DaoURIUpdated(_new_dao_URI: @Array) {} + #[derive(Drop, starknet::Event)] + struct MaxVotingDurationUpdated { + max_voting_duration: u32, + } - #[event] - fn MaxVotingDurationUpdated(_new_max_voting_duration: u32) {} + #[derive(Drop, starknet::Event)] + struct MinVotingDurationUpdated { + min_voting_duration: u32, + } - #[event] - fn MinVotingDurationUpdated(_new_min_voting_duration: u32) {} + #[derive(Drop, starknet::Event)] + struct ProposalValidationStrategyUpdated { + proposal_validation_strategy: Strategy, + proposal_validation_strategy_metadata_URI: Span, + } - #[event] - fn ProposalValidationStrategyUpdated( - _new_proposal_validation_strategy: @Strategy, - _new_proposal_validation_strategy_metadata_URI: @Array - ) {} + #[derive(Drop, starknet::Event)] + struct VotingDelayUpdated { + voting_delay: u32, + } - #[event] - fn VotingDelayUpdated(_new_voting_delay: u32) {} + #[derive(Drop, starknet::Event)] + struct Upgraded { + class_hash: ClassHash, + initialize_calldata: Span, + } - #[event] - fn Upgraded(class_hash: ClassHash, initialize_calldata: Array) {} + #[derive(Drop, starknet::Event)] + struct MetadataUriUpdated { + metadata_URI: Span, + } + + #[derive(Drop, starknet::Event)] + struct DaoUriUpdated { + dao_URI: Span, + } #[external(v0)] impl Space of ISpace { @@ -217,20 +265,27 @@ mod Space { metadata_URI: Array, dao_URI: Array, ) { - SpaceCreated( - info::get_contract_address(), - owner, - min_voting_duration, - max_voting_duration, - voting_delay, - @proposal_validation_strategy, - @proposal_validation_strategy_metadata_URI, - @voting_strategies, - @voting_strategy_metadata_URIs, - @authenticators, - @metadata_URI, - @dao_URI - ); + self + .emit( + Event::SpaceCreated( + SpaceCreated { + space: info::get_contract_address(), + owner: owner, + min_voting_duration: min_voting_duration, + max_voting_duration: max_voting_duration, + voting_delay: voting_delay, + proposal_validation_strategy: proposal_validation_strategy.clone(), + proposal_validation_strategy_metadata_URI: proposal_validation_strategy_metadata_URI + .span(), + voting_strategies: voting_strategies.span(), + voting_strategy_metadata_URIs: voting_strategy_metadata_URIs.span(), + authenticators: authenticators.span(), + metadata_URI: metadata_URI.span(), + dao_URI: dao_URI.span() + } + ) + ); + // Checking that the contract is not already initialized //TODO: temporary component syntax (see imports too) let mut state: Reinitializable::ContractState = @@ -292,16 +347,25 @@ mod Space { finalization_status: FinalizationStatus::Pending(()), active_voting_strategies: self._active_voting_strategies.read() }; - let snap_proposal = @proposal; + let clone_proposal = proposal.clone(); // TODO: Lots of copying, maybe figure out how to pass snapshots to events/storage writers. self._proposals.write(proposal_id, proposal); self._next_proposal_id.write(proposal_id + 1_u256); - ProposalCreated( - proposal_id, author, snap_proposal, @execution_strategy.params, @metadata_URI - ); + self + .emit( + Event::ProposalCreated( + ProposalCreated { + proposal_id: proposal_id, + author: author, + proposal: clone_proposal, + payload: execution_strategy.params.span(), + metadata_URI: metadata_URI.span() + } + ) + ); } fn vote( @@ -346,7 +410,18 @@ mod Space { ); self._vote_registry.write((proposal_id, voter), true); - VoteCast(proposal_id, voter, choice, voting_power, @metadata_URI); + self + .emit( + Event::VoteCast( + VoteCast { + proposal_id: proposal_id, + voter: voter, + choice: choice, + voting_power: voting_power, + metadata_URI: metadata_URI.span() + } + ) + ); } fn execute(ref self: ContractState, proposal_id: u256, execution_payload: Array) { @@ -368,7 +443,7 @@ mod Space { self._proposals.write(proposal_id, proposal); - ProposalExecuted(proposal_id); + self.emit(Event::ProposalExecuted(ProposalExecuted { proposal_id: proposal_id })); } fn update_proposal( @@ -396,7 +471,16 @@ mod Space { self._proposals.write(proposal_id, proposal); - ProposalUpdated(proposal_id, @execution_strategy, @metadata_URI); + self + .emit( + Event::ProposalUpdated( + ProposalUpdated { + proposal_id: proposal_id, + execution_strategy: execution_strategy, + metadata_URI: metadata_URI.span() + } + ) + ); } fn cancel_proposal(ref self: ContractState, proposal_id: u256) { @@ -410,7 +494,8 @@ mod Space { ); proposal.finalization_status = FinalizationStatus::Cancelled(()); self._proposals.write(proposal_id, proposal); - ProposalCancelled(proposal_id); + + self.emit(Event::ProposalCancelled(ProposalCancelled { proposal_id: proposal_id })); } fn upgrade( @@ -432,7 +517,15 @@ mod Space { info::get_contract_address(), INITIALIZE_SELECTOR, initialize_calldata.span() ) .unwrap_syscall(); - Upgraded(class_hash, initialize_calldata); + + self + .emit( + Event::Upgraded( + Upgraded { + class_hash: class_hash, initialize_calldata: initialize_calldata.span() + } + ) + ); } fn owner(self: @ContractState) -> ContractAddress { @@ -477,7 +570,6 @@ mod Space { self._proposal_validation_strategy.read() } - fn proposals(self: @ContractState, proposal_id: u256) -> Proposal { self._proposals.read(proposal_id) } @@ -490,63 +582,126 @@ mod Space { // if not NO_UPDATE if NoUpdateU32::should_update(@input.max_voting_duration) { _set_max_voting_duration(ref self, input.max_voting_duration); - MaxVotingDurationUpdated(input.max_voting_duration); + + self + .emit( + Event::MaxVotingDurationUpdated( + MaxVotingDurationUpdated { + max_voting_duration: input.max_voting_duration + } + ) + ); } if NoUpdateU32::should_update(@input.min_voting_duration) { _set_min_voting_duration(ref self, input.min_voting_duration); - MinVotingDurationUpdated(input.min_voting_duration); + + self + .emit( + Event::MinVotingDurationUpdated( + MinVotingDurationUpdated { + min_voting_duration: input.min_voting_duration + } + ) + ); } if NoUpdateU32::should_update(@input.voting_delay) { _set_voting_delay(ref self, input.voting_delay); - VotingDelayUpdated(input.voting_delay); + + self + .emit( + Event::VotingDelayUpdated( + VotingDelayUpdated { voting_delay: input.voting_delay } + ) + ); } if NoUpdateArray::should_update((@input).metadata_URI) { - MetadataURIUpdated(@input.metadata_URI); + self + .emit( + Event::MetadataUriUpdated( + MetadataUriUpdated { metadata_URI: input.metadata_URI.span() } + ) + ); } if NoUpdateArray::should_update((@input).dao_URI) { - DaoURIUpdated(@input.dao_URI); + self.emit(Event::DaoUriUpdated(DaoUriUpdated { dao_URI: input.dao_URI.span() })); } // if not NO_UPDATE if NoUpdateStrategy::should_update((@input).proposal_validation_strategy) { - // TODO: might be possible to remove need to clone by defining the event or setter on a snapshot. - // Similarly for all non value types. _set_proposal_validation_strategy( ref self, input.proposal_validation_strategy.clone() ); - ProposalValidationStrategyUpdated( - @input.proposal_validation_strategy, - @input.proposal_validation_strategy_metadata_URI - ); + self + .emit( + Event::ProposalValidationStrategyUpdated( + ProposalValidationStrategyUpdated { + proposal_validation_strategy: input + .proposal_validation_strategy + .clone(), + proposal_validation_strategy_metadata_URI: input + .proposal_validation_strategy_metadata_URI + .span() + } + ) + ); } if NoUpdateArray::should_update((@input).authenticators_to_add) { _add_authenticators(ref self, input.authenticators_to_add.clone()); - AuthenticatorsAdded(@input.authenticators_to_add); + self + .emit( + Event::AuthenticatorsAdded( + AuthenticatorsAdded { + authenticators: input.authenticators_to_add.span() + } + ) + ); } // if not NO_UPDATE if NoUpdateArray::should_update((@input).authenticators_to_remove) { _remove_authenticators(ref self, input.authenticators_to_remove.clone()); - AuthenticatorsRemoved(@input.authenticators_to_remove); + self + .emit( + Event::AuthenticatorsRemoved( + AuthenticatorsRemoved { + authenticators: input.authenticators_to_remove.span() + } + ) + ); } // if not NO_UPDATE if NoUpdateArray::should_update((@input).voting_strategies_to_add) { _add_voting_strategies(ref self, input.voting_strategies_to_add.clone()); - VotingStrategiesAdded( - @input.voting_strategies_to_add, @input.voting_strategies_metadata_URIs_to_add - ); + self + .emit( + Event::VotingStrategiesAdded( + VotingStrategiesAdded { + voting_strategies: input.voting_strategies_to_add.span(), + voting_strategy_metadata_URIs: input + .voting_strategies_metadata_URIs_to_add + .span() + } + ) + ); } // if not NO_UPDATE if NoUpdateArray::should_update((@input).voting_strategies_to_remove) { _remove_voting_strategies(ref self, input.voting_strategies_to_remove.clone()); - VotingStrategiesRemoved(@input.voting_strategies_to_remove); + self + .emit( + Event::VotingStrategiesRemoved( + VotingStrategiesRemoved { + voting_strategy_indices: input.voting_strategies_to_remove.span() + } + ) + ); } } From 36dda6719d1f9545a0cf718d699cda5543685f86 Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Fri, 25 Aug 2023 17:59:31 +0200 Subject: [PATCH 6/8] add vote tests (#515) Co-authored-by: Orland0x <37511817+Orland0x@users.noreply.github.com> --- starknet/src/space/space.cairo | 46 ++- starknet/src/tests.cairo | 2 + starknet/src/tests/mocks.cairo | 1 + starknet/src/tests/mocks/executor.cairo | 28 +- .../src/tests/mocks/no_voting_power.cairo | 22 + starknet/src/tests/vote.cairo | 378 ++++++++++++++++++ 6 files changed, 456 insertions(+), 21 deletions(-) create mode 100644 starknet/src/tests/mocks/no_voting_power.cairo create mode 100644 starknet/src/tests/vote.cairo diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index baaf6da6..26da1c84 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -17,7 +17,7 @@ trait ISpace { fn next_voting_strategy_index(self: @TContractState) -> u8; fn proposal_validation_strategy(self: @TContractState) -> Strategy; // #[view] - // fn vote_power(proposal_id: u256, choice: u8) -> u256; + fn vote_power(self: @TContractState, proposal_id: u256, choice: Choice) -> u256; // #[view] // fn vote_registry(proposal_id: u256, voter: ContractAddress) -> bool; fn proposals(self: @TContractState, proposal_id: u256) -> Proposal; @@ -75,32 +75,33 @@ trait ISpace { #[starknet::contract] mod Space { use super::ISpace; - use starknet::storage_access::{StorePacking, StoreUsingPacking}; - use starknet::{ClassHash, ContractAddress, info, Store, syscalls}; + use starknet::{ + storage_access::{StorePacking, StoreUsingPacking}, ClassHash, ContractAddress, info, Store, + syscalls + }; use zeroable::Zeroable; use array::{ArrayTrait, SpanTrait}; use clone::Clone; use option::OptionTrait; use hash::LegacyHash; use traits::{Into, TryInto}; - use serde::Serde; - - use sx::interfaces::{ - IProposalValidationStrategyDispatcher, IProposalValidationStrategyDispatcherTrait, - IVotingStrategyDispatcher, IVotingStrategyDispatcherTrait, IExecutionStrategyDispatcher, - IExecutionStrategyDispatcherTrait - }; - use sx::types::{ - UserAddress, Choice, FinalizationStatus, Strategy, IndexedStrategy, Proposal, - PackedProposal, IndexedStrategyTrait, IndexedStrategyImpl, UpdateSettingsCalldata, - NoUpdateU32, NoUpdateStrategy, NoUpdateArray + use sx::{ + interfaces::{ + IProposalValidationStrategyDispatcher, IProposalValidationStrategyDispatcherTrait, + IVotingStrategyDispatcher, IVotingStrategyDispatcherTrait, IExecutionStrategyDispatcher, + IExecutionStrategyDispatcherTrait + }, + types::{ + UserAddress, Choice, FinalizationStatus, Strategy, IndexedStrategy, Proposal, + PackedProposal, IndexedStrategyTrait, IndexedStrategyImpl, UpdateSettingsCalldata, + NoUpdateU32, NoUpdateStrategy, NoUpdateArray + }, + utils::{ + reinitializable::{Reinitializable}, ReinitializableImpl, bits::BitSetter, + legacy_hash::LegacyHashChoice, constants::INITIALIZE_SELECTOR + }, + external::ownable::Ownable }; - use sx::utils::reinitializable::Reinitializable; - use sx::utils::ReinitializableImpl; - use sx::utils::bits::BitSetter; - use sx::utils::legacy_hash::LegacyHashChoice; - use sx::external::ownable::Ownable; - use sx::utils::constants::INITIALIZE_SELECTOR; #[storage] struct Storage { @@ -304,6 +305,7 @@ mod Space { _add_authenticators(ref self, authenticators); self._next_proposal_id.write(1_u256); } + fn propose( ref self: ContractState, author: UserAddress, @@ -705,6 +707,10 @@ mod Space { } } + fn vote_power(self: @ContractState, proposal_id: u256, choice: Choice) -> u256 { + self._vote_power.read((proposal_id, choice)) + } + fn transfer_ownership(ref self: ContractState, new_owner: ContractAddress) { //TODO: temporary component syntax let mut state = Ownable::unsafe_new_contract_state(); diff --git a/starknet/src/tests.cairo b/starknet/src/tests.cairo index 96cd6099..ae1d679f 100644 --- a/starknet/src/tests.cairo +++ b/starknet/src/tests.cairo @@ -11,3 +11,5 @@ mod mocks; mod setup; mod utils; + +mod vote; diff --git a/starknet/src/tests/mocks.cairo b/starknet/src/tests/mocks.cairo index 70586b4d..95eba6d4 100644 --- a/starknet/src/tests/mocks.cairo +++ b/starknet/src/tests/mocks.cairo @@ -1,4 +1,5 @@ mod erc20_votes_preset; mod executor; +mod no_voting_power; mod proposal_validation_always_fail; mod space_v2; diff --git a/starknet/src/tests/mocks/executor.cairo b/starknet/src/tests/mocks/executor.cairo index b0b59ea2..ae7595f2 100644 --- a/starknet/src/tests/mocks/executor.cairo +++ b/starknet/src/tests/mocks/executor.cairo @@ -2,7 +2,6 @@ mod ExecutorExecutionStrategy { use sx::interfaces::IExecutionStrategy; use sx::types::{Proposal, ProposalStatus}; - use sx::execution_strategies::simple_quorum::SimpleQuorumExecutionStrategy; use starknet::ContractAddress; use core::serde::Serde; use core::array::ArrayTrait; @@ -39,3 +38,30 @@ mod ExecutorExecutionStrategy { #[constructor] fn constructor(ref self: ContractState) {} } + + +#[starknet::contract] +mod ExecutorWithoutTxExecutionStrategy { + use sx::interfaces::IExecutionStrategy; + use sx::types::{Proposal, ProposalStatus}; + use core::array::ArrayTrait; + + #[storage] + struct Storage {} + + #[external(v0)] + impl ExecutorWithoutTxExecutionStrategy of IExecutionStrategy { + // Dummy function that will do nothing + fn execute( + ref self: ContractState, + proposal: Proposal, + votes_for: u256, + votes_against: u256, + votes_abstain: u256, + payload: Array + ) {} + } + + #[constructor] + fn constructor(ref self: ContractState) {} +} diff --git a/starknet/src/tests/mocks/no_voting_power.cairo b/starknet/src/tests/mocks/no_voting_power.cairo new file mode 100644 index 00000000..fc4d60ca --- /dev/null +++ b/starknet/src/tests/mocks/no_voting_power.cairo @@ -0,0 +1,22 @@ +#[starknet::contract] +mod NoVotingPowerVotingStrategy { + use sx::interfaces::IVotingStrategy; + use sx::types::UserAddress; + use starknet::ContractAddress; + + #[storage] + struct Storage {} + + #[external(v0)] + impl NoVotingPowerVotingStrategy of IVotingStrategy { + fn get_voting_power( + self: @ContractState, + timestamp: u32, + voter: UserAddress, + params: Array, + user_params: Array, + ) -> u256 { + 0 + } + } +} diff --git a/starknet/src/tests/vote.cairo b/starknet/src/tests/vote.cairo new file mode 100644 index 00000000..c7c250f1 --- /dev/null +++ b/starknet/src/tests/vote.cairo @@ -0,0 +1,378 @@ +#[cfg(test)] +mod tests { + use array::ArrayTrait; + use starknet::{ + ContractAddress, syscalls::deploy_syscall, testing, contract_address_const, info + }; + use traits::{Into, TryInto}; + use result::ResultTrait; + use option::OptionTrait; + use integer::u256_from_felt252; + use clone::Clone; + use serde::{Serde}; + + use sx::space::space::{Space, ISpaceDispatcher, ISpaceDispatcherTrait}; + use sx::authenticators::vanilla::{ + VanillaAuthenticator, IVanillaAuthenticatorDispatcher, IVanillaAuthenticatorDispatcherTrait + }; + use sx::tests::mocks::executor::ExecutorWithoutTxExecutionStrategy; + use sx::voting_strategies::vanilla::VanillaVotingStrategy; + use sx::proposal_validation_strategies::vanilla::VanillaProposalValidationStrategy; + use sx::tests::mocks::proposal_validation_always_fail::AlwaysFailProposalValidationStrategy; + use sx::tests::mocks::no_voting_power::NoVotingPowerVotingStrategy; + use sx::tests::setup::setup::setup::{setup, deploy}; + use sx::types::{ + UserAddress, Strategy, IndexedStrategy, Choice, FinalizationStatus, Proposal, + UpdateSettingsCalldataImpl + }; + use sx::tests::utils::strategy_trait::{StrategyImpl}; + use sx::utils::constants::{PROPOSE_SELECTOR, VOTE_SELECTOR, UPDATE_PROPOSAL_SELECTOR}; + + use Space::Space as SpaceImpl; + + fn get_execution_strategy() -> Strategy { + let mut constructor_calldata = array![]; + + let (execution_strategy_address, _) = deploy_syscall( + ExecutorWithoutTxExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + constructor_calldata.span(), + false + ) + .unwrap(); + let strategy = StrategyImpl::from_address(execution_strategy_address); + strategy + } + + fn create_proposal( + authenticator: IVanillaAuthenticatorDispatcher, + space: ISpaceDispatcher, + execution_strategy: Strategy + ) { + let author = UserAddress::Starknet(contract_address_const::<0x5678>()); + let mut propose_calldata = array![]; + author.serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + } + + #[test] + #[available_gas(10000000000)] + fn vote_for() { + let config = setup(); + let (factory, space) = deploy(@config); + + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + // Increasing block timestamp pass voting delay + testing::set_block_timestamp(config.voting_delay); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + assert(space.vote_power(proposal_id, Choice::For(())) == 1, 'Vote power should be 1'); + assert(space.vote_power(proposal_id, Choice::Against(())) == 0, 'Vote power should be 0'); + assert(space.vote_power(proposal_id, Choice::Abstain(())) == 0, 'Vote power should be 0'); + // TODO : check event + } + + #[test] + #[available_gas(10000000000)] + fn vote_against() { + let config = setup(); + let (factory, space) = deploy(@config); + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + // Increasing block timestamp pass voting delay + testing::set_block_timestamp(config.voting_delay); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::Against(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + assert(space.vote_power(proposal_id, Choice::For(())) == 0, 'Vote power should be 0'); + assert(space.vote_power(proposal_id, Choice::Against(())) == 1, 'Vote power should be 1'); + assert(space.vote_power(proposal_id, Choice::Abstain(())) == 0, 'Vote power should be 0'); + // TODO : check event + } + + #[test] + #[available_gas(10000000000)] + fn vote_abstain() { + let config = setup(); + let (factory, space) = deploy(@config); + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + // Increasing block timestamp by voting delay + testing::set_block_timestamp(config.voting_delay); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::Abstain(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + assert(space.vote_power(proposal_id, Choice::For(())) == 0, 'Vote power should be 0'); + assert(space.vote_power(proposal_id, Choice::Against(())) == 0, 'Vote power should be 0'); + assert(space.vote_power(proposal_id, Choice::Abstain(())) == 1, 'Vote power should be 1'); + // TODO : check event + } + + #[test] + #[available_gas(10000000000)] + #[should_panic( + expected: ('Voting period has not started', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED') + )] + fn vote_too_early() { + let config = setup(); + let (factory, space) = deploy(@config); + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + // Do NOT increase block timestamp + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + // TODO : check event + } + + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Voting period has ended', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))] + fn vote_too_late() { + let config = setup(); + let (factory, space) = deploy(@config); + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + // Fast forward to end of voting period + testing::set_block_timestamp(config.voting_delay + config.max_voting_duration); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + // TODO : check event + } + + #[test] + #[available_gas(10000000000)] + #[should_panic( + expected: ('Proposal has been finalized', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED') + )] + fn vote_finalized_proposal() { + let config = setup(); + let (factory, space) = deploy(@config); + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + testing::set_block_timestamp(config.voting_delay); + + space + .execute( + 1, array![] + ); // Execute the proposal (will work because execution strategy doesn't check for finalization status or quorum) + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + } + + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Caller is not an authenticator', 'ENTRYPOINT_FAILED'))] + fn vote_without_authenticator() { + let config = setup(); + let (factory, space) = deploy(@config); + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + // Fast forward to end of voting period + testing::set_block_timestamp(config.voting_delay + config.max_voting_duration); + + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + let proposal_id = 1_u256; + let choice = Choice::For(()); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + let metadata_uri = array![]; + + space.vote(voter, proposal_id, choice, user_voting_strategies, metadata_uri); + // TODO : check event + } + + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Voter has already voted', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED'))] + fn vote_twice() { + let config = setup(); + let (factory, space) = deploy(@config); + + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + create_proposal(authenticator, space, execution_strategy); + + // Increasing block timestamp pass voting delay + testing::set_block_timestamp(config.voting_delay); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata.clone()); + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + } + + #[test] + #[available_gas(10000000000)] + #[should_panic( + expected: ('User has no voting power', 'ENTRYPOINT_FAILED', 'ENTRYPOINT_FAILED') + )] + fn vote_no_voting_power() { + let config = setup(); + let (factory, space) = deploy(@config); + + let execution_strategy = get_execution_strategy(); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let (no_voting_power_contract, _) = deploy_syscall( + NoVotingPowerVotingStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + array![].span(), + false + ) + .unwrap(); + let no_voting_power_strategy = StrategyImpl::from_address(no_voting_power_contract); + + let mut input = UpdateSettingsCalldataImpl::default(); + input.voting_strategies_to_add = array![no_voting_power_strategy]; + + testing::set_contract_address(config.owner); + space.update_settings(input); + + create_proposal(authenticator, space, execution_strategy); + + // Increasing block timestamp pass voting delay + testing::set_block_timestamp(config.voting_delay); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![ + IndexedStrategy { index: 1_u8, params: array![] } + ]; // index 1 + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + } +} From b64f7cb56c9e76135df76561581c476e5bb00302 Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Mon, 28 Aug 2023 11:38:58 +0200 Subject: [PATCH 7/8] make imports more compact (#509) --- starknet/src/authenticators/eth_sig.cairo | 11 +++++----- starknet/src/authenticators/eth_tx.cairo | 17 ++++++++------ starknet/src/authenticators/stark_sig.cairo | 7 +++--- starknet/src/authenticators/stark_tx.cairo | 6 +++-- starknet/src/authenticators/vanilla.cairo | 3 +-- .../execution_strategies/eth_relayer.cairo | 6 ++--- starknet/src/external/ownable.cairo | 3 +-- starknet/src/factory/factory.cairo | 11 +++++----- starknet/src/interfaces/i_account.cairo | 3 +-- .../proposing_power.cairo | 15 +++++++------ starknet/src/types/proposal.cairo | 10 ++++----- starknet/src/utils/merkle.cairo | 3 +-- starknet/src/utils/proposition_power.cairo | 13 ++++++----- starknet/src/utils/reinitializable.cairo | 3 +-- starknet/src/utils/signatures.cairo | 19 +++++++++------- starknet/src/utils/single_slot_proof.cairo | 3 --- starknet/src/utils/stark_eip712.cairo | 22 ++++++++++--------- starknet/src/utils/struct_hash.cairo | 14 +++++++----- .../voting_strategies/eth_balance_of.cairo | 7 +++--- .../voting_strategies/merkle_whitelist.cairo | 6 ++--- starknet/src/voting_strategies/vanilla.cairo | 3 +-- 21 files changed, 94 insertions(+), 91 deletions(-) diff --git a/starknet/src/authenticators/eth_sig.cairo b/starknet/src/authenticators/eth_sig.cairo index cb4af620..d1f4f164 100644 --- a/starknet/src/authenticators/eth_sig.cairo +++ b/starknet/src/authenticators/eth_sig.cairo @@ -1,5 +1,4 @@ -use starknet::{ContractAddress, EthAddress}; -use starknet::SyscallResult; +use starknet::{ContractAddress, EthAddress, SyscallResult}; use sx::types::{Strategy, IndexedStrategy, Choice}; #[starknet::interface] @@ -48,9 +47,11 @@ mod EthSigAuthenticator { use starknet::{ContractAddress, EthAddress, syscalls::call_contract_syscall}; use core::array::{ArrayTrait, SpanTrait}; use clone::Clone; - use sx::space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}; - use sx::types::{Strategy, IndexedStrategy, Choice, UserAddress}; - use sx::utils::{signatures, legacy_hash::LegacyHashEthAddress}; + use sx::{ + space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}, + types::{Strategy, IndexedStrategy, Choice, UserAddress}, + utils::{signatures, legacy_hash::LegacyHashEthAddress} + }; #[storage] struct Storage { diff --git a/starknet/src/authenticators/eth_tx.cairo b/starknet/src/authenticators/eth_tx.cairo index beb7c552..f1e482ee 100644 --- a/starknet/src/authenticators/eth_tx.cairo +++ b/starknet/src/authenticators/eth_tx.cairo @@ -34,16 +34,19 @@ trait IEthTxAuthenticator { #[starknet::contract] mod EthTxAuthenticator { use super::IEthTxAuthenticator; - use starknet::{ContractAddress, EthAddress, Felt252TryIntoEthAddress, EthAddressIntoFelt252}; - use starknet::syscalls::call_contract_syscall; - use core::serde::Serde; - use core::array::{ArrayTrait, SpanTrait}; + use starknet::{ + ContractAddress, EthAddress, Felt252TryIntoEthAddress, EthAddressIntoFelt252, + syscalls::call_contract_syscall + }; + use core::{serde::Serde, array::{ArrayTrait, SpanTrait}}; use traits::{PartialEq, TryInto, Into}; use option::OptionTrait; use zeroable::Zeroable; - use sx::space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}; - use sx::types::{UserAddress, Strategy, IndexedStrategy, Choice}; - use sx::utils::constants::{PROPOSE_SELECTOR, VOTE_SELECTOR, UPDATE_PROPOSAL_SELECTOR}; + use sx::{ + space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}, + types::{UserAddress, Strategy, IndexedStrategy, Choice}, + utils::constants::{PROPOSE_SELECTOR, VOTE_SELECTOR, UPDATE_PROPOSAL_SELECTOR} + }; #[storage] struct Storage { diff --git a/starknet/src/authenticators/stark_sig.cairo b/starknet/src/authenticators/stark_sig.cairo index b2a59e43..25edd142 100644 --- a/starknet/src/authenticators/stark_sig.cairo +++ b/starknet/src/authenticators/stark_sig.cairo @@ -44,9 +44,10 @@ mod StarkSigAuthenticator { use starknet::{ContractAddress, info}; use core::array::{ArrayTrait, SpanTrait}; use serde::Serde; - use sx::space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}; - use sx::types::{Strategy, IndexedStrategy, UserAddress, Choice}; - use sx::utils::stark_eip712; + use sx::{ + space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}, + types::{Strategy, IndexedStrategy, UserAddress, Choice}, utils::stark_eip712 + }; #[storage] struct Storage { diff --git a/starknet/src/authenticators/stark_tx.cairo b/starknet/src/authenticators/stark_tx.cairo index 3f8f88a8..920f2c00 100644 --- a/starknet/src/authenticators/stark_tx.cairo +++ b/starknet/src/authenticators/stark_tx.cairo @@ -35,8 +35,10 @@ mod StarkTxAuthenticator { use super::IStarkTxAuthenticator; use starknet::{ContractAddress, info}; use core::array::ArrayTrait; - use sx::space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}; - use sx::types::{UserAddress, Strategy, IndexedStrategy, Choice}; + use sx::{ + space::space::{ISpaceDispatcher, ISpaceDispatcherTrait}, + types::{UserAddress, Strategy, IndexedStrategy, Choice}, + }; #[storage] struct Storage {} diff --git a/starknet/src/authenticators/vanilla.cairo b/starknet/src/authenticators/vanilla.cairo index deb22ef0..c2140c40 100644 --- a/starknet/src/authenticators/vanilla.cairo +++ b/starknet/src/authenticators/vanilla.cairo @@ -1,5 +1,4 @@ -use starknet::ContractAddress; -use starknet::SyscallResult; +use starknet::{ContractAddress, SyscallResult}; #[starknet::interface] trait IVanillaAuthenticator { diff --git a/starknet/src/execution_strategies/eth_relayer.cairo b/starknet/src/execution_strategies/eth_relayer.cairo index ccf5344a..42d570ce 100644 --- a/starknet/src/execution_strategies/eth_relayer.cairo +++ b/starknet/src/execution_strategies/eth_relayer.cairo @@ -1,9 +1,7 @@ #[starknet::contract] mod EthRelayerExecutionStrategy { - use starknet::syscalls::send_message_to_l1_syscall; - use starknet::info::get_caller_address; - use sx::interfaces::IExecutionStrategy; - use sx::types::{Proposal}; + use starknet::{syscalls::send_message_to_l1_syscall, info::get_caller_address}; + use sx::{interfaces::IExecutionStrategy, types::Proposal}; #[external(v0)] impl EthRelayerExecutionStrategy of IExecutionStrategy { diff --git a/starknet/src/external/ownable.cairo b/starknet/src/external/ownable.cairo index 3de6e542..9256d651 100644 --- a/starknet/src/external/ownable.cairo +++ b/starknet/src/external/ownable.cairo @@ -2,8 +2,7 @@ // Migrated lib to v2 #[starknet::contract] mod Ownable { - use starknet::ContractAddress; - use starknet::get_caller_address; + use starknet::{ContractAddress, get_caller_address}; use zeroable::Zeroable; #[storage] diff --git a/starknet/src/factory/factory.cairo b/starknet/src/factory/factory.cairo index a3d9ceb0..b3f69e05 100644 --- a/starknet/src/factory/factory.cairo +++ b/starknet/src/factory/factory.cairo @@ -1,5 +1,4 @@ -use starknet::ContractAddress; -use starknet::ClassHash; +use starknet::{ContractAddress, ClassHash}; #[starknet::interface] trait IFactory { @@ -15,10 +14,10 @@ trait IFactory { #[starknet::contract] mod Factory { use super::IFactory; - use starknet::ContractAddress; - use starknet::contract_address_const; - use starknet::syscalls::{deploy_syscall, call_contract_syscall}; - use starknet::ClassHash; + use starknet::{ + ContractAddress, ClassHash, contract_address_const, + syscalls::{deploy_syscall, call_contract_syscall} + }; use result::ResultTrait; use array::{ArrayTrait, SpanTrait}; use sx::utils::constants::INITIALIZE_SELECTOR; diff --git a/starknet/src/interfaces/i_account.cairo b/starknet/src/interfaces/i_account.cairo index 21b94f82..672a2536 100644 --- a/starknet/src/interfaces/i_account.cairo +++ b/starknet/src/interfaces/i_account.cairo @@ -1,5 +1,4 @@ -use array::ArrayTrait; -use array::SpanTrait; +use array::{ArrayTrait, SpanTrait}; use starknet::account::Call; use starknet::ContractAddress; diff --git a/starknet/src/proposal_validation_strategies/proposing_power.cairo b/starknet/src/proposal_validation_strategies/proposing_power.cairo index cdbff952..fab93e41 100644 --- a/starknet/src/proposal_validation_strategies/proposing_power.cairo +++ b/starknet/src/proposal_validation_strategies/proposing_power.cairo @@ -1,19 +1,20 @@ #[starknet::contract] mod ProposingPowerProposalValidationStrategy { - use sx::interfaces::IProposalValidationStrategy; - use sx::types::{UserAddress, IndexedStrategy, IndexedStrategyTrait, Strategy}; - use sx::interfaces::{IVotingStrategyDispatcher, IVotingStrategyDispatcherTrait}; - use starknet::ContractAddress; - use starknet::info; + use sx::{ + interfaces::{ + IProposalValidationStrategy, IVotingStrategyDispatcher, IVotingStrategyDispatcherTrait + }, + types::{UserAddress, IndexedStrategy, IndexedStrategyTrait, Strategy}, + utils::{bits::BitSetter, proposition_power::_validate} + }; + use starknet::{ContractAddress, info}; use traits::{Into, TryInto}; use option::OptionTrait; use result::ResultTrait; use array::{ArrayTrait, SpanTrait}; use serde::Serde; - use sx::utils::bits::BitSetter; use box::BoxTrait; use clone::Clone; - use sx::utils::proposition_power::_validate; #[storage] struct Storage {} diff --git a/starknet/src/types/proposal.cairo b/starknet/src/types/proposal.cairo index 9747b451..bd34d64d 100644 --- a/starknet/src/types/proposal.cairo +++ b/starknet/src/types/proposal.cairo @@ -1,12 +1,10 @@ -use sx::types::user_address::UserAddressTrait; use clone::Clone; use serde::Serde; -use starknet::ContractAddress; -use starknet::storage_access::StorePacking; -use starknet::Store; -use sx::utils::math::pow; +use starknet::{ContractAddress, storage_access::StorePacking, Store}; +use sx::{ + utils::math::pow, types::{FinalizationStatus, UserAddress, user_address::UserAddressTrait} +}; use traits::{Into, TryInto}; -use sx::types::{FinalizationStatus, UserAddress}; use option::OptionTrait; const BITMASK_32: u128 = 0xffffffff; diff --git a/starknet/src/utils/merkle.cairo b/starknet/src/utils/merkle.cairo index 1f044389..a5d9cd26 100644 --- a/starknet/src/utils/merkle.cairo +++ b/starknet/src/utils/merkle.cairo @@ -2,10 +2,9 @@ use core::traits::Into; use array::{ArrayTrait, Span, SpanTrait}; use option::OptionTrait; use serde::Serde; -use sx::types::UserAddress; use clone::Clone; use hash::{LegacyHash}; -use sx::utils::legacy_hash::LegacyHashSpanFelt252; +use sx::{types::UserAddress, utils::legacy_hash::LegacyHashSpanFelt252}; /// Leaf struct for the merkle tree #[derive(Copy, Clone, Drop, Serde)] diff --git a/starknet/src/utils/proposition_power.cairo b/starknet/src/utils/proposition_power.cairo index 4da92af3..c4b3fb66 100644 --- a/starknet/src/utils/proposition_power.cairo +++ b/starknet/src/utils/proposition_power.cairo @@ -1,14 +1,15 @@ -use sx::interfaces::IProposalValidationStrategy; -use sx::types::{UserAddress, IndexedStrategy, IndexedStrategyTrait, Strategy}; -use sx::interfaces::{IVotingStrategyDispatcher, IVotingStrategyDispatcherTrait}; -use starknet::ContractAddress; -use starknet::info; +use sx::{ + interfaces::{ + IProposalValidationStrategy, IVotingStrategyDispatcher, IVotingStrategyDispatcherTrait + }, + types::{UserAddress, IndexedStrategy, IndexedStrategyTrait, Strategy}, utils::bits::BitSetter +}; +use starknet::{ContractAddress, info}; use traits::{Into, TryInto}; use option::OptionTrait; use result::ResultTrait; use array::{ArrayTrait, SpanTrait}; use serde::Serde; -use sx::utils::bits::BitSetter; use box::BoxTrait; use clone::Clone; diff --git a/starknet/src/utils/reinitializable.cairo b/starknet/src/utils/reinitializable.cairo index d044d172..2f137217 100644 --- a/starknet/src/utils/reinitializable.cairo +++ b/starknet/src/utils/reinitializable.cairo @@ -9,8 +9,7 @@ trait IReinitializable { #[starknet::contract] mod Reinitializable { use super::IReinitializable; - use starknet::ContractAddress; - use starknet::syscalls::call_contract_syscall; + use starknet::{ContractAddress, syscalls::call_contract_syscall}; use core::array::{ArrayTrait, SpanTrait}; #[storage] diff --git a/starknet/src/utils/signatures.cairo b/starknet/src/utils/signatures.cairo index 80d63db9..320ad323 100644 --- a/starknet/src/utils/signatures.cairo +++ b/starknet/src/utils/signatures.cairo @@ -4,14 +4,17 @@ use traits::Into; use clone::Clone; use core::keccak; use integer::u256_from_felt252; -use sx::types::{Strategy, IndexedStrategy, Choice}; -use sx::utils::Felt252ArrayIntoU256Array; -use sx::utils::math::pow; -use sx::utils::constants::{ - DOMAIN_TYPEHASH_LOW, DOMAIN_TYPEHASH_HIGH, ETHEREUM_PREFIX, STRATEGY_TYPEHASH_LOW, - STRATEGY_TYPEHASH_HIGH, INDEXED_STRATEGY_TYPEHASH_LOW, INDEXED_STRATEGY_TYPEHASH_HIGH, - PROPOSE_TYPEHASH_LOW, PROPOSE_TYPEHASH_HIGH, VOTE_TYPEHASH_LOW, VOTE_TYPEHASH_HIGH, - UPDATE_PROPOSAL_TYPEHASH_LOW, UPDATE_PROPOSAL_TYPEHASH_HIGH +use sx::{ + types::{Strategy, IndexedStrategy, Choice}, + utils::{ + Felt252ArrayIntoU256Array, math::pow, + constants::{ + DOMAIN_TYPEHASH_LOW, DOMAIN_TYPEHASH_HIGH, ETHEREUM_PREFIX, STRATEGY_TYPEHASH_LOW, + STRATEGY_TYPEHASH_HIGH, INDEXED_STRATEGY_TYPEHASH_LOW, INDEXED_STRATEGY_TYPEHASH_HIGH, + PROPOSE_TYPEHASH_LOW, PROPOSE_TYPEHASH_HIGH, VOTE_TYPEHASH_LOW, VOTE_TYPEHASH_HIGH, + UPDATE_PROPOSAL_TYPEHASH_LOW, UPDATE_PROPOSAL_TYPEHASH_HIGH + } + } }; impl ContractAddressIntoU256 of Into { diff --git a/starknet/src/utils/single_slot_proof.cairo b/starknet/src/utils/single_slot_proof.cairo index e152e64e..1499d30f 100644 --- a/starknet/src/utils/single_slot_proof.cairo +++ b/starknet/src/utils/single_slot_proof.cairo @@ -1,6 +1,3 @@ -use core::zeroable::Zeroable; -use array::ArrayTrait; - // Each word is 64 bits #[derive(Serde, Option, Drop)] struct StorageSlot { diff --git a/starknet/src/utils/stark_eip712.cairo b/starknet/src/utils/stark_eip712.cairo index 97a8446c..00d77e0a 100644 --- a/starknet/src/utils/stark_eip712.cairo +++ b/starknet/src/utils/stark_eip712.cairo @@ -4,18 +4,20 @@ use array::{ArrayTrait, SpanTrait}; use traits::Into; use box::BoxTrait; use serde::Serde; -use sx::types::{Strategy, IndexedStrategy, Choice}; -use sx::utils::{ - struct_hash::StructHash, - constants::{ - STARKNET_MESSAGE, DOMAIN_TYPEHASH, PROPOSE_TYPEHASH, VOTE_TYPEHASH, - UPDATE_PROPOSAL_TYPEHASH, ERC165_ACCOUNT_INTERFACE_ID, ERC165_OLD_ACCOUNT_INTERFACE_ID +use sx::{ + types::{Strategy, IndexedStrategy, Choice}, + utils::{ + struct_hash::StructHash, + constants::{ + STARKNET_MESSAGE, DOMAIN_TYPEHASH, PROPOSE_TYPEHASH, VOTE_TYPEHASH, + UPDATE_PROPOSAL_TYPEHASH, ERC165_ACCOUNT_INTERFACE_ID, ERC165_OLD_ACCOUNT_INTERFACE_ID + } + }, + interfaces::{ + AccountABIDispatcher, AccountABIDispatcherTrait, AccountCamelABIDispatcher, + AccountCamelABIDispatcherTrait } }; -use sx::interfaces::{ - AccountABIDispatcher, AccountABIDispatcherTrait, AccountCamelABIDispatcher, - AccountCamelABIDispatcherTrait -}; fn verify_propose_sig( domain_hash: felt252, diff --git a/starknet/src/utils/struct_hash.cairo b/starknet/src/utils/struct_hash.cairo index bcbabbeb..adb7e9d9 100644 --- a/starknet/src/utils/struct_hash.cairo +++ b/starknet/src/utils/struct_hash.cairo @@ -1,14 +1,16 @@ use array::{ArrayTrait, SpanTrait}; use hash::LegacyHash; use serde::Serde; -use sx::types::{Strategy, IndexedStrategy}; -use sx::utils::{ - constants::{ - STARKNET_MESSAGE, DOMAIN_TYPEHASH, STRATEGY_TYPEHASH, INDEXED_STRATEGY_TYPEHASH, - U256_TYPEHASH, PROPOSE_TYPEHASH, VOTE_TYPEHASH, UPDATE_PROPOSAL_TYPEHASH +use sx::{ + types::{Strategy, IndexedStrategy}, + utils::{ + constants::{ + STARKNET_MESSAGE, DOMAIN_TYPEHASH, STRATEGY_TYPEHASH, INDEXED_STRATEGY_TYPEHASH, + U256_TYPEHASH, PROPOSE_TYPEHASH, VOTE_TYPEHASH, UPDATE_PROPOSAL_TYPEHASH + }, + legacy_hash::LegacyHashSpanFelt252, } }; -use sx::utils::legacy_hash::LegacyHashSpanFelt252; trait StructHash { fn struct_hash(self: @T) -> felt252; diff --git a/starknet/src/voting_strategies/eth_balance_of.cairo b/starknet/src/voting_strategies/eth_balance_of.cairo index e060e0c1..a4fca095 100644 --- a/starknet/src/voting_strategies/eth_balance_of.cairo +++ b/starknet/src/voting_strategies/eth_balance_of.cairo @@ -2,9 +2,10 @@ mod EthBalanceOfVotingStrategy { use traits::Into; use starknet::{EthAddress, ContractAddress}; - use sx::interfaces::IVotingStrategy; - use sx::types::{UserAddress, UserAddressTrait}; - use sx::utils::single_slot_proof::SingleSlotProof; + use sx::{ + interfaces::IVotingStrategy, types::{UserAddress, UserAddressTrait}, + utils::single_slot_proof::SingleSlotProof + }; #[storage] struct Storage {} diff --git a/starknet/src/voting_strategies/merkle_whitelist.cairo b/starknet/src/voting_strategies/merkle_whitelist.cairo index 182608e7..1cef108a 100644 --- a/starknet/src/voting_strategies/merkle_whitelist.cairo +++ b/starknet/src/voting_strategies/merkle_whitelist.cairo @@ -1,11 +1,11 @@ #[starknet::contract] mod MerkleWhitelistVotingStrategy { - use sx::interfaces::IVotingStrategy; + use sx::{ + interfaces::IVotingStrategy, types::UserAddress, utils::merkle::{assert_valid_proof, Leaf} + }; use serde::Serde; - use sx::types::UserAddress; use array::{ArrayTrait, Span, SpanTrait}; use option::OptionTrait; - use sx::utils::merkle::{assert_valid_proof, Leaf}; const LEAF_SIZE: usize = 4; // Serde::::serialize().len() diff --git a/starknet/src/voting_strategies/vanilla.cairo b/starknet/src/voting_strategies/vanilla.cairo index 0e880acf..038912c6 100644 --- a/starknet/src/voting_strategies/vanilla.cairo +++ b/starknet/src/voting_strategies/vanilla.cairo @@ -1,7 +1,6 @@ #[starknet::contract] mod VanillaVotingStrategy { - use sx::interfaces::IVotingStrategy; - use sx::types::UserAddress; + use sx::{interfaces::IVotingStrategy, types::UserAddress}; use starknet::ContractAddress; #[storage] From a01414546d2f0290e831ba2b3447300af29667fe Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Mon, 28 Aug 2023 13:28:57 +0200 Subject: [PATCH 8/8] Fix missing checks in execute and use `match` in loops (#505) * fix missing checks in execute and add regression tests * fix: add checks when updating min/max voting durations * refactor: use match in add_voting_strategies * refactor: use match in remove_voting_strategies * refactor: use match in add_authenticators * refactor: use match in remove_authenticators * refactor: use match in assert_no_duplicate_indices * refactor: use match in felt252array_into_u256array * optimize get_voting_power; use Span in get_voting_power signature * fix: uncomment test --- .../i_proposal_validation_strategy.cairo | 4 +- .../src/interfaces/i_voting_strategy.cairo | 4 +- .../proposing_power.cairo | 4 +- .../vanilla.cairo | 4 +- starknet/src/space/space.cairo | 195 +++++++++++------- .../src/tests/mocks/no_voting_power.cairo | 4 +- .../proposal_validation_always_fail.cairo | 4 +- .../proposing_power.cairo | 26 +-- .../src/tests/test_merkle_whitelist.cairo | 7 +- starknet/src/tests/test_space.cairo | 76 ++++++- starknet/src/types/indexed_strategy.cairo | 28 +-- starknet/src/utils/felt_arr_to_uint_arr.cairo | 18 +- starknet/src/utils/proposition_power.cairo | 24 +-- starknet/src/utils/single_slot_proof.cairo | 5 +- .../src/voting_strategies/erc20_votes.cairo | 5 +- .../voting_strategies/eth_balance_of.cairo | 4 +- .../voting_strategies/merkle_whitelist.cairo | 6 +- starknet/src/voting_strategies/vanilla.cairo | 4 +- 18 files changed, 263 insertions(+), 159 deletions(-) diff --git a/starknet/src/interfaces/i_proposal_validation_strategy.cairo b/starknet/src/interfaces/i_proposal_validation_strategy.cairo index d22eb037..527afa40 100644 --- a/starknet/src/interfaces/i_proposal_validation_strategy.cairo +++ b/starknet/src/interfaces/i_proposal_validation_strategy.cairo @@ -6,7 +6,7 @@ trait IProposalValidationStrategy { fn validate( self: @TContractState, author: UserAddress, - params: Array, - user_params: Array + params: Span, + user_params: Span ) -> bool; } diff --git a/starknet/src/interfaces/i_voting_strategy.cairo b/starknet/src/interfaces/i_voting_strategy.cairo index fbdc3477..563bf29f 100644 --- a/starknet/src/interfaces/i_voting_strategy.cairo +++ b/starknet/src/interfaces/i_voting_strategy.cairo @@ -7,7 +7,7 @@ trait IVotingStrategy { self: @TContractState, timestamp: u32, voter: UserAddress, - params: Array, - user_params: Array, + params: Span, + user_params: Span, ) -> u256; } diff --git a/starknet/src/proposal_validation_strategies/proposing_power.cairo b/starknet/src/proposal_validation_strategies/proposing_power.cairo index fab93e41..1ab4c599 100644 --- a/starknet/src/proposal_validation_strategies/proposing_power.cairo +++ b/starknet/src/proposal_validation_strategies/proposing_power.cairo @@ -24,8 +24,8 @@ mod ProposingPowerProposalValidationStrategy { fn validate( self: @ContractState, author: UserAddress, - params: Array, // [proposal_threshold: u256, allowed_strategies: Array] - user_params: Array // [user_strategies: Array] + params: Span, // [proposal_threshold: u256, allowed_strategies: Array] + user_params: Span // [user_strategies: Array] ) -> bool { _validate(author, params, user_params) } diff --git a/starknet/src/proposal_validation_strategies/vanilla.cairo b/starknet/src/proposal_validation_strategies/vanilla.cairo index 54a2fa17..6786687d 100644 --- a/starknet/src/proposal_validation_strategies/vanilla.cairo +++ b/starknet/src/proposal_validation_strategies/vanilla.cairo @@ -12,8 +12,8 @@ mod VanillaProposalValidationStrategy { fn validate( self: @ContractState, author: UserAddress, - params: Array, - user_params: Array + params: Span, + user_params: Span ) -> bool { true } diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 26da1c84..17760b50 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -297,12 +297,14 @@ mod Space { let mut state = Ownable::unsafe_new_contract_state(); Ownable::initializer(ref state); Ownable::transfer_ownership(ref state, owner); + _set_max_voting_duration( + ref self, max_voting_duration + ); // Need to set max before min, or else `max == 0` and set_min will revert _set_min_voting_duration(ref self, min_voting_duration); - _set_max_voting_duration(ref self, max_voting_duration); _set_voting_delay(ref self, voting_delay); _set_proposal_validation_strategy(ref self, proposal_validation_strategy); - _add_voting_strategies(ref self, voting_strategies); - _add_authenticators(ref self, authenticators); + _add_voting_strategies(ref self, voting_strategies.span()); + _add_authenticators(ref self, authenticators.span()); self._next_proposal_id.write(1_u256); } @@ -323,7 +325,9 @@ mod Space { contract_address: proposal_validation_strategy.address } .validate( - author, proposal_validation_strategy.params, user_proposal_validation_params + author, + proposal_validation_strategy.params.span(), + user_proposal_validation_params.span() ); assert(is_valid, 'Proposal is not valid'); @@ -399,7 +403,7 @@ mod Space { @self, voter, proposal.start_timestamp, - user_voting_strategies, + user_voting_strategies.span(), proposal.active_voting_strategies ); @@ -430,6 +434,15 @@ mod Space { let mut proposal = self._proposals.read(proposal_id); assert_proposal_exists(@proposal); + let recovered_hash = poseidon::poseidon_hash_span(execution_payload.span()); + // Check that payload matches + assert(recovered_hash == proposal.execution_payload_hash, 'Invalid payload hash'); + + // Check that finalization status is not pending + assert( + proposal.finalization_status == FinalizationStatus::Pending(()), 'Already finalized' + ); + IExecutionStrategyDispatcher { contract_address: proposal.execution_strategy } @@ -469,7 +482,7 @@ mod Space { proposal .execution_payload_hash = - poseidon::poseidon_hash_span(execution_strategy.clone().params.span()); + poseidon::poseidon_hash_span(execution_strategy.params.span()); self._proposals.write(proposal_id, proposal); @@ -581,10 +594,29 @@ mod Space { let state = Ownable::unsafe_new_contract_state(); Ownable::assert_only_owner(@state); - // if not NO_UPDATE - if NoUpdateU32::should_update(@input.max_voting_duration) { - _set_max_voting_duration(ref self, input.max_voting_duration); + // Needed because the compiler will go crazy if we try to use `input` directly + let _min_voting_duration = input.min_voting_duration; + let _max_voting_duration = input.max_voting_duration; + + if NoUpdateU32::should_update(@_max_voting_duration) + && NoUpdateU32::should_update(@_min_voting_duration) { + // Check that min and max voting durations are valid + // We don't use the internal `_set_min_voting_duration` and `_set_max_voting_duration` functions because + // it would revert when `_min_voting_duration > max_voting_duration` (when the new `_min` is + // bigger than the current `max`). + assert(_min_voting_duration <= _max_voting_duration, 'Invalid duration'); + + self._min_voting_duration.write(input.min_voting_duration); + self + .emit( + Event::MinVotingDurationUpdated( + MinVotingDurationUpdated { + min_voting_duration: input.min_voting_duration + } + ) + ); + self._max_voting_duration.write(input.max_voting_duration); self .emit( Event::MaxVotingDurationUpdated( @@ -593,11 +625,8 @@ mod Space { } ) ); - } - - if NoUpdateU32::should_update(@input.min_voting_duration) { + } else if NoUpdateU32::should_update(@_min_voting_duration) { _set_min_voting_duration(ref self, input.min_voting_duration); - self .emit( Event::MinVotingDurationUpdated( @@ -606,6 +635,16 @@ mod Space { } ) ); + } else if NoUpdateU32::should_update(@_max_voting_duration) { + _set_max_voting_duration(ref self, input.max_voting_duration); + self + .emit( + Event::MaxVotingDurationUpdated( + MaxVotingDurationUpdated { + max_voting_duration: input.max_voting_duration + } + ) + ); } if NoUpdateU32::should_update(@input.voting_delay) { @@ -632,7 +671,6 @@ mod Space { self.emit(Event::DaoUriUpdated(DaoUriUpdated { dao_URI: input.dao_URI.span() })); } - // if not NO_UPDATE if NoUpdateStrategy::should_update((@input).proposal_validation_strategy) { _set_proposal_validation_strategy( ref self, input.proposal_validation_strategy.clone() @@ -653,7 +691,7 @@ mod Space { } if NoUpdateArray::should_update((@input).authenticators_to_add) { - _add_authenticators(ref self, input.authenticators_to_add.clone()); + _add_authenticators(ref self, input.authenticators_to_add.span()); self .emit( Event::AuthenticatorsAdded( @@ -664,9 +702,8 @@ mod Space { ); } - // if not NO_UPDATE if NoUpdateArray::should_update((@input).authenticators_to_remove) { - _remove_authenticators(ref self, input.authenticators_to_remove.clone()); + _remove_authenticators(ref self, input.authenticators_to_remove.span()); self .emit( Event::AuthenticatorsRemoved( @@ -677,9 +714,8 @@ mod Space { ); } - // if not NO_UPDATE if NoUpdateArray::should_update((@input).voting_strategies_to_add) { - _add_voting_strategies(ref self, input.voting_strategies_to_add.clone()); + _add_voting_strategies(ref self, input.voting_strategies_to_add.span()); self .emit( Event::VotingStrategiesAdded( @@ -693,9 +729,8 @@ mod Space { ); } - // if not NO_UPDATE if NoUpdateArray::should_update((@input).voting_strategies_to_remove) { - _remove_voting_strategies(ref self, input.voting_strategies_to_remove.clone()); + _remove_voting_strategies(ref self, input.voting_strategies_to_remove.span()); self .emit( Event::VotingStrategiesRemoved( @@ -743,35 +778,41 @@ mod Space { self: @ContractState, voter: UserAddress, timestamp: u32, - user_strategies: Array, + mut user_strategies: Span, allowed_strategies: u256 ) -> u256 { user_strategies.assert_no_duplicate_indices(); let mut total_voting_power = 0_u256; - let mut i = 0_usize; loop { - if i >= user_strategies.len() { - break (); - } - let strategy_index = user_strategies.at(i).index; - assert(allowed_strategies.is_bit_set(*strategy_index), 'Invalid strategy index'); - let strategy = self._voting_strategies.read(*strategy_index); - total_voting_power += IVotingStrategyDispatcher { - contract_address: strategy.address - } - .get_voting_power( - timestamp, voter, strategy.params, user_strategies.at(i).params.clone() - ); - i += 1; + match user_strategies.pop_front() { + Option::Some(strategy_index) => { + assert( + allowed_strategies.is_bit_set(*strategy_index.index), + 'Invalid strategy index' + ); + let strategy = self._voting_strategies.read(*strategy_index.index); + total_voting_power += IVotingStrategyDispatcher { + contract_address: strategy.address + } + .get_voting_power( + timestamp, voter, strategy.params.span(), strategy_index.params.span() + ); + }, + Option::None => { + break; + }, + }; }; total_voting_power } fn _set_max_voting_duration(ref self: ContractState, _max_voting_duration: u32) { + assert(_max_voting_duration >= self._min_voting_duration.read(), 'Invalid duration'); self._max_voting_duration.write(_max_voting_duration); } fn _set_min_voting_duration(ref self: ContractState, _min_voting_duration: u32) { + assert(_min_voting_duration <= self._max_voting_duration.read(), 'Invalid duration'); self._min_voting_duration.write(_min_voting_duration); } @@ -785,73 +826,71 @@ mod Space { self._proposal_validation_strategy.write(_proposal_validation_strategy); } - fn _add_voting_strategies(ref self: ContractState, _voting_strategies: Array) { + fn _add_voting_strategies(ref self: ContractState, mut _voting_strategies: Span) { let mut cachedActiveVotingStrategies = self._active_voting_strategies.read(); let mut cachedNextVotingStrategyIndex = self._next_voting_strategy_index.read(); assert( cachedNextVotingStrategyIndex.into() < 256_u32 - _voting_strategies.len(), 'Exceeds Voting Strategy Limit' ); - let mut _voting_strategies_span = _voting_strategies.span(); - let mut i = 0_usize; loop { - if i >= _voting_strategies.len() { - break (); - } - - let strategy = _voting_strategies_span.pop_front().unwrap().clone(); - assert(!strategy.address.is_zero(), 'Invalid voting strategy'); - cachedActiveVotingStrategies.set_bit(cachedNextVotingStrategyIndex, true); - self._voting_strategies.write(cachedNextVotingStrategyIndex, strategy); - cachedNextVotingStrategyIndex += 1_u8; - i += 1; + match _voting_strategies.pop_front() { + Option::Some(strategy) => { + assert(!(*strategy.address).is_zero(), 'Invalid voting strategy'); + cachedActiveVotingStrategies.set_bit(cachedNextVotingStrategyIndex, true); + self._voting_strategies.write(cachedNextVotingStrategyIndex, strategy.clone()); + cachedNextVotingStrategyIndex += 1_u8; + }, + Option::None => { + break; + }, + }; }; self._active_voting_strategies.write(cachedActiveVotingStrategies); self._next_voting_strategy_index.write(cachedNextVotingStrategyIndex); } - fn _remove_voting_strategies(ref self: ContractState, _voting_strategies: Array) { + fn _remove_voting_strategies(ref self: ContractState, mut _voting_strategies: Span) { let mut cachedActiveVotingStrategies = self._active_voting_strategies.read(); - let mut _voting_strategies_span = _voting_strategies.span(); - let mut i = 0_usize; loop { - if i >= _voting_strategies.len() { - break (); - } - - let index = _voting_strategies_span.pop_front().unwrap(); - cachedActiveVotingStrategies.set_bit(*index, false); - i += 1; + match _voting_strategies.pop_front() { + Option::Some(index) => { + cachedActiveVotingStrategies.set_bit(*index, false); + }, + Option::None => { + break; + }, + }; }; - if cachedActiveVotingStrategies == 0 { - panic_with_felt252('No active voting strategy left'); - } + assert(cachedActiveVotingStrategies != 0, 'No active voting strategy left'); self._active_voting_strategies.write(cachedActiveVotingStrategies); } - fn _add_authenticators(ref self: ContractState, _authenticators: Array) { - let mut _authenticators_span = _authenticators.span(); - let mut i = 0_usize; + fn _add_authenticators(ref self: ContractState, mut _authenticators: Span) { loop { - if i >= _authenticators.len() { - break (); - } - self._authenticators.write(*_authenticators_span.pop_front().unwrap(), true); - i += 1; + match _authenticators.pop_front() { + Option::Some(authenticator) => { + self._authenticators.write(*authenticator, true); + }, + Option::None => { + break; + }, + }; } } - fn _remove_authenticators(ref self: ContractState, _authenticators: Array) { - let mut _authenticators_span = _authenticators.span(); - let mut i = 0_usize; + fn _remove_authenticators(ref self: ContractState, mut _authenticators: Span) { loop { - if i >= _authenticators.len() { - break (); - } - self._authenticators.write(*_authenticators_span.pop_front().unwrap(), false); - i += 1; + match _authenticators.pop_front() { + Option::Some(authenticator) => { + self._authenticators.write(*authenticator, false); + }, + Option::None => { + break; + }, + }; } } } diff --git a/starknet/src/tests/mocks/no_voting_power.cairo b/starknet/src/tests/mocks/no_voting_power.cairo index fc4d60ca..b4cd799f 100644 --- a/starknet/src/tests/mocks/no_voting_power.cairo +++ b/starknet/src/tests/mocks/no_voting_power.cairo @@ -13,8 +13,8 @@ mod NoVotingPowerVotingStrategy { self: @ContractState, timestamp: u32, voter: UserAddress, - params: Array, - user_params: Array, + params: Span, + user_params: Span, ) -> u256 { 0 } diff --git a/starknet/src/tests/mocks/proposal_validation_always_fail.cairo b/starknet/src/tests/mocks/proposal_validation_always_fail.cairo index 2d418b35..d0ba185a 100644 --- a/starknet/src/tests/mocks/proposal_validation_always_fail.cairo +++ b/starknet/src/tests/mocks/proposal_validation_always_fail.cairo @@ -12,8 +12,8 @@ mod AlwaysFailProposalValidationStrategy { fn validate( self: @ContractState, author: UserAddress, - params: Array, - user_params: Array + params: Span, + user_params: Span ) -> bool { false } diff --git a/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo b/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo index b587e4a6..007059c8 100644 --- a/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo +++ b/starknet/src/tests/proposal_validation_strategies/proposing_power.cairo @@ -73,7 +73,7 @@ mod tests { let author = UserAddress::Starknet(contract_address_const::<0x123456789>()); // Vanilla should return 1 so it should be fine - let is_validated = contract.validate(author, params, user_params.clone()); + let is_validated = contract.validate(author, params.span(), user_params.span()); assert(is_validated, 'not enough VP'); // Now increase threshold @@ -83,7 +83,7 @@ mod tests { allowed_strategies.serialize(ref params); // Threshold is 2 but VP should be 1 - let is_validated = contract.validate(author, params.clone(), user_params); + let is_validated = contract.validate(author, params.span(), user_params.span()); assert(!is_validated, 'Threshold should not be reached'); // But now if we add the vanilla voting strategy twice then it should be fine @@ -101,7 +101,7 @@ mod tests { let mut user_params = array![]; used_strategies.serialize(ref user_params); - let is_validated = contract.validate(author, params, user_params); + let is_validated = contract.validate(author, params.span(), user_params.span()); assert(is_validated, 'should have 2 VP'); } @@ -174,7 +174,7 @@ mod tests { let mut user_params = array![]; used_strategies.serialize(ref user_params); - let is_validated = contract.validate(author, params.clone(), user_params.clone()); + let is_validated = contract.validate(author, params.span(), user_params.span()); assert(!is_validated, 'should not have enough VP'); // setup for voter2 @@ -187,8 +187,8 @@ mod tests { let mut user_params = array![]; used_strategies.serialize(ref user_params); - let is_validated = contract.validate(author, params.clone(), user_params.clone()); - assert(is_validated, 'should have enough VP'); + let is_validated = contract.validate(author, params.span(), user_params.span()); + assert(is_validated, 'should have enough VP1'); // setup for voter3 let author = leaf3.address; @@ -200,8 +200,8 @@ mod tests { let mut user_params = array![]; used_strategies.serialize(ref user_params); - let is_validated = contract.validate(author, params.clone(), user_params.clone()); - assert(is_validated, 'should have enough VP'); + let is_validated = contract.validate(author, params.span(), user_params.span()); + assert(is_validated, 'should have enough VP2'); // -- Now let's mix merkle and vanilla voting strategies -- @@ -233,8 +233,8 @@ mod tests { let mut user_params = array![]; used_strategies.serialize(ref user_params); - let is_validated = contract.validate(author, params.clone(), user_params.clone()); - assert(is_validated, 'should have enough VP'); + let is_validated = contract.validate(author, params.span(), user_params.span()); + assert(is_validated, 'should have enough VP2'); // and a random voter that doesn't use the whitelist should not have enough VP let author = UserAddress::Starknet(contract_address_const::<0x123456789>()); @@ -242,7 +242,7 @@ mod tests { let mut user_params = array![]; used_strategies.serialize(ref user_params); - let is_validated = contract.validate(author, params.clone(), user_params.clone()); + let is_validated = contract.validate(author, params.span(), user_params.span()); assert(!is_validated, 'should not have enough VP'); } @@ -313,7 +313,7 @@ mod tests { let author = UserAddress::Starknet(owner); - let is_validated = contract.validate(author, params, user_params.clone()); + let is_validated = contract.validate(author, params.span(), user_params.span()); assert(is_validated, 'not enough VP'); // Now increase threshold @@ -322,7 +322,7 @@ mod tests { proposal_threshold.serialize(ref params); allowed_strategies.serialize(ref params); - let is_validated = contract.validate(author, params.clone(), user_params); + let is_validated = contract.validate(author, params.span(), user_params.span()); assert(!is_validated, 'Threshold should not be reached'); } } diff --git a/starknet/src/tests/test_merkle_whitelist.cairo b/starknet/src/tests/test_merkle_whitelist.cairo index 559cd639..38e774fe 100644 --- a/starknet/src/tests/test_merkle_whitelist.cairo +++ b/starknet/src/tests/test_merkle_whitelist.cairo @@ -151,6 +151,7 @@ mod merkle_utils { members } } + #[cfg(test)] mod assert_valid_proof { use sx::tests::setup::setup::setup::{setup, deploy}; @@ -358,7 +359,7 @@ mod merkle_whitelist_voting_power { leaf.serialize(ref user_params); proof.serialize(ref user_params); - voting_strategy.get_voting_power(timestamp, voter, params, user_params); + voting_strategy.get_voting_power(timestamp, voter, params.span(), user_params.span()); } #[test] @@ -394,7 +395,7 @@ mod merkle_whitelist_voting_power { fake_leaf.serialize(ref user_params); proof.serialize(ref user_params); - voting_strategy.get_voting_power(timestamp, voter, params, user_params); + voting_strategy.get_voting_power(timestamp, voter, params.span(), user_params.span()); } #[test] @@ -431,6 +432,6 @@ mod merkle_whitelist_voting_power { fake_leaf.serialize(ref user_params); proof.serialize(ref user_params); - voting_strategy.get_voting_power(timestamp, voter, params, user_params); + voting_strategy.get_voting_power(timestamp, voter, params.span(), user_params.span()); } } diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 7e66afc0..6bae94af 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -239,7 +239,7 @@ mod tests { min_end_timestamp: timestamp + 2_u32, max_end_timestamp: timestamp + 3_u32, execution_payload_hash: poseidon::poseidon_hash_span( - vanilla_execution_strategy.clone().params.span() + vanilla_execution_strategy.params.span() ), execution_strategy: vanilla_execution_strategy.address, author: author, @@ -256,7 +256,7 @@ mod tests { // Keeping the same execution strategy contract but changing the payload let mut new_payload = array![1]; let execution_strategy = Strategy { - address: vanilla_execution_strategy.address, params: new_payload + address: vanilla_execution_strategy.address, params: new_payload.clone() }; execution_strategy.serialize(ref update_calldata); ArrayTrait::::new().serialize(ref update_calldata); @@ -264,8 +264,7 @@ mod tests { authenticator .authenticate(space.contract_address, UPDATE_PROPOSAL_SELECTOR, update_calldata); - // Increasing block timestamp by 1 to pass voting delay - testing::set_block_timestamp(1_u64); + testing::set_block_timestamp(config.voting_delay); let mut vote_calldata = array![]; let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); @@ -281,10 +280,10 @@ mod tests { // Vote on Proposal authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); - testing::set_block_timestamp(2_u64); + testing::set_block_timestamp(config.voting_delay + config.max_voting_duration); // Execute Proposal - space.execute(u256_from_felt252(1), vanilla_execution_strategy.params); + space.execute(u256_from_felt252(1), new_payload); } #[test] @@ -341,6 +340,68 @@ mod tests { authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); } + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Already finalized', 'ENTRYPOINT_FAILED'))] + fn execute_already_finalized() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let quorum = u256_from_felt252(1); + let mut constructor_calldata = array![]; + quorum.serialize(ref constructor_calldata); + + let (vanilla_execution_strategy_address, _) = deploy_syscall( + VanillaExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + constructor_calldata.span(), + false + ) + .unwrap(); + let vanilla_execution_strategy = StrategyImpl::from_address( + vanilla_execution_strategy_address + ); + let author = UserAddress::Starknet(contract_address_const::<0x5678>()); + let mut propose_calldata = array![]; + author.serialize(ref propose_calldata); + vanilla_execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + + assert(space.next_proposal_id() == 2_u256, 'next_proposal_id should be 2'); + + testing::set_block_timestamp(config.voting_delay); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = u256_from_felt252(1); + proposal_id.serialize(ref vote_calldata); + let choice = Choice::For(()); + choice.serialize(ref vote_calldata); + let mut user_voting_strategies = array![IndexedStrategy { index: 0_u8, params: array![] }]; + user_voting_strategies.serialize(ref vote_calldata); + ArrayTrait::::new().serialize(ref vote_calldata); + + // Vote on Proposal + authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + + testing::set_block_timestamp(config.voting_delay + config.max_voting_duration); + + // Execute Proposal + space.execute(u256_from_felt252(1), vanilla_execution_strategy.params.clone()); + + // Execute a second time + space.execute(u256_from_felt252(1), vanilla_execution_strategy.params.clone()); + } + #[test] #[available_gas(10000000000)] #[should_panic( @@ -380,8 +441,7 @@ mod tests { authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); let proposal_id = u256_from_felt252(1); - // Increasing block timestamp by 1 to pass voting delay - testing::set_block_timestamp(1_u64); + testing::set_block_timestamp(config.voting_delay); let proposal = space.proposals(proposal_id); assert(proposal.finalization_status == FinalizationStatus::Pending(()), 'pending'); diff --git a/starknet/src/types/indexed_strategy.cairo b/starknet/src/types/indexed_strategy.cairo index e7dbfb59..85c30a28 100644 --- a/starknet/src/types/indexed_strategy.cairo +++ b/starknet/src/types/indexed_strategy.cairo @@ -1,3 +1,4 @@ +use core::array::SpanTrait; use option::OptionTrait; use serde::Serde; use clone::Clone; @@ -11,28 +12,31 @@ struct IndexedStrategy { } trait IndexedStrategyTrait { - fn assert_no_duplicate_indices(self: @Array); + fn assert_no_duplicate_indices(self: Span); } impl IndexedStrategyImpl of IndexedStrategyTrait { - fn assert_no_duplicate_indices(self: @Array) { + fn assert_no_duplicate_indices(self: Span) { + let mut self = self; if self.len() < 2 { return (); } let mut bit_map = 0_u256; - let mut i = 0_usize; loop { - if i >= self.len() { - break (); - } - // Check that bit at index `strats[i].index` is not set. - let s = math::pow(2_u256, *self.at(i).index); + match self.pop_front() { + Option::Some(indexed_strategy) => { + // Check that bit at index `strats[i].index` is not set. + let s = math::pow(2_u256, *indexed_strategy.index); - assert((bit_map & s) != u256 { low: 1_u128, high: 0_u128 }, 'Duplicate Found'); - // Update aforementioned bit. - bit_map = bit_map | s; - i += 1; + assert((bit_map & s) != 1_u256, 'Duplicate Found'); + // Update aforementioned bit. + bit_map = bit_map | s; + }, + Option::None => { + break; + }, + }; }; } } diff --git a/starknet/src/utils/felt_arr_to_uint_arr.cairo b/starknet/src/utils/felt_arr_to_uint_arr.cairo index 7822e0a2..5811c9d1 100644 --- a/starknet/src/utils/felt_arr_to_uint_arr.cairo +++ b/starknet/src/utils/felt_arr_to_uint_arr.cairo @@ -2,15 +2,17 @@ use array::ArrayTrait; use traits::Into; impl Felt252ArrayIntoU256Array of Into, Array> { - fn into(self: Array) -> Array { - let mut arr = ArrayTrait::::new(); - let mut i = 0_usize; + fn into(mut self: Array) -> Array { + let mut arr = array![]; loop { - if i >= self.len() { - break (); - } - arr.append((*self.at(i)).into()); - i += 1; + match self.pop_front() { + Option::Some(el) => { + arr.append(el.into()); + }, + Option::None => { + break; + } + }; }; arr } diff --git a/starknet/src/utils/proposition_power.cairo b/starknet/src/utils/proposition_power.cairo index c4b3fb66..41360a6e 100644 --- a/starknet/src/utils/proposition_power.cairo +++ b/starknet/src/utils/proposition_power.cairo @@ -16,22 +16,25 @@ use clone::Clone; fn _get_cumulative_power( voter: UserAddress, timestamp: u32, - mut user_strategies: Array, - allowed_strategies: Array, + mut user_strategies: Span, + allowed_strategies: Span, ) -> u256 { user_strategies.assert_no_duplicate_indices(); let mut total_voting_power = 0_u256; loop { match user_strategies.pop_front() { Option::Some(indexed_strategy) => { - match allowed_strategies.get(indexed_strategy.index.into()) { + match allowed_strategies.get((*indexed_strategy.index).into()) { Option::Some(strategy) => { let strategy: Strategy = strategy.unbox().clone(); total_voting_power += IVotingStrategyDispatcher { contract_address: strategy.address } .get_voting_power( - timestamp, voter, strategy.params, indexed_strategy.params, + timestamp, + voter, + strategy.params.span(), + indexed_strategy.params.span(), ); }, Option::None => { @@ -48,22 +51,19 @@ fn _get_cumulative_power( fn _validate( author: UserAddress, - params: Array, // [proposal_threshold: u256, allowed_strategies: Array] - user_params: Array // [user_strategies: Array] + mut params: Span, // [proposal_threshold: u256, allowed_strategies: Array] + mut user_params: Span // [user_strategies: Array] ) -> bool { - let mut params_span = params.span(); let (proposal_threshold, allowed_strategies) = Serde::<( u256, Array - )>::deserialize(ref params_span) + )>::deserialize(ref params) .unwrap(); - let mut user_params_span = user_params.span(); - let user_strategies = Serde::>::deserialize(ref user_params_span) - .unwrap(); + let user_strategies = Serde::>::deserialize(ref user_params).unwrap(); let timestamp: u32 = info::get_block_timestamp().try_into().unwrap() - 1; let voting_power = _get_cumulative_power( - author, timestamp, user_strategies, allowed_strategies + author, timestamp, user_strategies.span(), allowed_strategies.span() ); voting_power >= proposal_threshold } diff --git a/starknet/src/utils/single_slot_proof.cairo b/starknet/src/utils/single_slot_proof.cairo index 1499d30f..7afd218d 100644 --- a/starknet/src/utils/single_slot_proof.cairo +++ b/starknet/src/utils/single_slot_proof.cairo @@ -64,11 +64,10 @@ mod SingleSlotProof { contract_address: felt252, slot_index: u256, mapping_key: u256, - encoded_proofs: Array + mut encoded_proofs: Span ) -> u256 { let slot_key = get_mapping_slot_key(slot_index, mapping_key); - let mut s = encoded_proofs.span(); - let proofs: Proofs = Serde::::deserialize(ref s).unwrap(); + let proofs: Proofs = Serde::::deserialize(ref encoded_proofs).unwrap(); // Check proof corresponds to correct storage slot. assert( diff --git a/starknet/src/voting_strategies/erc20_votes.cairo b/starknet/src/voting_strategies/erc20_votes.cairo index cb3e85af..65e3685f 100644 --- a/starknet/src/voting_strategies/erc20_votes.cairo +++ b/starknet/src/voting_strategies/erc20_votes.cairo @@ -22,15 +22,14 @@ mod ERC20VotesVotingStrategy { self: @ContractState, timestamp: u32, voter: UserAddress, - params: Array, - user_params: Array, + mut params: Span, + user_params: Span, ) -> u256 { // Cast voter address to a Starknet address // Will revert if the address is not a Starknet address let voter = voter.to_starknet_address(); // Get the ERC20 contract address from the params array - let mut params = params.span(); let erc20_contract_address = Serde::::deserialize(ref params).unwrap(); let erc20 = IVotesDispatcher { contract_address: erc20_contract_address, }; diff --git a/starknet/src/voting_strategies/eth_balance_of.cairo b/starknet/src/voting_strategies/eth_balance_of.cairo index a4fca095..ca6c4788 100644 --- a/starknet/src/voting_strategies/eth_balance_of.cairo +++ b/starknet/src/voting_strategies/eth_balance_of.cairo @@ -16,8 +16,8 @@ mod EthBalanceOfVotingStrategy { self: @ContractState, timestamp: u32, voter: UserAddress, - params: Array, - user_params: Array, + params: Span, + user_params: Span, ) -> u256 { // Cast voter address to an Ethereum address // Will revert if the address is not an Ethereum address diff --git a/starknet/src/voting_strategies/merkle_whitelist.cairo b/starknet/src/voting_strategies/merkle_whitelist.cairo index 1cef108a..749d36b0 100644 --- a/starknet/src/voting_strategies/merkle_whitelist.cairo +++ b/starknet/src/voting_strategies/merkle_whitelist.cairo @@ -18,10 +18,10 @@ mod MerkleWhitelistVotingStrategy { self: @ContractState, timestamp: u32, voter: UserAddress, - params: Array, // [root: felt252] - user_params: Array, // [leaf: Leaf, proof: Array)] + params: Span, // [root: felt252] + user_params: Span, // [leaf: Leaf, proof: Array] ) -> u256 { - let cache = user_params.span(); // cache + let cache = user_params; // cache let mut leaf_raw = cache.slice(0, LEAF_SIZE); let leaf = Serde::::deserialize(ref leaf_raw).unwrap(); diff --git a/starknet/src/voting_strategies/vanilla.cairo b/starknet/src/voting_strategies/vanilla.cairo index 038912c6..9b646c17 100644 --- a/starknet/src/voting_strategies/vanilla.cairo +++ b/starknet/src/voting_strategies/vanilla.cairo @@ -12,8 +12,8 @@ mod VanillaVotingStrategy { self: @ContractState, timestamp: u32, voter: UserAddress, - params: Array, - user_params: Array, + params: Span, + user_params: Span, ) -> u256 { 1_u256 }