improve how we handle canceled proposals

src/e2e_test.cairo

@@ -146,7 +146,7 @@ fn test_create_proposal_that_fails() {
 
     assert_eq!(proposal_info.proposer, delegate_yes);
     assert_eq!(
-        proposal_info.timestamps, ProposalTimestamps { created: start_time + 30, executed: 0 }
+        proposal_info.timestamps, ProposalTimestamps { created: start_time + 30, executed: 0, canceled: 0 }
     );
     assert_eq!(proposal_info.yea, 100);
     assert_eq!(proposal_info.nay, 101);

src/governor.cairo

@@ -4,6 +4,7 @@ use core::option::{Option, OptionTrait};
 use core::traits::{Into, TryInto};
 use governance::staker::{IStakerDispatcher};
 use starknet::account::{Call};
+use governance::utils::timestamps::{ThreeU64TupleStorePacking};
 use starknet::{ContractAddress, storage_access::{StorePacking}};
 
 #[derive(Copy, Drop, Serde, PartialEq, Debug)]
@@ -12,20 +13,20 @@ pub struct ProposalTimestamps {
     pub created: u64,
     // the timestamp when the proposal was executed
     pub executed: u64,
+    // the timestamp when the proposal was canceled
+    pub canceled: u64,
 }
 
 const TWO_POW_64: u128 = 0x10000000000000000_u128;
 
-impl ProposalTimestampsStorePacking of StorePacking<ProposalTimestamps, u128> {
-    fn pack(value: ProposalTimestamps) -> u128 {
-        value.created.into() + (value.executed.into() * TWO_POW_64)
+impl ProposalTimestampsStorePacking of StorePacking<ProposalTimestamps, felt252> {
+    fn pack(value: ProposalTimestamps) -> felt252 {
+        ThreeU64TupleStorePacking::pack((value.created, value.executed, value.canceled))
     }
 
-    fn unpack(value: u128) -> ProposalTimestamps {
-        let (executed, created) = u128_safe_divmod(value, TWO_POW_64.try_into().unwrap());
-        ProposalTimestamps {
-            created: created.try_into().unwrap(), executed: executed.try_into().unwrap()
-        }
+    fn unpack(value: felt252) -> ProposalTimestamps {
+        let (created, executed, canceled) = ThreeU64TupleStorePacking::unpack(value);
+        ProposalTimestamps { created, executed, canceled }
     }
 }
 
@@ -63,7 +64,8 @@ pub trait IGovernor<TContractState> {
     // Vote on the given proposal.
     fn vote(ref self: TContractState, id: felt252, yea: bool);
 
-    // Cancel the given proposal. Cancellation can happen by any address if the average voting weight is below the proposal_creation_threshold.
+    // 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.
     fn cancel(ref self: TContractState, id: felt252);
 
     // Execute the given proposal.
@@ -131,7 +133,7 @@ pub mod Governor {
         staker: IStakerDispatcher,
         config: Config,
         proposals: LegacyMap<felt252, ProposalInfo>,
-        voted: LegacyMap<(ContractAddress, felt252), bool>,
+        has_voted: LegacyMap<(ContractAddress, felt252), bool>,
         latest_proposal_by_proposer: LegacyMap<ContractAddress, felt252>,
     }
 
@@ -184,7 +186,11 @@ pub mod Governor {
                     id,
                     ProposalInfo {
                         proposer,
-                        timestamps: ProposalTimestamps { created: timestamp_current, executed: 0 },
+                        timestamps: ProposalTimestamps {
+                            created: timestamp_current,
+                            executed: Zero::zero(),
+                            canceled: Zero::zero()
+                        },
                         yea: 0,
                         nay: 0
                     }
@@ -198,18 +204,20 @@ pub mod Governor {
         }
 
         fn vote(ref self: ContractState, id: felt252, yea: bool) {
-            let config = self.config.read();
             let mut proposal = self.proposals.read(id);
 
             assert(proposal.proposer.is_non_zero(), 'DOES_NOT_EXIST');
+            assert(proposal.timestamps.canceled.is_zero(), 'PROPOSAL_CANCELED');
+
+            let config = self.config.read();
             let timestamp_current = get_block_timestamp();
             let voting_start_time = (proposal.timestamps.created + config.voting_start_delay);
             let voter = get_caller_address();
-            let voted = self.voted.read((voter, id));
+            let has_voted = self.has_voted.read((voter, id));
 
             assert(timestamp_current >= voting_start_time, 'VOTING_NOT_STARTED');
             assert(timestamp_current < (voting_start_time + config.voting_period), 'VOTING_ENDED');
-            assert(!voted, 'ALREADY_VOTED');
+            assert(!has_voted, 'ALREADY_VOTED');
 
             let weight = self
                 .staker
@@ -226,7 +234,7 @@ pub mod Governor {
                 proposal.nay = proposal.nay + weight;
             }
             self.proposals.write(id, proposal);
-            self.voted.write((voter, id), true);
+            self.has_voted.write((voter, id), true);
 
             self.emit(Voted { id, voter, weight, yea, });
         }
@@ -235,12 +243,10 @@ pub mod Governor {
         fn cancel(ref self: ContractState, id: felt252) {
             let config = self.config.read();
             let voting_token = self.staker.read();
-            let proposal = self.proposals.read(id);
+            let mut proposal = self.proposals.read(id);
 
             assert(proposal.proposer.is_non_zero(), 'DOES_NOT_EXIST');
 
-            let timestamp_current = get_block_timestamp();
-
             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(
@@ -254,24 +260,25 @@ pub mod Governor {
                 );
             }
 
+            let timestamp_current = get_block_timestamp();
+
             assert(
                 timestamp_current < (proposal.timestamps.created
                     + config.voting_start_delay
                     + config.voting_period),
                 'VOTING_ENDED'
             );
 
-            self
-                .proposals
-                .write(
-                    id,
-                    ProposalInfo {
-                        proposer: contract_address_const::<0>(),
-                        timestamps: ProposalTimestamps { created: 0, executed: 0 },
-                        yea: 0,
-                        nay: 0
-                    }
-                );
+            // we know it's not executed since we check voting has not ended
+            proposal
+                .timestamps =
+                    ProposalTimestamps {
+                        created: proposal.timestamps.created,
+                        executed: 0,
+                        canceled: timestamp_current
+                    };
+
+            self.proposals.write(id, proposal);
 
             // allows the proposer to create a new proposal
             self.latest_proposal_by_proposer.write(proposal.proposer, Zero::zero());
@@ -306,7 +313,9 @@ pub mod Governor {
             proposal
                 .timestamps =
                     ProposalTimestamps {
-                        created: proposal.timestamps.created, executed: timestamp_current
+                        created: proposal.timestamps.created,
+                        executed: timestamp_current,
+                        canceled: Zero::zero()
                     };
 
             self.proposals.write(id, proposal);

src/governor_test.cairo

@@ -141,7 +141,7 @@ fn test_propose() {
         ProposalInfo {
             proposer: proposer(),
             timestamps: ProposalTimestamps {
-                created: config.voting_weight_smoothing_duration, executed: 0
+                created: config.voting_weight_smoothing_duration, executed: 0, canceled: 0
             },
             yea: 0,
             nay: 0
@@ -164,30 +164,18 @@ fn test_propose_has_active_proposal() {
 }
 
 #[test]
-fn test_proposer_can_cancel_and_re_propose() {
+#[should_panic(expected: ('ALREADY_PROPOSED', 'ENTRYPOINT_FAILED'))]
+fn test_proposer_cannot_cancel_and_re_propose() {
     let (staker, token, governor, config) = setup();
 
     token.approve(staker.contract_address, config.proposal_creation_threshold.into());
     staker.stake(proposer());
     advance_time(config.voting_weight_smoothing_duration);
 
     set_contract_address(proposer());
-    let id_1 = governor.propose(transfer_call(token, recipient(), amount: 100));
-    governor.cancel(id_1);
-    let id_2 = governor.propose(transfer_call(token, recipient(), amount: 100));
-    assert_eq!(id_1, id_2);
-
-    assert_eq!(
-        governor.get_proposal(id_2),
-        ProposalInfo {
-            proposer: proposer(),
-            timestamps: ProposalTimestamps {
-                created: config.voting_weight_smoothing_duration, executed: 0
-            },
-            yea: 0,
-            nay: 0
-        }
-    );
+    let id = governor.propose(transfer_call(token, recipient(), amount: 100));
+    governor.cancel(id);
+    governor.propose(transfer_call(token, recipient(), amount: 100));
 }
 
 #[test]
@@ -232,6 +220,21 @@ fn test_vote_yes() {
     assert_eq!(proposal.nay, 0);
 }
 
+#[test]
+fn test_anyone_can_vote() {
+    let (staker, token, governor, config) = setup();
+    let id = create_proposal(governor, token, staker);
+
+    advance_time(config.voting_start_delay);
+
+    set_contract_address(anyone());
+    governor.vote(id, true);
+
+    let proposal = governor.get_proposal(id);
+    assert_eq!(proposal.yea, 0);
+    assert_eq!(proposal.nay, 0);
+}
+
 #[test]
 fn test_vote_no_staking_after_period_starts() {
     let (staker, token, governor, config) = setup();
@@ -251,6 +254,27 @@ fn test_vote_no_staking_after_period_starts() {
     assert_eq!(proposal.yea, 0);
 }
 
+
+#[test]
+#[should_panic(expected: ('DOES_NOT_EXIST', 'ENTRYPOINT_FAILED'))]
+fn test_vote_invalid_proposal() {
+    let (_, _, governor, _) = setup();
+
+    governor.vote(123, true);
+}
+
+#[test]
+#[should_panic(expected: ('PROPOSAL_CANCELED', 'ENTRYPOINT_FAILED'))]
+fn test_vote_after_cancel_proposal() {
+    let (staker, token, governor, config) = setup();
+
+    let id = create_proposal(governor, token, staker);
+    set_contract_address(proposer());
+    governor.cancel(id);
+    set_contract_address(voter1());
+    governor.vote(id, true);
+}
+
 #[test]
 #[should_panic(expected: ('VOTING_NOT_STARTED', 'ENTRYPOINT_FAILED'))]
 fn test_vote_before_voting_start_should_fail() {
@@ -310,25 +334,30 @@ fn test_vote_after_voting_period_should_fail() {
 
 #[test]
 fn test_cancel_by_proposer() {
-    let (staker, token, governor, _config) = setup();
+    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); // Cancel the proposal
+    governor.cancel(id);
 
-    // Expect that proposal is no longer available
     let proposal = governor.get_proposal(id);
-    assert(
-        proposal == ProposalInfo {
-            proposer: contract_address_const::<0>(),
-            timestamps: ProposalTimestamps { created: 0, executed: 0 },
+    assert_eq!(
+        proposal,
+        ProposalInfo {
+            proposer: proposer(),
+            timestamps: ProposalTimestamps {
+                created: config.voting_weight_smoothing_duration,
+                executed: 0,
+                canceled: config.voting_weight_smoothing_duration + 30
+            },
             yea: 0,
             nay: 0,
-        },
-        'proposal not cancelled'
+        }
     );
 }
 
@@ -351,8 +380,12 @@ fn test_cancel_by_non_proposer() {
     assert_eq!(
         proposal,
         ProposalInfo {
-            proposer: contract_address_const::<0>(),
-            timestamps: ProposalTimestamps { created: 0, executed: 0 },
+            proposer: proposer(),
+            timestamps: ProposalTimestamps {
+                created: config.voting_weight_smoothing_duration,
+                executed: 0,
+                canceled: config.voting_weight_smoothing_duration * 2
+            },
             yea: 0,
             nay: 0,
         }