Bug milestone round (#215)

* add failing test

* setup

* write migration

* test

* fix name confusion

* review and test

* fmt

pallets/proposals/src/impls.rs

@@ -33,16 +33,19 @@ impl<T: Config> Pallet<T> {
 
         let expiry_block =
             <T as Config>::MilestoneVotingWindow::get() + frame_system::Pallet::<T>::block_number();
-        Rounds::<T>::insert(project_key, RoundType::VotingRound, expiry_block);
+        Rounds::<T>::insert(
+            (project_key, milestone_key),
+            RoundType::VotingRound,
+            expiry_block,
+        );
         RoundsExpiring::<T>::try_mutate(expiry_block, |keys| {
             keys.try_push((project_key, RoundType::VotingRound, milestone_key))
                 .map_err(|_| Error::<T>::Overflow)?;
             Ok::<(), DispatchError>(())
         })?;
         UserHasVoted::<T>::remove((project_key, RoundType::VotingRound, milestone_key));
 
-        let vote = Vote::default();
-        <MilestoneVotes<T>>::insert(project_key, milestone_key, vote);
+        <MilestoneVotes<T>>::insert(project_key, milestone_key, Vote::default());
         Self::deposit_event(Event::MilestoneSubmitted(who, project_key, milestone_key));
         Self::deposit_event(Event::VotingRoundCreated(project_key));
         Ok(().into())
@@ -56,7 +59,7 @@ impl<T: Config> Pallet<T> {
     ) -> DispatchResultWithPostInfo {
         let project = Projects::<T>::get(project_key).ok_or(Error::<T>::ProjectDoesNotExist)?;
         ensure!(
-            Rounds::<T>::contains_key(project_key, RoundType::VotingRound),
+            Rounds::<T>::contains_key((project_key, milestone_key), RoundType::VotingRound),
             Error::<T>::VotingRoundNotStarted
         );
         let contribution_amount = project
@@ -206,7 +209,11 @@ impl<T: Config> Pallet<T> {
         let expiry_block = frame_system::Pallet::<T>::block_number()
             .saturating_add(<T as Config>::NoConfidenceTimeLimit::get());
 
-        Rounds::<T>::insert(project_key, RoundType::VoteOfNoConfidence, expiry_block);
+        Rounds::<T>::insert(
+            (project_key, 0),
+            RoundType::VoteOfNoConfidence,
+            expiry_block,
+        );
         RoundsExpiring::<T>::try_mutate(expiry_block, |keys| {
             // The milestone key does not matter here as we are voting on the entire project.
             keys.try_push((project_key, RoundType::VoteOfNoConfidence, 0))
@@ -233,7 +240,7 @@ impl<T: Config> Pallet<T> {
         is_yay: bool,
     ) -> DispatchResult {
         ensure!(
-            Rounds::<T>::contains_key(project_key, RoundType::VoteOfNoConfidence),
+            Rounds::<T>::contains_key((project_key, 0), RoundType::VoteOfNoConfidence),
             Error::<T>::ProjectNotInRound
         );
         let project = Self::projects(project_key).ok_or(Error::<T>::ProjectDoesNotExist)?;
@@ -320,7 +327,7 @@ impl<T: Config> Pallet<T> {
                 }
             }
             Projects::<T>::remove(project_key);
-            Rounds::<T>::remove(project_key, RoundType::VoteOfNoConfidence);
+            Rounds::<T>::remove((project_key, 0), RoundType::VoteOfNoConfidence);
             <T as Config>::DepositHandler::return_deposit(project.deposit_id)?;
             Self::deposit_event(Event::NoConfidenceRoundFinalised(who, project_key));
         }
@@ -335,7 +342,7 @@ impl<T: Config> Pallet<T> {
     ) -> DispatchResultWithPostInfo {
         let project = Projects::<T>::get(project_key).ok_or(Error::<T>::ProjectDoesNotExist)?;
         ensure!(
-            Rounds::<T>::contains_key(project_key, RoundType::VoteOfNoConfidence),
+            Rounds::<T>::contains_key((project_key, 0), RoundType::VoteOfNoConfidence),
             Error::<T>::ProjectNotInRound
         );
         ensure!(
@@ -444,8 +451,9 @@ impl<T: Config> Pallet<T> {
         user_has_voted_key: (ProjectKey, RoundType, MilestoneKey),
     ) -> Result<(), DispatchError> {
         // Prevent further voting.
-        let exp_block = Rounds::<T>::take(project_key, RoundType::VotingRound)
-            .ok_or(Error::<T>::VotingRoundNotStarted)?;
+        let exp_block =
+            Rounds::<T>::take((project_key, user_has_voted_key.2), RoundType::VotingRound)
+                .ok_or(Error::<T>::VotingRoundNotStarted)?;
         // Prevent hook from calling.
         RoundsExpiring::<T>::remove(exp_block);
         // Allow future votes to occur on this milestone

pallets/proposals/src/lib.rs

@@ -154,7 +154,7 @@ pub mod pallet {
     pub type Rounds<T> = StorageDoubleMap<
         _,
         Blake2_128Concat,
-        ProjectKey,
+        (ProjectKey, MilestoneKey),
         Blake2_128Concat,
         RoundType,
         BlockNumberFor<T>,
@@ -173,7 +173,7 @@ pub mod pallet {
 
     #[pallet::storage]
     #[pallet::getter(fn storage_version)]
-    pub(super) type StorageVersion<T: Config> = StorageValue<_, Release, ValueQuery>;
+    pub(super) type ProjectStorageVersion<T: Config> = StorageValue<_, Release, ValueQuery>;
 
     #[pallet::event]
     #[pallet::generate_deposit(pub(super) fn deposit_event)]
@@ -272,13 +272,7 @@ pub mod pallet {
         ///
         /// >
         fn on_runtime_upgrade() -> Weight {
-            let mut weight = T::DbWeight::get().reads_writes(1, 1);
-            // Only supporting latest upgrade for now.
-            if StorageVersion::<T>::get() == Release::V2 {
-                weight += migration::v3::migrate_all::<T>();
-                StorageVersion::<T>::set(Release::V3);
-            }
-            weight
+            migration::v4::migrate_rounds_to_include_milestone_key::<T>()
         }
 
         // SAFETY: ExpiringProjectRoundsPerBlock has to be sane to prevent overweight blocks.
@@ -291,7 +285,7 @@ pub mod pallet {
                 weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1));
 
                 // Remove the round prevents further voting.
-                Rounds::<T>::remove(project_key, round_type);
+                Rounds::<T>::remove((project_key, milestone_key), round_type);
                 match round_type {
                     // Voting rounds automatically finalise if its reached its threshold.
                     // Therefore we can remove it on round end.

pallets/proposals/src/migration.rs

@@ -1,5 +1,5 @@
 use crate::*;
-use frame_support::{pallet_prelude::OptionQuery, storage_alias, traits::Get, weights::Weight};
+use frame_support::*;
 pub use pallet::*;
 pub type TimestampOf<T> = <T as pallet_timestamp::Config>::Moment;
 
@@ -385,7 +385,7 @@ pub mod v3 {
                     round.project_keys.iter().for_each(|k| {
                         // Insert per project_key
                         *weight += T::DbWeight::get().reads_writes(1, 1);
-                        Rounds::<T>::insert(k, round.round_type.into_new(), round.end);
+                        v4::V4Rounds::<T>::insert(k, round.round_type.into_new(), round.end);
                     })
                 }
             }
@@ -405,10 +405,53 @@ pub mod v3 {
     }
 }
 
+pub(crate) mod v4 {
+    use super::*;
+    #[storage_alias]
+    pub type V4Rounds<T: Config> = StorageDoubleMap<
+        crate::Pallet<T>,
+        Blake2_128Concat,
+        ProjectKey,
+        Blake2_128Concat,
+        crate::RoundType,
+        BlockNumberFor<T>,
+        OptionQuery,
+    >;
+
+    // Essentially remove all votes that currenctly exist and force a resubmission of milestones.
+    pub(crate) fn migrate_rounds_to_include_milestone_key<T: Config>() -> Weight {
+        let mut weight = <Weight as Default>::default();
+        if ProjectStorageVersion::<T>::get() == Release::V3 {
+            V4Rounds::<T>::drain().for_each(|(project_key, _, block_number)| {
+                weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1));
+
+                weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1));
+                crate::RoundsExpiring::<T>::remove(block_number);
+
+                weight = weight.saturating_add(T::DbWeight::get().reads(1));
+                if let Some(project) = crate::Projects::<T>::get(project_key) {
+                    for (milestone_key, _) in project.milestones.iter() {
+                        weight = weight.saturating_add(T::DbWeight::get().reads(1));
+                        if crate::MilestoneVotes::<T>::contains_key(project_key, milestone_key) {
+                            weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1));
+                            crate::MilestoneVotes::<T>::remove(project_key, milestone_key);
+                        } else {
+                            break;
+                        }
+                    }
+                }
+            });
+            ProjectStorageVersion::<T>::set(Release::V4);
+        }
+        weight
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::*;
     use mock::*;
+    use test_utils::*;
 
     use v0::{ContributionV0, MilestoneV0, ProjectV0};
 
@@ -691,9 +734,44 @@ mod test {
             // #5
             assert!(OldRounds::<Test>::get(0).is_none());
             [1, 2, 3].iter().for_each(|k| {
-                let end = crate::Rounds::<Test>::get(k, crate::RoundType::VotingRound).unwrap();
+                let end = v4::V4Rounds::<Test>::get(k, crate::RoundType::VotingRound).unwrap();
                 assert_eq!(end, end_block_number);
             });
         })
     }
+
+    #[test]
+    fn migrate_v3_to_v4() {
+        build_test_externality().execute_with(|| {
+            let cont = get_contributions::<Test>(vec![*BOB, *DAVE], 100_000);
+            let prop_milestones = get_milestones(10);
+            let project_key =
+                create_project::<Test>(*ALICE, cont, prop_milestones, CurrencyId::Native);
+            let milestone_key: MilestoneKey = 0;
+            let expiry_block: BlockNumber = 10;
+            let rounds_expiring: BoundedProjectKeysPerBlock<Test> =
+                vec![(project_key, RoundType::VotingRound, milestone_key)]
+                    .try_into()
+                    .unwrap();
+
+            // insert a fake round to be mutated.
+            v4::V4Rounds::<Test>::insert(project_key, crate::RoundType::VotingRound, expiry_block);
+            crate::RoundsExpiring::<Test>::insert(expiry_block, rounds_expiring);
+            crate::MilestoneVotes::<Test>::insert(project_key, milestone_key, Vote::default());
+            crate::MilestoneVotes::<Test>::insert(project_key, milestone_key + 1, Vote::default());
+
+            let _ = v4::migrate_rounds_to_include_milestone_key::<Test>();
+            // assert that:
+            // 1: the round has been removed (to allow resubmission)
+            // 2: milestone votes have been reset (although resubmission resets this)
+            // 3: the round doesnt try and autocomplete and remove itself inadvertantly. (RoundsExpiring)
+            assert!(!v4::V4Rounds::<Test>::contains_key(
+                project_key,
+                crate::RoundType::VotingRound
+            ));
+            assert!(crate::RoundsExpiring::<Test>::get(expiry_block).is_empty());
+            assert!(crate::MilestoneVotes::<Test>::get(project_key, milestone_key).is_none());
+            assert!(crate::MilestoneVotes::<Test>::get(project_key, milestone_key + 1).is_none());
+        })
+    }
 }

pallets/proposals/src/tests.rs

@@ -211,9 +211,9 @@ fn ensure_milestone_vote_data_is_cleaned_after_autofinalisation_for() {
         ));
 
         // Assert that the state is good before auto finalisation
-        let exp_block = Rounds::<Test>::get(project_key, RoundType::VotingRound)
+        let exp_block = Rounds::<Test>::get(&(project_key, milestone_key), RoundType::VotingRound)
             .expect("There should be a round here for the project_key");
-        assert!(RoundsExpiring::<Test>::get(exp_block).contains(&(
+        assert!(RoundsExpiring::<Test>::get(&exp_block).contains(&(
             project_key,
             RoundType::VotingRound,
             milestone_key
@@ -231,9 +231,11 @@ fn ensure_milestone_vote_data_is_cleaned_after_autofinalisation_for() {
             true
         ));
 
-        assert!(Rounds::<Test>::get(project_key, RoundType::VotingRound).is_none());
+        assert!(
+            Rounds::<Test>::get(&(project_key, milestone_key), RoundType::VotingRound).is_none()
+        );
         assert_eq!(
-            RoundsExpiring::<Test>::get(exp_block).len(),
+            RoundsExpiring::<Test>::get(&exp_block).len(),
             0,
             "This vec should have been emptied on auto finalisation."
         );
@@ -264,9 +266,9 @@ fn ensure_milestone_vote_data_is_cleaned_after_autofinalisation_against() {
         ));
 
         // Assert that the state is good before auto finalisation
-        let exp_block = Rounds::<Test>::get(project_key, RoundType::VotingRound)
+        let exp_block = Rounds::<Test>::get(&(project_key, milestone_key), RoundType::VotingRound)
             .expect("There should be a round here for the project_key");
-        assert!(RoundsExpiring::<Test>::get(exp_block).contains(&(
+        assert!(RoundsExpiring::<Test>::get(&exp_block).contains(&(
             project_key,
             RoundType::VotingRound,
             milestone_key
@@ -284,9 +286,11 @@ fn ensure_milestone_vote_data_is_cleaned_after_autofinalisation_against() {
             false
         ));
 
-        assert!(Rounds::<Test>::get(project_key, RoundType::VotingRound).is_none());
+        assert!(
+            Rounds::<Test>::get(&(project_key, milestone_key), RoundType::VotingRound).is_none()
+        );
         assert_eq!(
-            RoundsExpiring::<Test>::get(exp_block).len(),
+            RoundsExpiring::<Test>::get(&exp_block).len(),
             0,
             "This vec should have been emptied on auto finalisation."
         );
@@ -413,6 +417,35 @@ fn vote_on_milestone_where_voting_round_is_active_but_not_the_correct_milestone(
     });
 }
 
+#[test]
+fn if_double_submission_and_one_finalises_voting_on_the_second_can_vote() {
+    build_test_externality().execute_with(|| {
+        let cont = get_contributions::<Test>(vec![*BOB, *CHARLIE], 100_000);
+        let prop_milestones = get_milestones(10);
+        let project_key = create_project::<Test>(*ALICE, cont, prop_milestones, CurrencyId::Native);
+        let expiring_block = frame_system::Pallet::<Test>::block_number()
+            + <Test as Config>::MilestoneVotingWindow::get();
+        assert_ok!(Proposals::submit_milestone(
+            RuntimeOrigin::signed(*ALICE),
+            project_key,
+            0
+        ));
+        run_to_block::<Test>(frame_system::Pallet::<Test>::block_number() + 10);
+        assert_ok!(Proposals::submit_milestone(
+            RuntimeOrigin::signed(*ALICE),
+            project_key,
+            1
+        ));
+        run_to_block::<Test>(expiring_block);
+        assert_ok!(Proposals::vote_on_milestone(
+            RuntimeOrigin::signed(*BOB),
+            project_key,
+            1,
+            true
+        ));
+    });
+}
+
 #[test]
 fn vote_on_milestone_not_contributor() {
     build_test_externality().execute_with(|| {
@@ -1051,7 +1084,6 @@ fn auto_finalizing_vote_on_no_confidence_when_threshold_is_met() {
         let cont = get_contributions::<Test>(vec![*BOB, *DAVE, *CHARLIE, *ALICE], 100_000);
         let prop_milestones = get_milestones(10);
         let project_key = create_project::<Test>(*ALICE, cont, prop_milestones, CurrencyId::Native);
-
         assert_ok!(Proposals::raise_vote_of_no_confidence(
             RuntimeOrigin::signed(*BOB),
             project_key
@@ -1098,7 +1130,7 @@ fn auto_finalizing_vote_on_no_confidence_when_threshold_is_met() {
         );
         assert_eq!(Projects::<Test>::get(project_key), None);
         assert_eq!(
-            Rounds::<Test>::get(project_key, RoundType::VoteOfNoConfidence),
+            Rounds::<Test>::get((project_key, 0), RoundType::VoteOfNoConfidence),
             None
         );
     });
@@ -1107,7 +1139,7 @@ fn auto_finalizing_vote_on_no_confidence_when_threshold_is_met() {
 #[test]
 fn close_voting_round_works() {
     build_test_externality().execute_with(|| {
-        Rounds::<Test>::insert(0, RoundType::VotingRound, 100);
+        Rounds::<Test>::insert((0, 0), RoundType::VotingRound, 100);
         let r_expiring: BoundedVec<
             (ProjectKey, RoundType, MilestoneKey),
             <Test as Config>::ExpiringProjectRoundsPerBlock,
@@ -1121,7 +1153,7 @@ fn close_voting_round_works() {
             0,
             (0, RoundType::VotingRound, 0)
         ));
-        assert!(Rounds::<Test>::get(0, RoundType::VotingRound).is_none());
+        assert!(Rounds::<Test>::get((0, 0), RoundType::VotingRound).is_none());
         assert!(RoundsExpiring::<Test>::get(100).len() == 0);
         assert!(UserHasVoted::<Test>::get((0, RoundType::VotingRound, 0)).is_empty());
     })