From 79f0eb226c7ad029406f5b92bcd3bd5a1dbd8ca6 Mon Sep 17 00:00:00 2001 From: Moody Salem Date: Wed, 12 Jun 2024 15:27:02 -0400 Subject: [PATCH 1/4] make the governor into an account contract to simplify proposal simulation --- src/governor.cairo | 137 ++++++++++++++++++++++++++------------------- 1 file changed, 78 insertions(+), 59 deletions(-) diff --git a/src/governor.cairo b/src/governor.cairo index c855015..bfb5db6 100644 --- a/src/governor.cairo +++ b/src/governor.cairo @@ -57,9 +57,6 @@ pub trait IGovernor { // This method can be called by anyone at any time before the proposal is executed. fn report_breach(ref self: TContractState, id: felt252, breach_timestamp: u64); - // Execute the given proposal. - fn execute(ref self: TContractState, id: felt252, calls: Span) -> Span>; - // Attaches the given text to the proposal. Simply emits an event containing the proposal description. fn describe(ref self: TContractState, id: felt252, description: ByteArray); @@ -108,14 +105,14 @@ pub mod Governor { use governance::staker::{IStakerDispatcherTrait}; use starknet::{ get_block_timestamp, get_caller_address, contract_address_const, get_contract_address, - syscalls::{replace_class_syscall} + syscalls::{replace_class_syscall}, account::{AccountContract}, get_tx_info, VALIDATED }; + use super::{ IStakerDispatcher, ContractAddress, Array, IGovernor, Config, ProposalInfo, Call, ExecutionState, ByteArray, ClassHash }; - #[derive(starknet::Event, Drop)] pub struct Proposed { pub id: felt252, @@ -402,60 +399,6 @@ pub mod Governor { self.emit(CreationThresholdBreached { id, breach_timestamp }); } - fn execute( - ref self: ContractState, id: felt252, mut calls: Span - ) -> Span> { - let calls_hash = hash_calls(@calls); - - let (mut proposal, config) = self.get_proposal_with_config(id); - - assert(proposal.calls_hash == calls_hash, 'CALLS_HASH_MISMATCH'); - assert(proposal.proposer.is_non_zero(), 'DOES_NOT_EXIST'); - assert(proposal.execution_state.executed.is_zero(), 'ALREADY_EXECUTED'); - assert(proposal.execution_state.canceled.is_zero(), 'PROPOSAL_CANCELED'); - - let timestamp_current = get_block_timestamp(); - // we cannot tell if a proposal is executed if it is executed at timestamp 0 - // this can only happen in testing, but it makes this method locally correct - assert(timestamp_current.is_non_zero(), 'TIMESTAMP_ZERO'); - - let voting_end = proposal.execution_state.created - + config.voting_start_delay - + config.voting_period; - assert(timestamp_current >= voting_end, 'VOTING_NOT_ENDED'); - - let window_start = voting_end + config.execution_delay; - assert(timestamp_current >= window_start, 'EXECUTION_WINDOW_NOT_STARTED'); - - let window_end = window_start + config.execution_window; - assert(timestamp_current < window_end, 'EXECUTION_WINDOW_OVER'); - - assert(proposal.yea >= config.quorum, 'QUORUM_NOT_MET'); - assert(proposal.yea >= proposal.nay, 'NO_MAJORITY'); - - proposal - .execution_state = - ExecutionState { - created: proposal.execution_state.created, - executed: timestamp_current, - canceled: Zero::zero() - }; - - self.proposals.write(id, proposal); - - let mut results: Array> = array![]; - - while let Option::Some(call) = calls.pop_front() { - results.append(call.execute()); - }; - - let result_span = results.span(); - - self.emit(Executed { id, result_data: result_span }); - - result_span - } - fn get_config(self: @ContractState) -> Config { self.get_config_version(self.latest_config_version.read()) } @@ -507,4 +450,80 @@ pub mod Governor { replace_class_syscall(class_hash).unwrap(); } } + + fn get_proposal_id_from_signature() -> felt252 { + let mut tx_info = get_tx_info().unbox(); + *tx_info.signature.pop_front().expect('PROPOSAL_ID_IN_SIGNATURE') + } + + #[abi(embed_v0)] + impl GovernorAccountContract of AccountContract { + fn __validate_declare__(self: @ContractState, class_hash: felt252) -> felt252 { + panic!("Declare not supported"); + 0 + } + + fn __validate__(ref self: ContractState, calls: Array) -> felt252 { + let id = get_proposal_id_from_signature(); + + let mut calls_span = calls.span(); + let calls_hash = hash_calls(@calls_span); + + let (proposal, config) = self.get_proposal_with_config(id); + + assert(proposal.calls_hash == calls_hash, 'CALLS_HASH_MISMATCH'); + assert(proposal.proposer.is_non_zero(), 'DOES_NOT_EXIST'); + assert(proposal.execution_state.executed.is_zero(), 'ALREADY_EXECUTED'); + assert(proposal.execution_state.canceled.is_zero(), 'PROPOSAL_CANCELED'); + + let timestamp_current = get_block_timestamp(); + // we cannot tell if a proposal is executed if it is executed at timestamp 0 + // this can only happen in testing, but it makes this method locally correct + assert(timestamp_current.is_non_zero(), 'TIMESTAMP_ZERO'); + + let voting_end = proposal.execution_state.created + + config.voting_start_delay + + config.voting_period; + assert(timestamp_current >= voting_end, 'VOTING_NOT_ENDED'); + + let window_start = voting_end + config.execution_delay; + assert(timestamp_current >= window_start, 'EXECUTION_WINDOW_NOT_STARTED'); + + let window_end = window_start + config.execution_window; + assert(timestamp_current < window_end, 'EXECUTION_WINDOW_OVER'); + + assert(proposal.yea >= config.quorum, 'QUORUM_NOT_MET'); + assert(proposal.yea >= proposal.nay, 'NO_MAJORITY'); + + VALIDATED + } + + fn __execute__(ref self: ContractState, calls: Array) -> Array> { + let id = get_proposal_id_from_signature(); + let mut calls_span = calls.span(); + + let (mut proposal, _) = self.get_proposal_with_config(id); + + proposal + .execution_state = + ExecutionState { + created: proposal.execution_state.created, + executed: get_block_timestamp(), + canceled: Zero::zero() + }; + + self.proposals.write(id, proposal); + + let mut results: Array> = array![]; + + while let Option::Some(call) = calls_span + .pop_front() { + results.append(call.execute()); + }; + + self.emit(Executed { id, result_data: results.span() }); + + results + } + } } From b1243c289858a5df214e00f18572fb5d1dcf0036 Mon Sep 17 00:00:00 2001 From: Moody Salem Date: Wed, 12 Jun 2024 15:28:04 -0400 Subject: [PATCH 2/4] get just the proposal --- src/governor.cairo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/governor.cairo b/src/governor.cairo index bfb5db6..60504ae 100644 --- a/src/governor.cairo +++ b/src/governor.cairo @@ -502,7 +502,7 @@ pub mod Governor { let id = get_proposal_id_from_signature(); let mut calls_span = calls.span(); - let (mut proposal, _) = self.get_proposal_with_config(id); + let mut proposal = self.get_proposal(id); proposal .execution_state = From a636813bad732c4677b5c2cecc2b5048acee8ed6 Mon Sep 17 00:00:00 2001 From: Moody Salem Date: Wed, 12 Jun 2024 16:03:47 -0400 Subject: [PATCH 3/4] fix unit tests --- src/governor_test.cairo | 92 ++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 56 deletions(-) diff --git a/src/governor_test.cairo b/src/governor_test.cairo index 3a9c618..fbc2501 100644 --- a/src/governor_test.cairo +++ b/src/governor_test.cairo @@ -5,6 +5,7 @@ use core::option::{OptionTrait}; use core::result::{Result, ResultTrait}; use core::serde::Serde; +use core::starknet::account::AccountContractDispatcherTrait; use core::traits::{TryInto}; use governance::execution_state::{ExecutionState}; @@ -19,9 +20,25 @@ use starknet::account::{Call}; use starknet::{ get_contract_address, syscalls::deploy_syscall, ClassHash, contract_address_const, ContractAddress, get_block_timestamp, - testing::{set_block_timestamp, set_contract_address, pop_log} + testing::{set_block_timestamp, set_contract_address, pop_log, set_signature}, + account::{AccountContractDispatcher} }; +#[generate_trait] +impl GovernorExecuteImpl of GovernorExecuteTrait { + fn validate(self: IGovernorDispatcher, id: felt252, calls: Array) -> felt252 { + set_signature(array![id].span()); + AccountContractDispatcher { contract_address: self.contract_address } + .__validate__(calls) + } + fn execute(self: IGovernorDispatcher, id: felt252, calls: Array) -> Span> { + set_signature(array![id].span()); + AccountContractDispatcher { contract_address: self.contract_address } + .__execute__(calls) + .span() + } +} + pub(crate) fn recipient() -> ContractAddress { 'recipient'.try_into().unwrap() @@ -381,7 +398,7 @@ fn test_describe_proposal_fails_if_executed() { governor.vote(id, true); advance_time(config.voting_period + config.execution_delay); set_contract_address(anyone()); - governor.execute(id, array![transfer_call(token, anyone(), amount: 0)].span()); + governor.execute(id, array![transfer_call(token, anyone(), amount: 0)]); set_contract_address(proposer()); governor.describe(id, "This proposal is already executed"); @@ -653,10 +670,7 @@ fn test_execute_valid_proposal() { set_contract_address(anyone()); - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.execute(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); let proposal = governor.get_proposal(id); assert(proposal.execution_state.executed.is_non_zero(), 'execute failed'); @@ -673,10 +687,7 @@ fn test_canceled_proposal_cannot_be_executed() { advance_time(config.voting_start_delay); advance_time(config.voting_period); set_contract_address(anyone()); - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } #[test] @@ -688,10 +699,7 @@ fn test_execute_before_voting_ends_should_fail() { advance_time(config.voting_start_delay); // Execute the proposal. The vote is still active, this should fail. - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } @@ -705,10 +713,7 @@ fn test_execute_quorum_not_met_should_fail() { set_contract_address(proposer()); // Execute the proposal. The quorum was not met, this should fail. - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } #[test] @@ -747,10 +752,7 @@ fn test_execute_no_majority_should_fail() { advance_time(config.voting_period + config.execution_delay); // Execute the proposal. The majority of votes are no, this should fail. - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } #[test] @@ -771,10 +773,7 @@ fn test_execute_before_execution_window_begins() { governor.vote(id, true); advance_time(config.voting_period); - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } @@ -796,10 +795,7 @@ fn test_execute_after_execution_window_ends() { governor.vote(id, true); advance_time(config.voting_period + config.execution_delay + config.execution_window); - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } #[test] @@ -849,10 +845,7 @@ fn test_verify_votes_are_counted_over_voting_weight_smoothing_duration_from_star advance_time(config.voting_period + config.execution_delay); // Execute the proposal. The quorum was not met, this should fail. - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } #[test] @@ -879,10 +872,7 @@ fn test_quorum_counts_only_yes_votes_not_met() { advance_time(config.voting_period + config.execution_delay); - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); } #[test] @@ -891,8 +881,7 @@ fn test_quorum_counts_only_yes_votes_exactly_met() { let calls = array![ transfer_call(token: token, recipient: recipient(), amount: 150), transfer_call(token: token, recipient: recipient(), amount: 50) - ] - .span(); + ]; // so execution can succeed token.transfer(governor.contract_address, 200); @@ -904,7 +893,7 @@ fn test_quorum_counts_only_yes_votes_exactly_met() { advance_time(config.voting_weight_smoothing_duration); set_contract_address(voter1()); - let id = governor.propose(calls); + let id = governor.propose(calls.span()); advance_time(config.voting_start_delay); governor.vote(id, true); @@ -923,8 +912,7 @@ fn test_execute_emits_logs_from_data() { let calls = array![ transfer_call(token: token, recipient: recipient(), amount: 150), transfer_call(token: token, recipient: recipient(), amount: 50) - ] - .span(); + ]; // so execution can succeed token.transfer(governor.contract_address, 200); @@ -936,7 +924,7 @@ fn test_execute_emits_logs_from_data() { advance_time(config.voting_weight_smoothing_duration); set_contract_address(voter1()); - let id = governor.propose(calls); + let id = governor.propose(calls.span()); advance_time(config.voting_start_delay); governor.vote(id, true); @@ -976,13 +964,10 @@ fn test_execute_already_executed_should_fail() { advance_time(config.voting_period + config.execution_delay); set_contract_address(anyone()); + governor.execute(id, array![transfer_call(token: token, recipient: recipient(), amount: 100)]); governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() - ); - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 100)].span() + .validate( + id, array![transfer_call(token: token, recipient: recipient(), amount: 100)] ); // Try to execute again } @@ -1003,10 +988,7 @@ fn test_execute_invalid_call_id() { advance_time(config.voting_period); set_contract_address(anyone()); - governor - .execute( - id, array![transfer_call(token: token, recipient: recipient(), amount: 101)].span() - ); + governor.validate(id, array![transfer_call(token: token, recipient: recipient(), amount: 101)]); } #[test] @@ -1052,7 +1034,6 @@ fn test_upgrade_succeeds_self_call() { calldata: array![Governor::TEST_CLASS_HASH].span() } ] - .span() ); } @@ -1121,7 +1102,6 @@ fn test_reconfigure_succeeds_self_call() { calldata: args.span() } ] - .span() ); pop_log::(governor.contract_address).unwrap(); From df27c62077423ac39056f72aa8d23358a31a6ac5 Mon Sep 17 00:00:00 2001 From: Moody Salem Date: Wed, 12 Jun 2024 16:04:41 -0400 Subject: [PATCH 4/4] scarb fmt --- src/governor_test.cairo | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/governor_test.cairo b/src/governor_test.cairo index fbc2501..cf28be1 100644 --- a/src/governor_test.cairo +++ b/src/governor_test.cairo @@ -28,8 +28,7 @@ use starknet::{ impl GovernorExecuteImpl of GovernorExecuteTrait { fn validate(self: IGovernorDispatcher, id: felt252, calls: Array) -> felt252 { set_signature(array![id].span()); - AccountContractDispatcher { contract_address: self.contract_address } - .__validate__(calls) + AccountContractDispatcher { contract_address: self.contract_address }.__validate__(calls) } fn execute(self: IGovernorDispatcher, id: felt252, calls: Array) -> Span> { set_signature(array![id].span());