From 6ccf3040446b335b8a814d733ecf4b0140df9959 Mon Sep 17 00:00:00 2001 From: Moody Salem Date: Tue, 23 Apr 2024 14:11:04 -0400 Subject: [PATCH] fix a couple nits with the interfaces and code (#37) * fix a couple nits with the interfaces and code * stop writing 0 for latest proposal id * allow canceling a proposal for the specified breach timestamp --- README.md | 4 +-- src/governor.cairo | 63 +++++++++++++++++++++++++---------------- src/governor_test.cairo | 28 ++++++++++++++++-- src/staker.cairo | 46 +++++++++++++++++++++++------- src/staker_test.cairo | 6 ++-- 5 files changed, 104 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 1c72850..7cdeabc 100644 --- a/README.md +++ b/README.md @@ -26,8 +26,8 @@ Contracts in this repository are designed so that they may be used together _or_ `Staker` enables users to delegate the balance of a token towards an account, and tracks the historical delegation at each block. In addition, it allows the computation of the time-weighted average delegated tokens of any account over any historical period. -- Users call `Token#approve(staker, stake_amount)`, then `Staker#stake(delegate)` to stake and delegate their tokens to other addresses -- Users call `Staker#withdraw(delegate, recipient, amount)` to remove part or all of their delegation +- Users call `Token#approve(staker, stake_amount)`, then `Staker#stake_amount(delegate, stake_amount)` to stake and delegate their tokens to other addresses +- Users call `Staker#withdraw_amount(delegate, recipient, amount)` to remove part or all of their delegation - The average delegation weight is computable over *any* historical period - The contract has no owner, and cannot be updated nor configured diff --git a/src/governor.cairo b/src/governor.cairo index b25c664..915887c 100644 --- a/src/governor.cairo +++ b/src/governor.cairo @@ -43,10 +43,15 @@ pub trait IGovernor { // Vote on the given proposal. fn vote(ref self: TContractState, id: felt252, yea: bool); - // Cancel the given proposal. The proposer may cancel the proposal at any time during before or during the voting period. - // Cancellation can happen by any address if the average voting weight is below the proposal_creation_threshold. + // Cancel the proposal with the given ID. Same as #cancel_at_timestamp, but uses the current timestamp for computing the voting weight. fn cancel(ref self: TContractState, id: felt252); + // Cancel the proposal with the given ID. The proposal may be canceled at any time before it is executed. + // There are two ways the proposal cancellation can be authorized: + // - The proposer can cancel the proposal + // - Anyone can cancel if the average voting weight of the proposer was below the proposal_creation_threshold during the voting period (at the given breach_timestamp) + fn cancel_at_timestamp(ref self: TContractState, id: felt252, breach_timestamp: u64); + // Execute the given proposal. fn execute(ref self: TContractState, call: Call) -> Span; @@ -100,6 +105,7 @@ pub mod Governor { #[derive(starknet::Event, Drop)] pub struct Canceled { pub id: felt252, + pub breach_timestamp: u64, } #[derive(starknet::Event, Drop)] @@ -150,7 +156,8 @@ pub mod Governor { if latest_proposal_id.is_non_zero() { let latest_proposal_state = self.get_proposal(latest_proposal_id).execution_state; - if (latest_proposal_state.canceled.is_zero()) { + // if the proposal is not canceled, check that the voting for that proposal has ended + if latest_proposal_state.canceled.is_zero() { assert( latest_proposal_state.created + config.voting_start_delay @@ -236,54 +243,60 @@ pub mod Governor { self.proposals.write(id, proposal); self.has_voted.write((voter, id), true); - self.emit(Voted { id, voter, weight, yea, }); + self.emit(Voted { id, voter, weight, yea }); } fn cancel(ref self: ContractState, id: felt252) { + self.cancel_at_timestamp(id, get_block_timestamp()) + } + + fn cancel_at_timestamp(ref self: ContractState, id: felt252, breach_timestamp: u64) { let config = self.config.read(); - let voting_token = self.staker.read(); + let staker = self.staker.read(); let mut proposal = self.proposals.read(id); assert(proposal.proposer.is_non_zero(), 'DOES_NOT_EXIST'); - if (proposal.proposer != get_caller_address()) { - // if at any point the average voting weight is below the proposal_creation_threshold for the proposer, it can be canceled + assert(proposal.execution_state.canceled.is_zero(), 'ALREADY_CANCELED'); + assert(proposal.execution_state.executed.is_zero(), 'ALREADY_EXECUTED'); + assert(breach_timestamp >= proposal.execution_state.created, 'PROPOSAL_NOT_CREATED'); + + assert( + breach_timestamp < (proposal.execution_state.created + + config.voting_start_delay + + config.voting_period), + 'VOTING_ENDED' + ); + + // iff the proposer is not calling this we need to check the voting weight + if proposal.proposer != get_caller_address() { + // if at the given timestamp (during the voting period), + // the average voting weight is below the proposal_creation_threshold for the proposer, it can be canceled assert( - voting_token - .get_average_delegated_over_last( + staker + .get_average_delegated( delegate: proposal.proposer, - period: config.voting_weight_smoothing_duration + start: breach_timestamp - config.voting_weight_smoothing_duration, + end: breach_timestamp ) < config .proposal_creation_threshold, 'THRESHOLD_NOT_BREACHED' ); } - let timestamp_current = get_block_timestamp(); - - assert( - timestamp_current < (proposal.execution_state.created - + config.voting_start_delay - + config.voting_period), - 'VOTING_ENDED' - ); - - // we know it's not executed since we check voting has not ended proposal .execution_state = ExecutionState { created: proposal.execution_state.created, + // we asserted that it is not already executed executed: 0, - canceled: timestamp_current + canceled: get_block_timestamp() }; self.proposals.write(id, proposal); - // allows the proposer to create a new proposal - self.latest_proposal_by_proposer.write(proposal.proposer, Zero::zero()); - - self.emit(Canceled { id }); + self.emit(Canceled { id, breach_timestamp }); } fn execute(ref self: ContractState, call: Call) -> Span { diff --git a/src/governor_test.cairo b/src/governor_test.cairo index 540fcff..2d1d916 100644 --- a/src/governor_test.cairo +++ b/src/governor_test.cairo @@ -424,6 +424,14 @@ fn test_vote_after_voting_period_should_fail() { governor.vote(id, true); // vote should fail } +#[test] +#[should_panic(expected: ('DOES_NOT_EXIST', 'ENTRYPOINT_FAILED'))] +fn test_cancel_fails_if_proposal_not_exists() { + let (_staker, _token, governor, _config) = setup(); + let id = 1234; + governor.cancel(id); +} + #[test] fn test_cancel_by_proposer() { let (staker, token, governor, config) = setup(); @@ -453,12 +461,28 @@ fn test_cancel_by_proposer() { ); } +#[test] +#[should_panic(expected: ('ALREADY_CANCELED', 'ENTRYPOINT_FAILED'))] +fn test_double_cancel_by_proposer() { + let (staker, token, governor, _config) = setup(); + + let proposer = proposer(); + + let id = create_proposal(governor, token, staker); + + advance_time(30); + + set_contract_address(proposer); + governor.cancel(id); + governor.cancel(id); +} + #[test] fn test_cancel_by_non_proposer() { let (staker, token, governor, config) = setup(); let id = create_proposal(governor, token, staker); - staker.withdraw(proposer(), recipient: Zero::zero(), amount: 25); + staker.withdraw_amount(proposer(), recipient: Zero::zero(), amount: 25); // Fast forward one smoothing duration advance_time(config.voting_weight_smoothing_duration); @@ -630,7 +654,7 @@ fn test_verify_votes_are_counted_over_voting_weight_smoothing_duration_from_star advance_time(config.voting_start_delay - (config.voting_weight_smoothing_duration / 3)); // undelegate 1/3rd of a duration before voting starts, so only a third of voting power is counted for voter1 set_contract_address(voter1); - staker.withdraw(voter1, recipient: Zero::zero(), amount: 49); + staker.withdraw_amount(voter1, recipient: Zero::zero(), amount: 49); advance_time((config.voting_weight_smoothing_duration / 3)); diff --git a/src/staker.cairo b/src/staker.cairo index 749576d..10e22af 100644 --- a/src/staker.cairo +++ b/src/staker.cairo @@ -13,8 +13,14 @@ pub trait IStaker { // Transfer the approved amount of token from the caller into this contract and delegates it to the given address fn stake(ref self: TContractState, delegate: ContractAddress); - // Withdraws the delegated tokens from the contract to the given recipient address - fn withdraw( + // Transfer the specified amount of token from the caller into this contract and delegates the voting weight to the specified delegate + fn stake_amount(ref self: TContractState, delegate: ContractAddress, amount: u128); + + // Unstakes and withdraws all of the tokens delegated by the sender to the delegate from the contract to the given recipient address + fn withdraw(ref self: TContractState, delegate: ContractAddress, recipient: ContractAddress); + + // Unstakes and withdraws the specified amount of tokens delegated by the sender to the delegate from the contract to the given recipient address + fn withdraw_amount( ref self: TContractState, delegate: ContractAddress, recipient: ContractAddress, @@ -214,27 +220,45 @@ pub mod Staker { } fn stake(ref self: ContractState, delegate: ContractAddress) { + self + .stake_amount( + delegate, + self + .token + .read() + .allowance(get_caller_address(), get_contract_address()) + .try_into() + .expect('ALLOWANCE_OVERFLOW') + ); + } + + fn stake_amount(ref self: ContractState, delegate: ContractAddress, amount: u128) { let from = get_caller_address(); - let this_address = get_contract_address(); let token = self.token.read(); - let amount = token.allowance(from, this_address); - let amount_small: u128 = amount.try_into().expect('ALLOWANCE_OVERFLOW'); assert( - token.transferFrom(from, get_contract_address(), amount), 'TRANSFER_FROM_FAILED' + token.transferFrom(from, get_contract_address(), amount.into()), + 'TRANSFER_FROM_FAILED' ); let key = (from, delegate); - self.staked.write((from, delegate), amount_small + self.staked.read(key)); + self.staked.write((from, delegate), amount + self.staked.read(key)); self .amount_delegated - .write( - delegate, self.insert_snapshot(delegate, get_block_timestamp()) + amount_small - ); - self.emit(Staked { from, delegate, amount: amount_small }); + .write(delegate, self.insert_snapshot(delegate, get_block_timestamp()) + amount); + self.emit(Staked { from, delegate, amount }); } fn withdraw( + ref self: ContractState, delegate: ContractAddress, recipient: ContractAddress + ) { + self + .withdraw_amount( + delegate, recipient, self.staked.read((get_caller_address(), delegate)) + ) + } + + fn withdraw_amount( ref self: ContractState, delegate: ContractAddress, recipient: ContractAddress, diff --git a/src/staker_test.cairo b/src/staker_test.cairo index de01a7e..cba2c99 100644 --- a/src/staker_test.cairo +++ b/src/staker_test.cairo @@ -305,7 +305,7 @@ fn test_get_average_delegated() { // rewind to undelegate at 8 set_block_timestamp(8); - staker.withdraw(delegatee, recipient: contract_address_const::<0>(), amount: 12345); + staker.withdraw_amount(delegatee, recipient: contract_address_const::<0>(), amount: 12345); set_block_timestamp(12); assert(staker.get_average_delegated(delegatee, 4, 10) == 8230, 'average (4 sec * 12345)/6'); @@ -320,7 +320,7 @@ fn test_transfer_delegates_moved() { set_block_timestamp(2); token.approve(staker.contract_address, 12345); staker.stake(delegatee); - staker.withdraw(delegatee, contract_address_const::<3456>(), 500); + staker.withdraw_amount(delegatee, contract_address_const::<3456>(), 500); set_block_timestamp(5); assert_eq!(staker.get_delegated(delegatee), (12345 - 500)); @@ -338,7 +338,7 @@ fn test_delegate_undelegate() { staker.stake(delegatee); set_block_timestamp(5); - staker.withdraw(delegatee, Zero::zero(), 12345); + staker.withdraw_amount(delegatee, Zero::zero(), 12345); set_block_timestamp(8); assert(staker.get_delegated(delegatee) == 0, 'delegated');