replace ProposalState::Canceled with distinguished Rejected and Super…

…sededBy(id)

democracy/src/lib.rs

@@ -75,7 +75,10 @@ pub mod pallet {
 	use frame_support::pallet_prelude::*;
 	use frame_system::pallet_prelude::*;
 
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
 	#[pallet::pallet]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T>(PhantomData<T>);
 
 	#[pallet::config]
@@ -192,9 +195,14 @@ pub mod pallet {
 		StorageMap<_, Blake2_128Concat, ProposalIdType, Tally, OptionQuery>;
 
 	#[pallet::storage]
-	#[pallet::getter(fn cancelled_at)]
-	pub(super) type CancelledAt<T: Config> =
-		StorageMap<_, Blake2_128Concat, ProposalActionIdentifier, T::Moment, OptionQuery>;
+	#[pallet::getter(fn last_approved_proposal_for_action)]
+	pub(super) type LastApprovedProposalForAction<T: Config> = StorageMap<
+		_,
+		Blake2_128Concat,
+		ProposalActionIdentifier,
+		(T::Moment, ProposalIdType),
+		OptionQuery,
+	>;
 
 	#[pallet::storage]
 	#[pallet::getter(fn enactment_queue)]
@@ -425,12 +433,14 @@ pub mod pallet {
 			let old_proposal_state = proposal.state;
 			let now = <pallet_timestamp::Pallet<T>>::get();
 			let proposal_action_identifier = proposal.action.clone().get_identifier();
-			let cancelled_at = Self::cancelled_at(proposal_action_identifier);
-			let proposal_cancelled_by_other =
-				cancelled_at.is_some() && proposal.start < cancelled_at.unwrap();
+			let last_approved_proposal_for_action =
+				Self::last_approved_proposal_for_action(proposal_action_identifier);
+			let proposal_cancelled_by_other = last_approved_proposal_for_action.is_some() &&
+				proposal.start < last_approved_proposal_for_action.unwrap().0;
 			let proposal_too_old = now - proposal.start > T::ProposalLifetime::get();
 			if proposal_cancelled_by_other {
-				proposal.state = ProposalState::Cancelled;
+				proposal.state =
+					ProposalState::SupersededBy { id: last_approved_proposal_for_action.unwrap().1 }
 			} else {
 				// passing
 				if Self::is_passing(proposal_id)? {
@@ -442,19 +452,22 @@ pub mod pallet {
 						{
 							proposal.state = ProposalState::Approved;
 							<EnactmentQueue<T>>::insert(proposal_action_identifier, proposal_id);
-							<CancelledAt<T>>::insert(proposal_action_identifier, now);
+							<LastApprovedProposalForAction<T>>::insert(
+								proposal_action_identifier,
+								(now, proposal_id),
+							);
 							approved = true;
 						}
 					// not yet confirming
 					} else if proposal_too_old {
-						proposal.state = ProposalState::Cancelled;
+						proposal.state = ProposalState::Rejected;
 					} else {
 						proposal.state = ProposalState::Confirming { since: now };
 					}
 
 				// not passing
 				} else if proposal_too_old {
-					proposal.state = ProposalState::Cancelled;
+					proposal.state = ProposalState::Rejected;
 				} else if let ProposalState::Confirming { since: _ } = proposal.state {
 					proposal.state = ProposalState::Ongoing;
 				}
@@ -588,6 +601,7 @@ impl<T: Config> OnCeremonyPhaseChange for Pallet<T> {
 	}
 }
 
+mod migrations;
 #[cfg(test)]
 mod mock;
 #[cfg(test)]

democracy/src/migrations.rs

@@ -0,0 +1,135 @@
+use super::*;
+
+use frame_support::{pallet_prelude::*, storage_alias, traits::OnRuntimeUpgrade};
+
+/// The log target.
+const TARGET: &str = "democracy::migration::v1";
+
+mod v0 {
+	use super::*;
+	use encointer_primitives::democracy::ProposalActionIdentifier;
+
+	#[storage_alias]
+	pub(super) type CancelledAt<T: Config> =
+		StorageMap<Pallet<T>, Blake2_128Concat, ProposalActionIdentifier, u64, OptionQuery>;
+}
+
+pub mod v1 {
+	use super::*;
+
+	pub struct MigrateV0toV1purging<T>(sp_std::marker::PhantomData<T>);
+
+	impl<T: Config + frame_system::Config> OnRuntimeUpgrade for MigrateV0toV1purging<T> {
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::DispatchError> {
+			Ok(0u8.encode())
+		}
+
+		fn on_runtime_upgrade() -> Weight {
+			let current_version = Pallet::<T>::in_code_storage_version();
+			let onchain_version = Pallet::<T>::on_chain_storage_version();
+
+			log::info!(
+				target: TARGET,
+				"Running migration with current storage version {:?} / onchain {:?}",
+				current_version,
+				onchain_version
+			);
+
+			if onchain_version >= 1 {
+				log::warn!(
+					target: TARGET,
+					"skipping on_runtime_upgrade: executed on wrong storage version."
+				);
+				return T::DbWeight::get().reads(1)
+			}
+
+			let mut purged_keys = 0u64;
+			// this has been refactored to LastApprovedProposalForAction
+			purged_keys += v0::CancelledAt::<T>::clear(u32::MAX, None).unique as u64;
+			// ProposalState incompatible with new proposal struct
+			purged_keys += Proposals::<T>::clear(u32::MAX, None).unique as u64;
+			// ProposalState incompatible with new proposal struct
+			purged_keys += EnactmentQueue::<T>::clear(u32::MAX, None).unique as u64;
+			// we must keep ProposalCount and PurposeIds as we dont want to purge burnt rep
+
+			StorageVersion::new(1).put::<Pallet<T>>();
+			T::DbWeight::get().reads_writes(purged_keys, purged_keys + 1)
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(_state: Vec<u8>) -> Result<(), sp_runtime::DispatchError> {
+			assert_eq!(Pallet::<T>::on_chain_storage_version(), 1, "must upgrade");
+			Ok(())
+		}
+	}
+}
+
+#[cfg(test)]
+#[cfg(feature = "try-runtime")]
+mod test {
+	use super::*;
+	use encointer_primitives::democracy::{ProposalActionIdentifier, ProposalState};
+	use frame_support::{assert_storage_noop, traits::OnRuntimeUpgrade};
+	use mock::{new_test_ext, TestRuntime};
+
+	#[allow(deprecated)]
+	#[test]
+	fn migration_v0_to_v1_works() {
+		new_test_ext().execute_with(|| {
+			StorageVersion::new(0).put::<Pallet<TestRuntime>>();
+
+			// Insert some values into the v0 storage:
+
+			v0::CancelledAt::<TestRuntime>::insert(
+				ProposalActionIdentifier::SetInactivityTimeout,
+				42,
+			);
+			Proposals::<TestRuntime>::insert(
+				1,
+				Proposal {
+					start: 0,
+					start_cindex: 0,
+					action: ProposalAction::SetInactivityTimeout(42),
+					state: ProposalState::Approved,
+					electorate_size: 0,
+				},
+			);
+			EnactmentQueue::<TestRuntime>::insert(
+				ProposalActionIdentifier::SetInactivityTimeout,
+				1,
+			);
+
+			assert_eq!(v0::CancelledAt::<TestRuntime>::iter_keys().count(), 1);
+			assert_eq!(Proposals::<TestRuntime>::iter_keys().count(), 1);
+			assert_eq!(EnactmentQueue::<TestRuntime>::iter_keys().count(), 1);
+
+			// Migrate V0 to V1.
+			let state = v1::MigrateV0toV1purging::<TestRuntime>::pre_upgrade().unwrap();
+			let _weight = v1::MigrateV0toV1purging::<TestRuntime>::on_runtime_upgrade();
+			v1::MigrateV0toV1purging::<TestRuntime>::post_upgrade(state).unwrap();
+
+			// Check that all values got migrated.
+			assert_eq!(v0::CancelledAt::<TestRuntime>::iter_keys().count(), 0);
+			assert_eq!(Proposals::<TestRuntime>::iter_keys().count(), 0);
+			assert_eq!(EnactmentQueue::<TestRuntime>::iter_keys().count(), 0);
+		});
+	}
+
+	#[allow(deprecated)]
+	#[test]
+	fn migration_v1_to_v1_is_noop() {
+		new_test_ext().execute_with(|| {
+			StorageVersion::new(1).put::<Pallet<TestRuntime>>();
+
+			LastApprovedProposalForAction::<TestRuntime>::insert(
+				ProposalActionIdentifier::SetInactivityTimeout,
+				(42, 43),
+			);
+
+			let state = v1::MigrateV0toV1purging::<TestRuntime>::pre_upgrade().unwrap();
+			assert_storage_noop!(v1::MigrateV0toV1purging::<TestRuntime>::on_runtime_upgrade());
+			v1::MigrateV0toV1purging::<TestRuntime>::post_upgrade(state).unwrap();
+		});
+	}
+}

democracy/src/tests.rs

@@ -412,7 +412,7 @@ fn do_update_proposal_state_fails_with_wrong_state() {
 			start: Moment::from(1u64),
 			start_cindex: 1,
 			action: ProposalAction::UpdateNominalIncome(cid, NominalIncomeType::from(100u32)),
-			state: ProposalState::Cancelled,
+			state: ProposalState::Rejected,
 			electorate_size: 0,
 		};
 		Proposals::<TestRuntime>::insert(1, proposal);
@@ -426,6 +426,15 @@ fn do_update_proposal_state_fails_with_wrong_state() {
 		};
 		Proposals::<TestRuntime>::insert(2, proposal2);
 
+		let proposal3: Proposal<Moment> = Proposal {
+			start: Moment::from(1u64),
+			start_cindex: 1,
+			action: ProposalAction::UpdateNominalIncome(cid, NominalIncomeType::from(100u32)),
+			state: ProposalState::SupersededBy { id: 2 },
+			electorate_size: 0,
+		};
+		Proposals::<TestRuntime>::insert(3, proposal3);
+
 		assert_err!(
 			EncointerDemocracy::do_update_proposal_state(1),
 			Error::<TestRuntime>::ProposalCannotBeUpdated
@@ -435,6 +444,11 @@ fn do_update_proposal_state_fails_with_wrong_state() {
 			EncointerDemocracy::do_update_proposal_state(2),
 			Error::<TestRuntime>::ProposalCannotBeUpdated
 		);
+
+		assert_err!(
+			EncointerDemocracy::do_update_proposal_state(3),
+			Error::<TestRuntime>::ProposalCannotBeUpdated
+		);
 	});
 }
 
@@ -449,9 +463,9 @@ fn do_update_proposal_state_cancels_superseded_proposal() {
 		));
 
 		//another proposal of same action has been scheduled for enactment
-		CancelledAt::<TestRuntime>::insert(
+		LastApprovedProposalForAction::<TestRuntime>::insert(
 			ProposalActionIdentifier::SetInactivityTimeout,
-			3 * BLOCKTIME,
+			(3 * BLOCKTIME, 2),
 		);
 
 		assert_eq!(EncointerDemocracy::proposals(1).unwrap().state, ProposalState::Ongoing);
@@ -460,7 +474,10 @@ fn do_update_proposal_state_cancels_superseded_proposal() {
 
 		assert_ok!(EncointerDemocracy::do_update_proposal_state(1));
 
-		assert_eq!(EncointerDemocracy::proposals(1).unwrap().state, ProposalState::Cancelled);
+		assert_eq!(
+			EncointerDemocracy::proposals(1).unwrap().state,
+			ProposalState::SupersededBy { id: 2 }
+		);
 	});
 }
 
@@ -483,7 +500,7 @@ fn do_update_proposal_state_works_with_too_old_proposal() {
 		advance_n_blocks(1);
 
 		assert_ok!(EncointerDemocracy::do_update_proposal_state(1));
-		assert_eq!(EncointerDemocracy::proposals(1).unwrap().state, ProposalState::Cancelled);
+		assert_eq!(EncointerDemocracy::proposals(1).unwrap().state, ProposalState::Rejected);
 	});
 }
 

primitives/src/democracy.rs

@@ -104,7 +104,8 @@ pub enum ProposalState<Moment> {
 	Ongoing,
 	Confirming { since: Moment },
 	Approved,
-	Cancelled,
+	SupersededBy { id: ProposalIdType },
+	Rejected,
 	Enacted,
 }