From 812446739eeab999a55c09e8addd83fb4aa71aeb Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Wed, 30 Aug 2023 13:46:32 +0200 Subject: [PATCH 01/23] add vote_registry view function --- starknet/src/space/space.cairo | 10 +++++----- starknet/src/tests/vote.cairo | 3 +++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index a1e20fe0..85503810 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -16,13 +16,9 @@ trait ISpace { fn active_voting_strategies(self: @TContractState) -> u256; fn next_voting_strategy_index(self: @TContractState) -> u8; fn proposal_validation_strategy(self: @TContractState) -> Strategy; - // #[view] fn vote_power(self: @TContractState, proposal_id: u256, choice: Choice) -> u256; - // #[view] - // fn vote_registry(proposal_id: u256, voter: ContractAddress) -> bool; + fn vote_registry(self: @TContractState, proposal_id: u256, voter: UserAddress) -> bool; fn proposals(self: @TContractState, proposal_id: u256) -> Proposal; - // #[view] - // fn get_proposal_status(proposal_id: u256) -> u8; // Owner Actions fn update_settings(ref self: TContractState, input: UpdateSettingsCalldata); @@ -745,6 +741,10 @@ mod Space { } } + fn vote_registry(self: @ContractState, proposal_id: u256, voter: UserAddress) -> bool { + self._vote_registry.read((proposal_id, voter)) + } + fn vote_power(self: @ContractState, proposal_id: u256, choice: Choice) -> u256 { self._vote_power.read((proposal_id, choice)) } diff --git a/starknet/src/tests/vote.cairo b/starknet/src/tests/vote.cairo index a66822c6..a71ab8b8 100644 --- a/starknet/src/tests/vote.cairo +++ b/starknet/src/tests/vote.cairo @@ -89,6 +89,7 @@ mod tests { ArrayTrait::::new().serialize(ref vote_calldata); authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + assert(space.vote_registry(proposal_id, voter) == true, 'vote registry incorrect'); 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'); @@ -123,6 +124,7 @@ mod tests { ArrayTrait::::new().serialize(ref vote_calldata); authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + assert(space.vote_registry(proposal_id, voter) == true, 'vote registry incorrect'); 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'); @@ -157,6 +159,7 @@ mod tests { ArrayTrait::::new().serialize(ref vote_calldata); authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); + assert(space.vote_registry(proposal_id, voter) == true, 'vote registry incorrect'); 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'); From 7ab6b7c7199aa1e15f1e6ad518156fa734b8cb77 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Wed, 30 Aug 2023 17:07:28 +0200 Subject: [PATCH 02/23] add assert in initializer function; test them --- starknet/src/factory/factory.cairo | 15 ++-- starknet/src/space/space.cairo | 4 + starknet/src/tests/setup/setup.cairo | 41 ++++++++-- starknet/src/tests/test_factory.cairo | 16 +++- starknet/src/tests/test_space.cairo | 103 +++++++++++++++++++++++++- 5 files changed, 158 insertions(+), 21 deletions(-) diff --git a/starknet/src/factory/factory.cairo b/starknet/src/factory/factory.cairo index cf6e1130..89867278 100644 --- a/starknet/src/factory/factory.cairo +++ b/starknet/src/factory/factory.cairo @@ -1,4 +1,4 @@ -use starknet::{ContractAddress, ClassHash}; +use starknet::{ContractAddress, ClassHash, SyscallResult}; #[starknet::interface] trait IFactory { @@ -7,7 +7,7 @@ trait IFactory { class_hash: ClassHash, contract_address_salt: felt252, initialize_calldata: Span - ) -> ContractAddress; + ) -> SyscallResult; } @@ -16,7 +16,7 @@ mod Factory { use super::IFactory; use starknet::{ ContractAddress, ClassHash, contract_address_const, - syscalls::{deploy_syscall, call_contract_syscall} + syscalls::{deploy_syscall, call_contract_syscall}, SyscallResult }; use result::ResultTrait; use array::{ArrayTrait, SpanTrait}; @@ -44,18 +44,17 @@ mod Factory { class_hash: ClassHash, contract_address_salt: felt252, initialize_calldata: Span - ) -> ContractAddress { + ) -> SyscallResult { let (space_address, _) = deploy_syscall( class_hash, contract_address_salt, array![].span(), false - ) - .unwrap(); + )?; // Call initializer. - call_contract_syscall(space_address, INITIALIZE_SELECTOR, initialize_calldata).unwrap(); + call_contract_syscall(space_address, INITIALIZE_SELECTOR, initialize_calldata)?; self.emit(Event::SpaceDeployed(SpaceDeployed { class_hash, space_address })); - space_address + Result::Ok(space_address) } } } diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 85503810..ef49312a 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -294,6 +294,10 @@ mod Space { Reinitializable::unsafe_new_contract_state(); ReinitializableImpl::initialize(ref state); + assert(voting_strategies.len() != 0, 'empty voting strategies'); + assert(authenticators.len() != 0, 'empty authenticators'); + assert(voting_strategies.len() == voting_strategy_metadata_URIs.len(), 'len mismatch'); + //TODO: temporary component syntax let mut state = Ownable::unsafe_new_contract_state(); Ownable::initializer(ref state); diff --git a/starknet/src/tests/setup/setup.cairo b/starknet/src/tests/setup/setup.cairo index 38d4e412..3d060936 100644 --- a/starknet/src/tests/setup/setup.cairo +++ b/starknet/src/tests/setup/setup.cairo @@ -18,6 +18,7 @@ mod setup { use sx::factory::factory::{Factory, IFactoryDispatcher, IFactoryDispatcherTrait}; use starknet::ClassHash; use sx::space::space::{Space, ISpaceDispatcher, ISpaceDispatcherTrait}; + use debug::PrintTrait; #[derive(Drop)] struct Config { @@ -114,7 +115,18 @@ mod setup { proposal_validation_strategy.serialize(ref initializer_calldata); ArrayTrait::::new().serialize(ref initializer_calldata); voting_strategies.serialize(ref initializer_calldata); - ArrayTrait::::new().serialize(ref initializer_calldata); + let mut voting_strategies_metadata_uris: Array> = array![]; + + let mut i = 0; + loop { + if i >= voting_strategies.len() { + break; + } + voting_strategies_metadata_uris.append(array![]); + i += 1; + }; + + voting_strategies_metadata_uris.serialize(ref initializer_calldata); authenticators.serialize(ref initializer_calldata); ArrayTrait::::new().serialize(ref initializer_calldata); ArrayTrait::::new().serialize(ref initializer_calldata); @@ -126,10 +138,17 @@ mod setup { let space_class_hash: ClassHash = Space::TEST_CLASS_HASH.try_into().unwrap(); let contract_address_salt = 0; - let (factory_address, _) = deploy_syscall( - Factory::TEST_CLASS_HASH.try_into().unwrap(), 0, array![].span(), false - ) - .unwrap(); + let factory_address = + match deploy_syscall( + Factory::TEST_CLASS_HASH.try_into().unwrap(), 0, array![].span(), false + ) { + Result::Ok((address, _)) => address, + Result::Err(e) => { + e.print(); + panic_with_felt252('deploy failed'); + contract_address_const::<0>() + } + }; let factory = IFactoryDispatcher { contract_address: factory_address }; @@ -142,8 +161,16 @@ mod setup { config.voting_strategies, config.authenticators ); - let space_address = factory - .deploy(space_class_hash, contract_address_salt, initializer_calldata.span()); + let space_address = + match factory + .deploy(space_class_hash, contract_address_salt, initializer_calldata.span()) { + Result::Ok(address) => address, + Result::Err(e) => { + e.print(); + panic_with_felt252('deploy failed'); + contract_address_const::<0>() + }, + }; let space = ISpaceDispatcher { contract_address: space_address }; diff --git a/starknet/src/tests/test_factory.cairo b/starknet/src/tests/test_factory.cairo index b5f0bd75..42a75b33 100644 --- a/starknet/src/tests/test_factory.cairo +++ b/starknet/src/tests/test_factory.cairo @@ -9,6 +9,7 @@ mod tests { use sx::space::space::Space; use sx::types::Strategy; use starknet::ClassHash; + use debug::PrintTrait; use sx::tests::setup::setup::setup::{setup, get_initialize_calldata, deploy}; @@ -28,10 +29,17 @@ mod tests { fn test_deploy_reuse_salt() { let mut constructor_calldata = array![]; - let (factory_address, _) = deploy_syscall( - Factory::TEST_CLASS_HASH.try_into().unwrap(), 0, constructor_calldata.span(), false - ) - .unwrap(); + let factory_address = + match deploy_syscall( + Factory::TEST_CLASS_HASH.try_into().unwrap(), 0, constructor_calldata.span(), false + ) { + Result::Ok((address, _)) => address, + Result::Err(e) => { + e.print(); + panic_with_felt252('deploy failed'); + contract_address_const::<0>() + }, + }; let factory = IFactoryDispatcher { contract_address: factory_address }; diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index ba651290..3f1919a6 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -87,7 +87,7 @@ mod tests { vanilla_proposal_validation_strategy.clone(), array![], voting_strategies, - array![], + array![array![]], authenticators, array![], array![] @@ -103,6 +103,105 @@ mod tests { ); } + #[test] + #[available_gas(10000000)] + #[should_panic(expected: ('empty voting strategies',))] + fn empty_voting_strategies() { + let mut state = Space::unsafe_new_contract_state(); + let owner = contract_address_const::<0x123456789>(); + let min_voting_duration = 1_u32; + let max_voting_duration = 2_u32; + let voting_delay = 1_u32; + let proposal_validation_strategy = StrategyImpl::test_value(); + let proposal_validation_strategy_metadata_uri = array![]; + let voting_strategies = array![]; + let voting_strategies_metadata_uris = array![]; + let authenticators = array![contract_address_const::<0>()]; + let metadata_uri = array![]; + let dao_uri = array![]; + + Space::Space::initialize( + ref state, + owner, + min_voting_duration, + max_voting_duration, + voting_delay, + proposal_validation_strategy, + proposal_validation_strategy_metadata_uri, + voting_strategies, + voting_strategies_metadata_uris, + authenticators, + metadata_uri, + dao_uri, + ) + } + + #[test] + #[available_gas(10000000)] + #[should_panic(expected: ('empty authenticators',))] + fn empty_authenticators() { + let mut state = Space::unsafe_new_contract_state(); + let owner = contract_address_const::<0x123456789>(); + let min_voting_duration = 1_u32; + let max_voting_duration = 2_u32; + let voting_delay = 1_u32; + let proposal_validation_strategy = StrategyImpl::test_value(); + let proposal_validation_strategy_metadata_uri = array![]; + let voting_strategies = array![StrategyImpl::test_value()]; + let voting_strategies_metadata_uris = array![array![]]; + let authenticators = array![]; + let metadata_uri = array![]; + let dao_uri = array![]; + + Space::Space::initialize( + ref state, + owner, + min_voting_duration, + max_voting_duration, + voting_delay, + proposal_validation_strategy, + proposal_validation_strategy_metadata_uri, + voting_strategies, + voting_strategies_metadata_uris, + authenticators, + metadata_uri, + dao_uri, + ) + } + + #[test] + #[available_gas(10000000)] + #[should_panic(expected: ('len mismatch',))] + fn voting_strategies_and_metadata_uris_mismatch() { + let mut state = Space::unsafe_new_contract_state(); + let owner = contract_address_const::<0x123456789>(); + let min_voting_duration = 1_u32; + let max_voting_duration = 2_u32; + let voting_delay = 1_u32; + let proposal_validation_strategy = StrategyImpl::test_value(); + let proposal_validation_strategy_metadata_uri = array![]; + let voting_strategies = array![StrategyImpl::test_value()]; + let voting_strategies_metadata_uris = array![array![], array![]]; + let authenticators = array![contract_address_const::<0>()]; + let metadata_uri = array![]; + let dao_uri = array![]; + + Space::Space::initialize( + ref state, + owner, + min_voting_duration, + max_voting_duration, + voting_delay, + proposal_validation_strategy, + proposal_validation_strategy_metadata_uri, + voting_strategies, + voting_strategies_metadata_uris, + authenticators, + metadata_uri, + dao_uri, + ) + } + #[test] #[available_gas(100000000)] #[should_panic(expected: ('Already Initialized', 'ENTRYPOINT_FAILED'))] @@ -173,7 +272,7 @@ mod tests { vanilla_proposal_validation_strategy.clone(), array![], voting_strategies.clone(), - array![], + array![array![]], authenticators.clone(), array![], array![] From 072efe727d65824fca791841bff68bf5e575231c Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Wed, 30 Aug 2023 17:53:12 +0200 Subject: [PATCH 03/23] parity for initialize fn --- starknet/src/space/space.cairo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index ef49312a..b06ee4a3 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -306,8 +306,8 @@ mod Space { 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_voting_delay(ref self, voting_delay); _set_proposal_validation_strategy(ref self, proposal_validation_strategy); + _set_voting_delay(ref self, voting_delay); _add_voting_strategies(ref self, voting_strategies.span()); _add_authenticators(ref self, authenticators.span()); self._next_proposal_id.write(1_u256); From af29aaaa26940381b7ada3b5e0eed2c1a16bc76a Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 11:57:18 +0200 Subject: [PATCH 04/23] reorder propose arguments --- starknet/src/authenticators/eth_sig.cairo | 4 ++-- starknet/src/authenticators/eth_tx.cairo | 6 +++--- starknet/src/authenticators/stark_sig.cairo | 4 ++-- starknet/src/authenticators/stark_tx.cairo | 4 ++-- starknet/src/space/space.cairo | 4 ++-- starknet/src/tests/test_space.cairo | 14 +++++++------- starknet/src/tests/test_upgrade.cairo | 2 +- starknet/src/tests/vote.cairo | 2 +- .../src/tests/voting_strategies/erc20_votes.cairo | 6 +++--- 9 files changed, 23 insertions(+), 23 deletions(-) diff --git a/starknet/src/authenticators/eth_sig.cairo b/starknet/src/authenticators/eth_sig.cairo index 86eda08e..768649eb 100644 --- a/starknet/src/authenticators/eth_sig.cairo +++ b/starknet/src/authenticators/eth_sig.cairo @@ -91,9 +91,9 @@ mod EthSigAuthenticator { ISpaceDispatcher { contract_address: target } .propose( UserAddress::Ethereum(author), + metadata_URI, execution_strategy, user_proposal_validation_params, - metadata_URI ); } @@ -129,7 +129,7 @@ mod EthSigAuthenticator { proposal_id, choice, user_voting_strategies, - metadata_URI + metadata_URI, ); } diff --git a/starknet/src/authenticators/eth_tx.cairo b/starknet/src/authenticators/eth_tx.cairo index e9be8b46..4edf7d30 100644 --- a/starknet/src/authenticators/eth_tx.cairo +++ b/starknet/src/authenticators/eth_tx.cairo @@ -65,9 +65,9 @@ mod EthTxAuthenticator { target.serialize(ref payload); PROPOSE_SELECTOR.serialize(ref payload); author.serialize(ref payload); + metadata_URI.serialize(ref payload); execution_strategy.serialize(ref payload); user_proposal_validation_params.serialize(ref payload); - metadata_URI.serialize(ref payload); let payload_hash = poseidon::poseidon_hash_span(payload.span()); consume_commit(ref self, payload_hash, author); @@ -75,9 +75,9 @@ mod EthTxAuthenticator { ISpaceDispatcher { contract_address: target } .propose( UserAddress::Ethereum(author), + metadata_URI, execution_strategy, user_proposal_validation_params, - metadata_URI ); } @@ -108,7 +108,7 @@ mod EthTxAuthenticator { proposal_id, choice, user_voting_strategies, - metadata_URI + metadata_URI, ); } diff --git a/starknet/src/authenticators/stark_sig.cairo b/starknet/src/authenticators/stark_sig.cairo index b69a0dff..8f587013 100644 --- a/starknet/src/authenticators/stark_sig.cairo +++ b/starknet/src/authenticators/stark_sig.cairo @@ -86,9 +86,9 @@ mod StarkSigAuthenticator { ISpaceDispatcher { contract_address: target } .propose( UserAddress::Starknet(author), + metadata_URI, execution_strategy, user_proposal_validation_params, - metadata_URI ); } @@ -123,7 +123,7 @@ mod StarkSigAuthenticator { proposal_id, choice, user_voting_strategies, - metadata_URI + metadata_URI, ); } diff --git a/starknet/src/authenticators/stark_tx.cairo b/starknet/src/authenticators/stark_tx.cairo index ff4e13f6..4e72a2af 100644 --- a/starknet/src/authenticators/stark_tx.cairo +++ b/starknet/src/authenticators/stark_tx.cairo @@ -58,9 +58,9 @@ mod StarkTxAuthenticator { ISpaceDispatcher { contract_address: space } .propose( UserAddress::Starknet(author), + metadata_URI, execution_strategy, user_proposal_validation_params, - metadata_URI ); } @@ -81,7 +81,7 @@ mod StarkTxAuthenticator { proposal_id, choice, user_voting_strategies, - metadata_URI + metadata_URI, ); } diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index b06ee4a3..a9818872 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -42,9 +42,9 @@ trait ISpace { fn propose( ref self: TContractState, author: UserAddress, + metadata_URI: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_URI: Array, ); fn vote( ref self: TContractState, @@ -316,9 +316,9 @@ mod Space { fn propose( ref self: ContractState, author: UserAddress, + metadata_URI: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_URI: Array, ) { assert_only_authenticator(@self); assert(author.is_non_zero(), 'Zero Address'); diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 3f1919a6..04a0f320 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -322,8 +322,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal @@ -431,8 +431,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Try to create Proposal @@ -467,8 +467,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal @@ -530,8 +530,8 @@ mod tests { let mut propose_calldata = array![]; let author = UserAddress::Starknet(contract_address_const::<0x5678>()); author.serialize(ref propose_calldata); - vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal @@ -592,8 +592,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal @@ -629,8 +629,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal @@ -682,8 +682,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal diff --git a/starknet/src/tests/test_upgrade.cairo b/starknet/src/tests/test_upgrade.cairo index a46ed4e6..3720af6a 100644 --- a/starknet/src/tests/test_upgrade.cairo +++ b/starknet/src/tests/test_upgrade.cairo @@ -94,8 +94,8 @@ mod tests { let mut propose_calldata = array![]; UserAddress::Starknet(contract_address_const::<0x7676>()).serialize(ref propose_calldata); - execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); let authenticator = IVanillaAuthenticatorDispatcher { diff --git a/starknet/src/tests/vote.cairo b/starknet/src/tests/vote.cairo index a71ab8b8..f5fe4d80 100644 --- a/starknet/src/tests/vote.cairo +++ b/starknet/src/tests/vote.cairo @@ -52,8 +52,8 @@ mod tests { 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); + execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal diff --git a/starknet/src/tests/voting_strategies/erc20_votes.cairo b/starknet/src/tests/voting_strategies/erc20_votes.cairo index cbb33210..c0c281f3 100644 --- a/starknet/src/tests/voting_strategies/erc20_votes.cairo +++ b/starknet/src/tests/voting_strategies/erc20_votes.cairo @@ -170,8 +170,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create a proposal @@ -217,8 +217,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create proposal @@ -267,8 +267,8 @@ mod tests { 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); + vanilla_execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create proposal From 4d143c7e676cece3a62608bca8017aced7193372 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 12:17:32 +0200 Subject: [PATCH 05/23] add comment regarding testing assert_proposal_exists --- starknet/src/space/space.cairo | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index a9818872..887db5e4 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -778,7 +778,9 @@ mod Space { } fn assert_proposal_exists(proposal: @Proposal) { - assert(!(*proposal.start_timestamp).is_zero(), 'Proposal does not exist'); + assert( + *proposal.start_timestamp != 0, 'Proposal does not exist' + ); // TODO: test this assertion } fn _get_cumulative_power( From 8df0316d0c564dbdc477b69162a69751f39dc2ba Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 12:28:13 +0200 Subject: [PATCH 06/23] reorder proposal_id in fn propose --- starknet/src/space/space.cairo | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 887db5e4..e6b6cdc6 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -322,7 +322,6 @@ mod Space { ) { assert_only_authenticator(@self); assert(author.is_non_zero(), 'Zero Address'); - let proposal_id = self._next_proposal_id.read(); // Proposal Validation let proposal_validation_strategy = self._proposal_validation_strategy.read(); @@ -358,10 +357,9 @@ mod Space { finalization_status: FinalizationStatus::Pending(()), active_voting_strategies: self._active_voting_strategies.read() }; - 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); + let proposal_id = self._next_proposal_id.read(); + self._proposals.write(proposal_id, proposal.clone()); self._next_proposal_id.write(proposal_id + 1_u256); @@ -371,7 +369,7 @@ mod Space { ProposalCreated { proposal_id: proposal_id, author: author, - proposal: clone_proposal, // TODO: use span, remove clone + proposal, payload: execution_strategy.params.span(), metadata_URI: metadata_URI.span() } From 53ec2b3d312b774801a98d027e26e56d70b0302d Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 12:35:11 +0200 Subject: [PATCH 07/23] remove useless cote in test_vote_zero_address --- starknet/src/tests/test_space.cairo | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 04a0f320..1b17359c 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -689,23 +689,6 @@ mod tests { // 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); @@ -717,9 +700,9 @@ mod tests { 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() }); + let mut user_voting_strategies = array![ + IndexedStrategy { index: 0_u8, params: ArrayTrait::::new() } + ]; user_voting_strategies.serialize(ref vote_calldata); ArrayTrait::::new().serialize(ref vote_calldata); From c0c6011f3484b8e7e231b1f48b3e56d6bf849a89 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 12:40:41 +0200 Subject: [PATCH 08/23] re-order proposalcreated event --- starknet/src/space/space.cairo | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index e6b6cdc6..64204951 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -163,8 +163,8 @@ mod Space { proposal_id: u256, author: UserAddress, proposal: Proposal, - payload: Span, metadata_URI: Span, + payload: Span, } #[derive(Drop, starknet::Event)] @@ -361,7 +361,7 @@ mod Space { let proposal_id = self._next_proposal_id.read(); self._proposals.write(proposal_id, proposal.clone()); - self._next_proposal_id.write(proposal_id + 1_u256); + self._next_proposal_id.write(proposal_id + 1); self .emit( @@ -370,8 +370,8 @@ mod Space { proposal_id: proposal_id, author: author, proposal, + metadata_URI: metadata_URI.span(), payload: execution_strategy.params.span(), - metadata_URI: metadata_URI.span() } ) ); From 5989b97b8eb3c984980da3ca1901a923453d2c2f Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 12:53:25 +0200 Subject: [PATCH 09/23] parity with vote function --- starknet/src/space/space.cairo | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 64204951..2ca4ef6b 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -386,9 +386,9 @@ mod Space { metadata_URI: Array ) { assert_only_authenticator(@self); - assert(voter.is_non_zero(), 'Zero Address'); + assert(voter.is_non_zero(), 'Zero Address'); // TODO: test this branch let proposal = self._proposals.read(proposal_id); - assert_proposal_exists(@proposal); + assert_proposal_exists(@proposal); // TODO: test this branch let timestamp = info::get_block_timestamp().try_into().unwrap(); @@ -402,6 +402,9 @@ mod Space { self._vote_registry.read((proposal_id, voter)) == false, 'Voter has already voted' ); + // Written here to prevent re-entrency attacks via malicious voting strategies + self._vote_registry.write((proposal_id, voter), true); + let voting_power = _get_cumulative_power( @self, voter, @@ -409,16 +412,17 @@ mod Space { user_voting_strategies.span(), proposal.active_voting_strategies ); + assert(voting_power > 0, 'User has no voting power'); - assert(voting_power > 0_u256, 'User has no voting power'); self ._vote_power .write( (proposal_id, choice), self._vote_power.read((proposal_id, choice)) + voting_power ); - self._vote_registry.write((proposal_id, voter), true); + // Contrary to the SX-EVM implementation, we don't differentiate between `VoteCast` and `VoteCastWithMetadata` + // because calldata is free. self .emit( Event::VoteCast( From 0cf5e3716ea7831f80e3755e340b18e62157c475 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 13:02:43 +0200 Subject: [PATCH 10/23] prevent re-entrency attacks in execute fn; parity with execute function --- starknet/src/space/space.cairo | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 2ca4ef6b..0f75e81f 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -437,6 +437,7 @@ mod Space { ); } + // TODO: missing `nonReentrant` modifier fn execute(ref self: ContractState, proposal_id: u256, execution_payload: Array) { let mut proposal = self._proposals.read(proposal_id); assert_proposal_exists(@proposal); @@ -450,19 +451,22 @@ mod Space { proposal.finalization_status == FinalizationStatus::Pending(()), 'Already finalized' ); - IExecutionStrategyDispatcher { contract_address: proposal.execution_strategy } + // We cache the proposal to prevent re-entrency attacks by setting + // the finalization status to `Executed` before calling the `execute` function. + let cached_proposal = proposal.clone(); + proposal.finalization_status = FinalizationStatus::Executed(()); + + self._proposals.write(proposal_id, proposal); + + IExecutionStrategyDispatcher { contract_address: cached_proposal.execution_strategy } .execute( - proposal.clone(), + cached_proposal, self._vote_power.read((proposal_id, Choice::For(()))), self._vote_power.read((proposal_id, Choice::Against(()))), self._vote_power.read((proposal_id, Choice::Abstain(()))), execution_payload ); - proposal.finalization_status = FinalizationStatus::Executed(()); - - self._proposals.write(proposal_id, proposal); - self.emit(Event::ProposalExecuted(ProposalExecuted { proposal_id: proposal_id })); } From 5d7fc526a0af498687193c4734cc3de08eea3478 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 13:05:17 +0200 Subject: [PATCH 11/23] reorder function delclaration; rename cancel_upgrade to cancel --- starknet/src/space/space.cairo | 32 ++++++++++++++--------------- starknet/src/tests/test_space.cairo | 2 +- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 0f75e81f..2514cf6a 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -62,7 +62,7 @@ trait ISpace { execution_strategy: Strategy, metadata_URI: Array, ); - fn cancel_proposal(ref self: TContractState, proposal_id: u256); + fn cancel(ref self: TContractState, proposal_id: u256); fn upgrade( ref self: TContractState, class_hash: ClassHash, initialize_calldata: Array ); @@ -470,6 +470,21 @@ mod Space { self.emit(Event::ProposalExecuted(ProposalExecuted { proposal_id: proposal_id })); } + fn cancel(ref self: ContractState, proposal_id: u256) { + //TODO: temporary component syntax + let state = Ownable::unsafe_new_contract_state(); + Ownable::assert_only_owner(@state); + let mut proposal = self._proposals.read(proposal_id); + assert_proposal_exists(@proposal); + assert( + proposal.finalization_status == FinalizationStatus::Pending(()), 'Already Finalized' + ); + proposal.finalization_status = FinalizationStatus::Cancelled(()); + self._proposals.write(proposal_id, proposal); + + self.emit(Event::ProposalCancelled(ProposalCancelled { proposal_id: proposal_id })); + } + fn update_proposal( ref self: ContractState, author: UserAddress, @@ -507,21 +522,6 @@ mod Space { ); } - fn cancel_proposal(ref self: ContractState, proposal_id: u256) { - //TODO: temporary component syntax - let state = Ownable::unsafe_new_contract_state(); - Ownable::assert_only_owner(@state); - let mut proposal = self._proposals.read(proposal_id); - assert_proposal_exists(@proposal); - assert( - proposal.finalization_status == FinalizationStatus::Pending(()), 'Already Finalized' - ); - proposal.finalization_status = FinalizationStatus::Cancelled(()); - self._proposals.write(proposal_id, proposal); - - self.emit(Event::ProposalCancelled(ProposalCancelled { proposal_id: proposal_id })); - } - fn upgrade( ref self: ContractState, class_hash: ClassHash, initialize_calldata: Array ) { diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 1b17359c..450cfee2 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -545,7 +545,7 @@ mod tests { // Cancel Proposal testing::set_caller_address(config.owner); testing::set_contract_address(config.owner); - space.cancel_proposal(proposal_id); + space.cancel(proposal_id); let proposal = space.proposals(proposal_id); assert(proposal.finalization_status == FinalizationStatus::Cancelled(()), 'cancelled'); From b09524b77df3691249d89e4593a8abed8aa79a45 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 17:56:49 +0200 Subject: [PATCH 12/23] parity with cancel function ; add tests --- starknet/src/space/space.cairo | 3 +- starknet/src/tests/test_space.cairo | 81 ++++++++++++++++++++++++----- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 2514cf6a..41f99e77 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -474,10 +474,11 @@ mod Space { //TODO: temporary component syntax let state = Ownable::unsafe_new_contract_state(); Ownable::assert_only_owner(@state); + let mut proposal = self._proposals.read(proposal_id); assert_proposal_exists(@proposal); assert( - proposal.finalization_status == FinalizationStatus::Pending(()), 'Already Finalized' + proposal.finalization_status == FinalizationStatus::Pending(()), 'Already finalized' ); proposal.finalization_status = FinalizationStatus::Cancelled(()); self._proposals.write(proposal_id, proposal); diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 450cfee2..ce96cf91 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -26,6 +26,7 @@ mod tests { }; use sx::tests::utils::strategy_trait::{StrategyImpl}; use sx::utils::constants::{PROPOSE_SELECTOR, VOTE_SELECTOR, UPDATE_PROPOSAL_SELECTOR}; + use sx::tests::mocks::executor::ExecutorWithoutTxExecutionStrategy; use Space::Space as SpaceImpl; @@ -563,6 +564,71 @@ mod tests { authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); } + #[test] + #[available_gas(1000000000)] + #[should_panic(expected: ('Caller is not the owner',))] + fn cancel_unauthorized() { + let mut state = Space::unsafe_new_contract_state(); + + testing::set_caller_address(contract_address_const::<'random'>()); + Space::Space::cancel(ref state, 0); + } + + #[test] + #[available_gas(1000000000)] + #[should_panic( + expected: ('Unknown enum indicator:', 0, 'ENTRYPOINT_FAILED') + )] // TODO: replace once `default` works on Proposal + // #[should_panic(expected: ('Proposal does not exist', 'ENTRYPOINT_FAILED'))] + fn cancel_inexistent_proposal() { + let config = setup(); + let (factory, space) = deploy(@config); + + testing::set_contract_address(config.owner); + space.cancel(0); + } + + #[test] + #[available_gas(1000000000)] + #[should_panic(expected: ('Already finalized', 'ENTRYPOINT_FAILED'))] + fn cancel_already_finalized() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let (executor_address, _) = deploy_syscall( + ExecutorWithoutTxExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + array![].span(), + false + ) + .unwrap(); + + let execution_strategy = StrategyImpl::from_address(executor_address); + let author = UserAddress::Starknet(contract_address_const::<'author'>()); + let mut propose_calldata = array![]; + author.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + + testing::set_block_timestamp(config.voting_delay + config.max_voting_duration); + + // Execute Proposal + space.execute(1, array![]); + + testing::set_contract_address(config.owner); + // Cancel the proposal + space.cancel(1); + } + + #[test] #[available_gas(10000000000)] #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED'))] @@ -669,17 +735,8 @@ mod tests { 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 vanilla_execution_strategy = StrategyImpl::test_value(); + let author = UserAddress::Starknet(contract_address_const::<'author'>()); let mut propose_calldata = array::ArrayTrait::::new(); author.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); @@ -696,7 +753,7 @@ mod tests { // 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); + let proposal_id = 1_u256; proposal_id.serialize(ref vote_calldata); let choice = Choice::For(()); choice.serialize(ref vote_calldata); From 6bff80e9635b41ddde1e496cead89eac6a048b58 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 18:01:20 +0200 Subject: [PATCH 13/23] add invalid payload test for execute --- starknet/src/tests/test_space.cairo | 51 ++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index ce96cf91..70ae5473 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -502,6 +502,41 @@ mod tests { space.execute(u256_from_felt252(1), vanilla_execution_strategy.params.clone()); } + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Invalid payload hash', 'ENTRYPOINT_FAILED'))] + fn execute_invalid_payload() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let (executor_address, _) = deploy_syscall( + ExecutorWithoutTxExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), + 0, + array![].span(), + false + ) + .unwrap(); + let execution_strategy = StrategyImpl::from_address(executor_address); + let author = UserAddress::Starknet(contract_address_const::<0x5678>()); + let mut propose_calldata = array![]; + author.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + + testing::set_block_timestamp(config.voting_delay + config.max_voting_duration); + + // Execute Proposal + space.execute(1, array!['random', 'stuff']); + } + #[test] #[available_gas(10000000000)] #[should_panic(expected: ('Proposal has been finalized', 'ENTRYPOINT_FAILED'))] @@ -514,25 +549,19 @@ mod tests { 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(), + let (executor_address, _) = deploy_syscall( + ExecutorWithoutTxExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), 0, - constructor_calldata.span(), + array![].span(), false ) .unwrap(); - let vanilla_execution_strategy = StrategyImpl::from_address( - vanilla_execution_strategy_address - ); + let execution_strategy = StrategyImpl::from_address(executor_address); let mut propose_calldata = array![]; let author = UserAddress::Starknet(contract_address_const::<0x5678>()); author.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); - vanilla_execution_strategy.serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal From 0ab1887c9e38adcaebc9f89a7a13477b64ad337a Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 18:04:50 +0200 Subject: [PATCH 14/23] remove useless imports in test_upgrade --- starknet/src/tests/test_upgrade.cairo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/starknet/src/tests/test_upgrade.cairo b/starknet/src/tests/test_upgrade.cairo index 3720af6a..5a73eff6 100644 --- a/starknet/src/tests/test_upgrade.cairo +++ b/starknet/src/tests/test_upgrade.cairo @@ -24,7 +24,7 @@ mod tests { UserAddress, Strategy, IndexedStrategy, Choice, FinalizationStatus, Proposal, UpdateSettingsCalldataImpl }; - use sx::utils::constants::{PROPOSE_SELECTOR, VOTE_SELECTOR, UPDATE_PROPOSAL_SELECTOR}; + use sx::utils::constants::{PROPOSE_SELECTOR}; use sx::tests::setup::setup::setup::{setup, deploy}; use sx::interfaces::{ IProposalValidationStrategyDispatcher, IProposalValidationStrategyDispatcherTrait From 687b36417d740eebf9239678bc8ebdee1a096e76 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 18:10:14 +0200 Subject: [PATCH 15/23] reorder update_proposal; add missing assert in update_proposal --- starknet/src/space/space.cairo | 16 +++++---- starknet/src/tests/test_space.cairo | 54 +++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 41f99e77..e2be0212 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -493,21 +493,23 @@ mod Space { execution_strategy: Strategy, metadata_URI: Array, ) { - assert_only_authenticator(@self); - assert(author.is_non_zero(), 'Zero Address'); + assert_only_authenticator(@self); // TODO: test this branch + assert(author.is_non_zero(), 'Zero Address'); // TODO: test this branch ? let mut proposal = self._proposals.read(proposal_id); - assert_proposal_exists(@proposal); - assert(proposal.author == author, 'Only Author'); + assert_proposal_exists(@proposal); // TODO: test this branch + assert( + proposal.finalization_status == FinalizationStatus::Pending(()), 'Already finalized' + ); + assert(proposal.author == author, 'Invalid caller'); // TODO: test this branch assert( info::get_block_timestamp() < proposal.start_timestamp.into(), 'Voting period started' - ); - - proposal.execution_strategy = execution_strategy.address; + ); // TODO: test this branch proposal .execution_payload_hash = poseidon::poseidon_hash_span(execution_strategy.params.span()); + proposal.execution_strategy = execution_strategy.address; self._proposals.write(proposal_id, proposal); diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 70ae5473..05e48f01 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -749,6 +749,60 @@ mod tests { .authenticate(space.contract_address, UPDATE_PROPOSAL_SELECTOR, update_calldata); } + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('Zero Address', '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); + ArrayTrait::::new().serialize(ref propose_calldata); + vanilla_execution_strategy.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'))] From 08fdc5ad5c95b97e17647607bb7cfa97967b78b7 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 18:52:26 +0200 Subject: [PATCH 16/23] add tests for assertions in update_proposal --- starknet/src/space/space.cairo | 10 +- starknet/src/tests/test_space.cairo | 189 +++++++++++++++++++++------- 2 files changed, 152 insertions(+), 47 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index e2be0212..8d916025 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -493,18 +493,18 @@ mod Space { execution_strategy: Strategy, metadata_URI: Array, ) { - assert_only_authenticator(@self); // TODO: test this branch - assert(author.is_non_zero(), 'Zero Address'); // TODO: test this branch ? + assert_only_authenticator(@self); + assert(author.is_non_zero(), 'Zero Address'); let mut proposal = self._proposals.read(proposal_id); - assert_proposal_exists(@proposal); // TODO: test this branch + assert_proposal_exists(@proposal); assert( proposal.finalization_status == FinalizationStatus::Pending(()), 'Already finalized' ); - assert(proposal.author == author, 'Invalid caller'); // TODO: test this branch + assert(proposal.author == author, 'Invalid caller'); assert( info::get_block_timestamp() < proposal.start_timestamp.into(), 'Voting period started' - ); // TODO: test this branch + ); proposal .execution_payload_hash = diff --git a/starknet/src/tests/test_space.cairo b/starknet/src/tests/test_space.cairo index 05e48f01..4c458c36 100644 --- a/starknet/src/tests/test_space.cairo +++ b/starknet/src/tests/test_space.cairo @@ -706,26 +706,13 @@ mod tests { 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 execution_strategy = StrategyImpl::test_value(); // 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); ArrayTrait::::new().serialize(ref propose_calldata); - vanilla_execution_strategy.serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal @@ -737,22 +724,20 @@ mod tests { 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); + let mut new_execution_strategy = execution_strategy; + new_execution_strategy.params = array!['random', 'stuff']; + new_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'))] - fn test_update_zero_address() { + #[should_panic(expected: ('Already finalized', 'ENTRYPOINT_FAILED'))] + fn update_already_finalized() { let config = setup(); let (factory, space) = deploy(@config); @@ -760,49 +745,169 @@ mod tests { 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(), + let (executor_address, _) = deploy_syscall( + ExecutorWithoutTxExecutionStrategy::TEST_CLASS_HASH.try_into().unwrap(), 0, - constructor_calldata.span(), + array![].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 execution_strategy = StrategyImpl::from_address(executor_address); + + let author = UserAddress::Starknet(contract_address_const::<'author'>()); let mut propose_calldata = array::ArrayTrait::::new(); author.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); - vanilla_execution_strategy.serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); ArrayTrait::::new().serialize(ref propose_calldata); // Create Proposal authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); - // Update Proposal + // Execute proposal + space.execute(1, array![]); + + // Try to 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 + let mut new_execution_strategy = execution_strategy; + new_execution_strategy.params = array!['random', 'stuff']; + new_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: ('Invalid caller', 'ENTRYPOINT_FAILED'))] + fn update_invalid_caller() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), }; - execution_strategy.serialize(ref update_calldata); + + let execution_strategy = StrategyImpl::test_value(); + let author = UserAddress::Starknet(contract_address_const::<'author'>()); + let mut propose_calldata = array::ArrayTrait::::new(); + author.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + + // Try to update Proposal + let mut update_calldata = array::ArrayTrait::::new(); + + // author is different this time + let author = UserAddress::Starknet(contract_address_const::<'author2'>()); + 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_execution_strategy = execution_strategy; + new_execution_strategy.params = array!['random', 'stuff']; + new_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: ('Voting period started', 'ENTRYPOINT_FAILED'))] + fn update_voting_period_started() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + let execution_strategy = StrategyImpl::test_value(); + let author = UserAddress::Starknet(contract_address_const::<'author'>()); + let mut propose_calldata = array::ArrayTrait::::new(); + author.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + execution_strategy.serialize(ref propose_calldata); + ArrayTrait::::new().serialize(ref propose_calldata); + + // Create Proposal + authenticator.authenticate(space.contract_address, PROPOSE_SELECTOR, propose_calldata); + + // Skip voting delay + testing::set_block_timestamp(config.voting_delay); + + // Try to 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_execution_strategy = execution_strategy; + new_execution_strategy.params = array!['random', 'stuff']; + new_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: ('Unknown enum indicator:', 'ENTRYPOINT_FAILED') + )] // TODO: replace once `default` works on Proposal + // #[should_panic(expected: ('Proposal does not exist', 'ENTRYPOINT_FAILED'))] + fn update_inexistent_proposal() { + let config = setup(); + let (factory, space) = deploy(@config); + + let authenticator = IVanillaAuthenticatorDispatcher { + contract_address: *config.authenticators.at(0), + }; + + // Update Proposal + let mut update_calldata = array::ArrayTrait::::new(); + let author = UserAddress::Starknet(contract_address_const::<'author'>()); + author.serialize(ref update_calldata); + let proposal_id = 1_u256; + proposal_id.serialize(ref update_calldata); + // Keeping the same execution strategy contract but changing the payload + let new_execution_strategy = StrategyImpl::test_value(); + new_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(1000000000)] + #[should_panic(expected: ('Caller is not an authenticator',))] + fn update_unauthorized() { + let mut state = Space::unsafe_new_contract_state(); + + let author = UserAddress::Starknet(contract_address_const::<'author'>()); + let proposal_id = 1; + let execution_strategy = StrategyImpl::test_value(); + let metadata_uri = array![]; + testing::set_caller_address(contract_address_const::<'random'>()); + Space::Space::update_proposal( + ref state, author, proposal_id, execution_strategy, metadata_uri + ); + } + #[test] #[available_gas(10000000000)] #[should_panic(expected: ('Zero Address', 'ENTRYPOINT_FAILED'))] From d4e7476337115816ddc33b98bac194d1bd72ed64 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Thu, 31 Aug 2023 19:05:06 +0200 Subject: [PATCH 17/23] return a SyscallResult in upgrade --- starknet/src/space/space.cairo | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 8d916025..c6f36e81 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -1,6 +1,6 @@ use core::traits::TryInto; use core::traits::Destruct; -use starknet::{ClassHash, ContractAddress}; +use starknet::{ClassHash, ContractAddress, SyscallResult}; use sx::types::{UserAddress, Strategy, Proposal, IndexedStrategy, Choice, UpdateSettingsCalldata}; #[starknet::interface] @@ -65,7 +65,7 @@ trait ISpace { fn cancel(ref self: TContractState, proposal_id: u256); fn upgrade( ref self: TContractState, class_hash: ClassHash, initialize_calldata: Array - ); + ) -> SyscallResult<()>; } #[starknet::contract] @@ -73,7 +73,7 @@ mod Space { use super::ISpace; use starknet::{ storage_access::{StorePacking, StoreUsingPacking}, ClassHash, ContractAddress, info, Store, - syscalls + syscalls, SyscallResult, }; use zeroable::Zeroable; use array::{ArrayTrait, SpanTrait}; @@ -527,12 +527,12 @@ mod Space { fn upgrade( ref self: ContractState, class_hash: ClassHash, initialize_calldata: Array - ) { + ) -> SyscallResult<()> { let state: Ownable::ContractState = Ownable::unsafe_new_contract_state(); Ownable::assert_only_owner(@state); assert(class_hash.is_non_zero(), 'Class Hash cannot be zero'); - starknet::replace_class_syscall(class_hash).unwrap(); + starknet::replace_class_syscall(class_hash)?; // Allowing initializer to be called again. let mut state: Reinitializable::ContractState = @@ -542,8 +542,7 @@ mod Space { // Call initializer on the new version. syscalls::call_contract_syscall( info::get_contract_address(), INITIALIZE_SELECTOR, initialize_calldata.span() - ) - .unwrap(); + )?; self .emit( @@ -553,6 +552,7 @@ mod Space { } ) ); + SyscallResult::Ok(()) } fn owner(self: @ContractState) -> ContractAddress { From 19c81a289c6281e02ef3c0fea426245ea830c92d Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Fri, 1 Sep 2023 17:29:44 +0200 Subject: [PATCH 18/23] add set_dao_uri / test update dao_uri --- starknet/src/space/space.cairo | 51 ++++++++++++------- starknet/src/tests/test_update_settings.cairo | 8 +-- starknet/src/tests/vote.cairo | 1 + .../tests/voting_strategies/erc20_votes.cairo | 1 + .../src/types/update_settings_calldata.cairo | 12 ++--- 5 files changed, 45 insertions(+), 28 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index be2536ba..61f703c2 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -9,6 +9,7 @@ trait ISpace { fn owner(self: @TContractState) -> ContractAddress; fn max_voting_duration(self: @TContractState) -> u32; fn min_voting_duration(self: @TContractState) -> u32; + fn dao_uri(self: @TContractState) -> Array; fn next_proposal_id(self: @TContractState) -> u256; fn voting_delay(self: @TContractState) -> u32; fn authenticators(self: @TContractState, account: ContractAddress) -> bool; @@ -90,7 +91,7 @@ mod Space { types::{ UserAddress, Choice, FinalizationStatus, Strategy, IndexedStrategy, Proposal, PackedProposal, IndexedStrategyTrait, IndexedStrategyImpl, UpdateSettingsCalldata, - NoUpdateTrait, NoUpdateString, + NoUpdateTrait, NoUpdateString, strategy::StoreFelt252Array, }, utils::{ reinitializable::{Reinitializable}, ReinitializableImpl, bits::BitSetter, @@ -103,13 +104,13 @@ mod Space { }; use hash::{HashStateTrait, Hash, HashStateExTrait}; - #[storage] struct Storage { _min_voting_duration: u32, _max_voting_duration: u32, _next_proposal_id: u256, _voting_delay: u32, + _dao_uri: Array, _active_voting_strategies: u256, _voting_strategies: LegacyMap::, _next_voting_strategy_index: u8, @@ -302,6 +303,7 @@ mod Space { let mut state = Ownable::unsafe_new_contract_state(); Ownable::initializer(ref state); Ownable::transfer_ownership(ref state, owner); + _set_dao_uri(ref self, dao_URI); _set_max_voting_duration( ref self, max_voting_duration ); // Need to set max before min, or else `max == 0` and set_min will revert @@ -577,6 +579,10 @@ mod Space { self._voting_delay.read() } + fn dao_uri(self: @ContractState) -> Array { + self._dao_uri.read() + } + fn authenticators(self: @ContractState, account: ContractAddress) -> bool { self._authenticators.read(account) } @@ -669,6 +675,20 @@ mod Space { ); } + if NoUpdateString::should_update((@input).metadata_URI) { + self + .emit( + Event::MetadataUriUpdated( + MetadataUriUpdated { metadata_URI: input.metadata_URI.span() } + ) + ); + } + + if NoUpdateString::should_update((@input).dao_URI) { + _set_dao_uri(ref self, input.dao_URI.clone()); + self.emit(Event::DaoUriUpdated(DaoUriUpdated { dao_URI: input.dao_URI.span() })); + } + if input.proposal_validation_strategy.should_update() { _set_proposal_validation_strategy( ref self, input.proposal_validation_strategy.clone() @@ -713,6 +733,14 @@ mod Space { } if input.voting_strategies_to_add.should_update() { + assert( + input + .voting_strategies_to_add + .len() == input + .voting_strategies_metadata_URIs_to_add + .len(), + 'len mismatch' + ); // TODO :test this branch _add_voting_strategies(ref self, input.voting_strategies_to_add.span()); self .emit( @@ -738,21 +766,6 @@ mod Space { ) ); } - - // TODO: test once #506 is merged - if NoUpdateString::should_update((@input).metadata_URI) { - self - .emit( - Event::MetadataUriUpdated( - MetadataUriUpdated { metadata_URI: input.metadata_URI.span() } - ) - ); - } - - // TODO: test once #506 is merged - if NoUpdateString::should_update((@input).dao_URI) { - self.emit(Event::DaoUriUpdated(DaoUriUpdated { dao_URI: input.dao_URI.span() })); - } } fn vote_registry(self: @ContractState, proposal_id: u256, voter: UserAddress) -> bool { @@ -838,6 +851,10 @@ mod Space { self._voting_delay.write(_voting_delay); } + fn _set_dao_uri(ref self: ContractState, _dao_uri: Array) { + self._dao_uri.write(_dao_uri); + } + fn _set_proposal_validation_strategy( ref self: ContractState, _proposal_validation_strategy: Strategy ) { diff --git a/starknet/src/tests/test_update_settings.cairo b/starknet/src/tests/test_update_settings.cairo index b46caed1..ab3c581e 100644 --- a/starknet/src/tests/test_update_settings.cairo +++ b/starknet/src/tests/test_update_settings.cairo @@ -154,11 +154,10 @@ mod tests { fn dao_uri() { let (config, space) = setup_update_settings(); let mut input = UpdateSettingsCalldataImpl::default(); - let mut arr = array![]; - 'hello!'.serialize(ref arr); - input.dao_URI = arr; + input.dao_URI = array!['hello!']; space.update_settings(input.clone()); + assert(space.dao_uri() == input.dao_URI, 'dao uri not updated'); // TODO: check event once it's been added } @@ -228,6 +227,7 @@ mod tests { let mut arr = array![vs1.clone(), vs2.clone()]; input.voting_strategies_to_add = arr; + input.voting_strategies_metadata_URIs_to_add = array![array![], array![]]; space.update_settings(input); @@ -235,7 +235,6 @@ mod tests { assert(space.voting_strategies(2) == vs2, 'Voting strategy 2 not added'); assert(space.active_voting_strategies() == 0b111, 'Voting strategies not active'); // TODO: check event once it's been added - // voting_strategies_metadata_URIs_to_add: Array>, } @@ -249,6 +248,7 @@ mod tests { let vs1 = StrategyImpl::from_address(contract_address_const::<'votingStrategy1'>()); let mut arr = array![vs1.clone()]; input.voting_strategies_to_add = arr; + input.voting_strategies_metadata_URIs_to_add = array![array![]]; space.update_settings(input); assert(space.voting_strategies(1) == vs1, 'Voting strategy 1 not added'); assert(space.active_voting_strategies() == 0b11, 'Voting strategy not active'); diff --git a/starknet/src/tests/vote.cairo b/starknet/src/tests/vote.cairo index eeff5845..4ae9cf08 100644 --- a/starknet/src/tests/vote.cairo +++ b/starknet/src/tests/vote.cairo @@ -352,6 +352,7 @@ mod tests { let mut input = UpdateSettingsCalldataImpl::default(); input.voting_strategies_to_add = array![no_voting_power_strategy]; + input.voting_strategies_metadata_URIs_to_add = array![array![]]; testing::set_contract_address(config.owner); space.update_settings(input); diff --git a/starknet/src/tests/voting_strategies/erc20_votes.cairo b/starknet/src/tests/voting_strategies/erc20_votes.cairo index f96c6279..92f05cc2 100644 --- a/starknet/src/tests/voting_strategies/erc20_votes.cairo +++ b/starknet/src/tests/voting_strategies/erc20_votes.cairo @@ -82,6 +82,7 @@ mod tests { let to_add = array![erc20_voting_strategy]; let mut settings = UpdateSettingsCalldataImpl::default(); settings.voting_strategies_to_add = to_add; + settings.voting_strategies_metadata_URIs_to_add = array![array![]]; settings.voting_strategies_to_remove = to_remove; testing::set_contract_address(config.owner); diff --git a/starknet/src/types/update_settings_calldata.cairo b/starknet/src/types/update_settings_calldata.cairo index da73883e..96f1ae8f 100644 --- a/starknet/src/types/update_settings_calldata.cairo +++ b/starknet/src/types/update_settings_calldata.cairo @@ -1,5 +1,3 @@ -use array::ArrayTrait; -use clone::Clone; use starknet::{ContractAddress, contract_address_const}; use sx::types::Strategy; @@ -87,7 +85,7 @@ impl NoUpdateString of NoUpdateTrait> { fn should_update(self: @Array) -> bool { match self.get(0) { Option::Some(e) => { - *e.unbox() == 'No update' + *e.unbox() != 'No update' }, Option::None => false, } @@ -111,14 +109,14 @@ impl UpdateSettingsCalldataImpl of UpdateSettingsCalldataTrait { min_voting_duration: NoUpdateU32::no_update(), max_voting_duration: NoUpdateU32::no_update(), voting_delay: NoUpdateU32::no_update(), - metadata_URI: NoUpdateArray::no_update(), // TODO: string - dao_URI: NoUpdateArray::no_update(), // TODO: string + metadata_URI: NoUpdateString::no_update(), + dao_URI: NoUpdateString::no_update(), proposal_validation_strategy: NoUpdateStrategy::no_update(), - proposal_validation_strategy_metadata_URI: NoUpdateArray::no_update(), // TODO: string + proposal_validation_strategy_metadata_URI: NoUpdateString::no_update(), // TODO: string authenticators_to_add: NoUpdateArray::no_update(), authenticators_to_remove: NoUpdateArray::no_update(), voting_strategies_to_add: NoUpdateArray::no_update(), - voting_strategies_metadata_URIs_to_add: NoUpdateArray::no_update(), // TODO: string + voting_strategies_metadata_URIs_to_add: NoUpdateArray::no_update(), voting_strategies_to_remove: NoUpdateArray::no_update(), } } From 462b9780ee0ff26b4084817824e0038f0e3fe275 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Fri, 1 Sep 2023 17:38:57 +0200 Subject: [PATCH 19/23] finish testing branches --- starknet/src/space/space.cairo | 6 ++-- starknet/src/tests/test_update_settings.cairo | 22 ++++++++++++ starknet/src/tests/vote.cairo | 34 +++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/starknet/src/space/space.cairo b/starknet/src/space/space.cairo index 61f703c2..8fbbbc5b 100644 --- a/starknet/src/space/space.cairo +++ b/starknet/src/space/space.cairo @@ -388,9 +388,9 @@ mod Space { metadata_URI: Array ) { assert_only_authenticator(@self); - assert(voter.is_non_zero(), 'Zero Address'); // TODO: test this branch + assert(voter.is_non_zero(), 'Zero Address'); let proposal = self._proposals.read(proposal_id); - assert_proposal_exists(@proposal); // TODO: test this branch + assert_proposal_exists(@proposal); let timestamp = info::get_block_timestamp().try_into().unwrap(); @@ -740,7 +740,7 @@ mod Space { .voting_strategies_metadata_URIs_to_add .len(), 'len mismatch' - ); // TODO :test this branch + ); _add_voting_strategies(ref self, input.voting_strategies_to_add.span()); self .emit( diff --git a/starknet/src/tests/test_update_settings.cairo b/starknet/src/tests/test_update_settings.cairo index ab3c581e..72be413b 100644 --- a/starknet/src/tests/test_update_settings.cairo +++ b/starknet/src/tests/test_update_settings.cairo @@ -237,6 +237,28 @@ mod tests { // TODO: check event once it's been added } + #[test] + #[available_gas(10000000000)] + #[should_panic(expected: ('len mismatch', 'ENTRYPOINT_FAILED'))] + fn add_voting_strategies_mismatch() { + let (config, space) = setup_update_settings(); + let mut input = UpdateSettingsCalldataImpl::default(); + + let vs1 = StrategyImpl::from_address(contract_address_const::<'votingStrategy1'>()); + let vs2 = StrategyImpl::from_address(contract_address_const::<'votingStrategy2'>()); + + let mut arr = array![vs1.clone(), vs2.clone()]; + input.voting_strategies_to_add = arr; + input.voting_strategies_metadata_URIs_to_add = array![array![]]; // missing one uri! + + space.update_settings(input); + + assert(space.voting_strategies(1) == vs1, 'Voting strategy 1 not added'); + assert(space.voting_strategies(2) == vs2, 'Voting strategy 2 not added'); + assert(space.active_voting_strategies() == 0b111, 'Voting strategies not active'); + // TODO: check event once it's been added + } + #[test] #[available_gas(10000000000)] diff --git a/starknet/src/tests/vote.cairo b/starknet/src/tests/vote.cairo index 4ae9cf08..2607e91d 100644 --- a/starknet/src/tests/vote.cairo +++ b/starknet/src/tests/vote.cairo @@ -377,4 +377,38 @@ mod tests { authenticator.authenticate(space.contract_address, VOTE_SELECTOR, vote_calldata); } + + #[test] + #[available_gas(10000000000)] + #[should_panic( + expected: ('Unknown enum indicator:', 'ENTRYPOINT_FAILED') + )] // TODO: replace once `default` works on Proposal + fn vote_inexistant_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); + + // Increasing block timestamp pass voting delay + testing::set_block_timestamp(config.voting_delay.into()); + + let mut vote_calldata = array![]; + let voter = UserAddress::Starknet(contract_address_const::<0x8765>()); + voter.serialize(ref vote_calldata); + let proposal_id = 42_u256; // inexistent proposal + 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); + } } From af0347bc0deeb7d625e13473aad4028c39ac7c3e Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Mon, 4 Sep 2023 14:12:15 +0200 Subject: [PATCH 20/23] remove string comments --- starknet/src/types/update_settings_calldata.cairo | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/starknet/src/types/update_settings_calldata.cairo b/starknet/src/types/update_settings_calldata.cairo index 96f1ae8f..acd4be93 100644 --- a/starknet/src/types/update_settings_calldata.cairo +++ b/starknet/src/types/update_settings_calldata.cairo @@ -92,7 +92,6 @@ impl NoUpdateString of NoUpdateTrait> { } } -// TODO: find a way for "Strings" impl NoUpdateArray of NoUpdateTrait> { fn no_update() -> Array { array![] @@ -112,7 +111,7 @@ impl UpdateSettingsCalldataImpl of UpdateSettingsCalldataTrait { metadata_URI: NoUpdateString::no_update(), dao_URI: NoUpdateString::no_update(), proposal_validation_strategy: NoUpdateStrategy::no_update(), - proposal_validation_strategy_metadata_URI: NoUpdateString::no_update(), // TODO: string + proposal_validation_strategy_metadata_URI: NoUpdateString::no_update(), authenticators_to_add: NoUpdateArray::no_update(), authenticators_to_remove: NoUpdateArray::no_update(), voting_strategies_to_add: NoUpdateArray::no_update(), From 128c77992a6700d655f14b1200187be9cbb1405c Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Tue, 5 Sep 2023 17:19:26 +0200 Subject: [PATCH 21/23] change metadataURI argument order in l1-execution.test.ts --- starknet/test/l1-execution.test.ts | 18 +++++++++--------- starknet/test/stark-sig.test.ts | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/starknet/test/l1-execution.test.ts b/starknet/test/l1-execution.test.ts index 5f655f49..98f53ad2 100644 --- a/starknet/test/l1-execution.test.ts +++ b/starknet/test/l1-execution.test.ts @@ -140,12 +140,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -243,12 +243,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -341,12 +341,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -441,12 +441,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -538,12 +538,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -643,12 +643,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -740,12 +740,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -837,12 +837,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); @@ -922,12 +922,12 @@ describe('L1 Avatar Execution', function () { CallData.compile({ space: space.address, author: account.address, + metadataURI: [], executionStrategy: { address: ethRelayer.address, params: executionPayload, }, userProposalValidationParams: [], - metadataURI: [], }), { rawInput: true }, ); diff --git a/starknet/test/stark-sig.test.ts b/starknet/test/stark-sig.test.ts index 0e7a22aa..533632f4 100644 --- a/starknet/test/stark-sig.test.ts +++ b/starknet/test/stark-sig.test.ts @@ -105,7 +105,7 @@ describe('Starknet Signature Authenticator', () => { }, _proposal_validation_strategy_metadata_URI: [], _voting_strategies: [{ address: vanillaVotingStrategyAddress, params: [] }], - _voting_strategies_metadata_URI: [], + _voting_strategies_metadata_URI: [[]], _authenticators: [starkSigAuthAddress], _metadata_URI: [], _dao_URI: [], From 105fb6f6e7389fc8d243cb0fcdf6b13df74f4a60 Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Tue, 5 Sep 2023 17:32:55 +0200 Subject: [PATCH 22/23] fix len mismatch error in l1 test --- starknet/test/l1-execution.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/starknet/test/l1-execution.test.ts b/starknet/test/l1-execution.test.ts index 98f53ad2..868386df 100644 --- a/starknet/test/l1-execution.test.ts +++ b/starknet/test/l1-execution.test.ts @@ -77,7 +77,7 @@ describe('L1 Avatar Execution', function () { }, _proposal_validation_strategy_metadata_URI: [], _voting_strategies: [{ address: vanillaVotingStrategy.address, params: [] }], - _voting_strategies_metadata_URI: [], + _voting_strategies_metadata_URI: [[]], _authenticators: [starkTxAuthenticator.address], _metadata_URI: [], _dao_URI: [], From bdad1a629050e160a7db11023cedb4aa100322db Mon Sep 17 00:00:00 2001 From: Scott Piriou <30843220+pscott@users.noreply.github.com> Date: Tue, 5 Sep 2023 17:56:06 +0200 Subject: [PATCH 23/23] modify stark / eth authenticators to take metadata_uri in second position --- starknet/src/authenticators/eth_sig.cairo | 4 ++-- starknet/src/authenticators/eth_tx.cairo | 4 ++-- starknet/src/authenticators/stark_sig.cairo | 4 ++-- starknet/src/authenticators/stark_tx.cairo | 4 ++-- starknet/src/tests/test_stark_tx_auth.cairo | 10 +++++----- starknet/test/stark-sig.test.ts | 2 +- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/starknet/src/authenticators/eth_sig.cairo b/starknet/src/authenticators/eth_sig.cairo index faa22937..ee22916d 100644 --- a/starknet/src/authenticators/eth_sig.cairo +++ b/starknet/src/authenticators/eth_sig.cairo @@ -10,9 +10,9 @@ trait IEthSigAuthenticator { v: u256, target: ContractAddress, author: EthAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array, salt: u256, ); fn authenticate_vote( @@ -67,9 +67,9 @@ mod EthSigAuthenticator { v: u256, target: ContractAddress, author: EthAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array, salt: u256, ) { signatures::verify_propose_sig( diff --git a/starknet/src/authenticators/eth_tx.cairo b/starknet/src/authenticators/eth_tx.cairo index 17e45b75..f91dc904 100644 --- a/starknet/src/authenticators/eth_tx.cairo +++ b/starknet/src/authenticators/eth_tx.cairo @@ -7,9 +7,9 @@ trait IEthTxAuthenticator { ref self: TContractState, target: ContractAddress, author: EthAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array ); fn authenticate_vote( ref self: TContractState, @@ -53,9 +53,9 @@ mod EthTxAuthenticator { ref self: ContractState, target: ContractAddress, author: EthAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array ) { let mut payload = array![]; target.serialize(ref payload); diff --git a/starknet/src/authenticators/stark_sig.cairo b/starknet/src/authenticators/stark_sig.cairo index 3aceffcc..64dff69b 100644 --- a/starknet/src/authenticators/stark_sig.cairo +++ b/starknet/src/authenticators/stark_sig.cairo @@ -8,9 +8,9 @@ trait IStarkSigAuthenticator { signature: Array, target: ContractAddress, author: ContractAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array, salt: felt252, account_type: felt252 ); @@ -60,9 +60,9 @@ mod StarkSigAuthenticator { signature: Array, target: ContractAddress, author: ContractAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array, salt: felt252, account_type: felt252 ) { diff --git a/starknet/src/authenticators/stark_tx.cairo b/starknet/src/authenticators/stark_tx.cairo index 0cd4e5c5..74751df2 100644 --- a/starknet/src/authenticators/stark_tx.cairo +++ b/starknet/src/authenticators/stark_tx.cairo @@ -7,9 +7,9 @@ trait IStarkTxAuthenticator { ref self: TContractState, space: ContractAddress, author: ContractAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array ); fn authenticate_vote( ref self: TContractState, @@ -48,9 +48,9 @@ mod StarkTxAuthenticator { ref self: ContractState, space: ContractAddress, author: ContractAddress, + metadata_uri: Array, execution_strategy: Strategy, user_proposal_validation_params: Array, - metadata_uri: Array ) { assert(info::get_caller_address() == author, 'Invalid Caller'); diff --git a/starknet/src/tests/test_stark_tx_auth.cairo b/starknet/src/tests/test_stark_tx_auth.cairo index 30c18e46..592a4c80 100644 --- a/starknet/src/tests/test_stark_tx_auth.cairo +++ b/starknet/src/tests/test_stark_tx_auth.cairo @@ -72,7 +72,7 @@ mod tests { testing::set_contract_address(author); authenticator .authenticate_propose( - space.contract_address, author, vanilla_execution_strategy, array![], array![] + space.contract_address, author, array![], vanilla_execution_strategy, array![] ); assert(space.next_proposal_id() == 2_u256, 'next_proposal_id should be 2'); @@ -89,7 +89,7 @@ mod tests { testing::set_contract_address(author); authenticator .authenticate_update_proposal( - space.contract_address, author, proposal_id, new_execution_strategy, array![] + space.contract_address, author, proposal_id, new_execution_strategy, array![], ); // Increasing block timestamp by 1 to pass voting delay @@ -140,7 +140,7 @@ mod tests { testing::set_contract_address(config.owner); authenticator .authenticate_propose( - space.contract_address, author, vanilla_execution_strategy, array![], array![] + space.contract_address, author, array![], vanilla_execution_strategy, array![], ); } @@ -172,7 +172,7 @@ mod tests { testing::set_contract_address(author); authenticator .authenticate_propose( - space.contract_address, author, vanilla_execution_strategy, array![], array![] + space.contract_address, author, array![], vanilla_execution_strategy, array![], ); assert(space.next_proposal_id() == 2_u256, 'next_proposal_id should be 2'); @@ -222,7 +222,7 @@ mod tests { testing::set_contract_address(author); authenticator .authenticate_propose( - space.contract_address, author, vanilla_execution_strategy, array![], array![] + space.contract_address, author, array![], vanilla_execution_strategy, array![], ); assert(space.next_proposal_id() == 2_u256, 'next_proposal_id should be 2'); diff --git a/starknet/test/stark-sig.test.ts b/starknet/test/stark-sig.test.ts index 533632f4..483b0320 100644 --- a/starknet/test/stark-sig.test.ts +++ b/starknet/test/stark-sig.test.ts @@ -125,12 +125,12 @@ describe('Starknet Signature Authenticator', () => { const proposeMsg: Propose = { space: spaceAddress, author: address0, + metadataURI: ['0x1', '0x2', '0x3', '0x4'], executionStrategy: { address: '0x0000000000000000000000000000000000001234', params: ['0x5', '0x6', '0x7', '0x8'], }, userProposalValidationParams: ['0x1', '0x2', '0x3', '0x4'], - metadataURI: ['0x1', '0x2', '0x3', '0x4'], salt: '0x0', };