[democracy] several fixes and enhancements

communities/src/lib.rs

@@ -175,6 +175,11 @@ pub mod pallet {
 			location: Location,
 		) -> DispatchResultWithPostInfo {
 			T::TrustableForNonDestructiveAction::ensure_origin(origin)?;
+			ensure!(
+				<pallet_encointer_scheduler::Pallet<T>>::current_phase() ==
+					CeremonyPhaseType::Registering,
+				Error::<T>::RegistrationPhaseRequired
+			);
 			Self::do_add_location(cid, location)
 		}
 
@@ -191,6 +196,11 @@ pub mod pallet {
 			location: Location,
 		) -> DispatchResultWithPostInfo {
 			T::CommunityMaster::ensure_origin(origin)?;
+			ensure!(
+				<pallet_encointer_scheduler::Pallet<T>>::current_phase() ==
+					CeremonyPhaseType::Registering,
+				Error::<T>::RegistrationPhaseRequired
+			);
 			Self::do_remove_location(cid, location)
 		}
 
@@ -409,11 +419,6 @@ impl<T: Config> Pallet<T> {
 		cid: CommunityIdentifier,
 		location: Location,
 	) -> DispatchResultWithPostInfo {
-		ensure!(
-			<pallet_encointer_scheduler::Pallet<T>>::current_phase() ==
-				CeremonyPhaseType::Registering,
-			Error::<T>::RegistrationPhaseRequired
-		);
 		Self::ensure_cid_exists(&cid)?;
 		Self::validate_location(&location)?;
 		let geo_hash = GeoHash::try_from_params(location.lat, location.lon)
@@ -450,11 +455,6 @@ impl<T: Config> Pallet<T> {
 		cid: CommunityIdentifier,
 		location: Location,
 	) -> DispatchResultWithPostInfo {
-		ensure!(
-			<pallet_encointer_scheduler::Pallet<T>>::current_phase() ==
-				CeremonyPhaseType::Registering,
-			Error::<T>::RegistrationPhaseRequired
-		);
 		Self::ensure_cid_exists(&cid)?;
 
 		let geo_hash = GeoHash::try_from_params(location.lat, location.lon)

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)]
@@ -273,6 +281,15 @@ pub mod pallet {
 			let sender = ensure_signed(origin)?;
 			let tally = <Tallies<T>>::get(proposal_id).ok_or(Error::<T>::InexistentProposal)?;
 
+			// make sure we don't vote on proposal that can't update anymore
+			match Self::do_update_proposal_state(proposal_id) {
+				Ok(_) => Ok(()),
+				Err(error) => match error {
+					Error::<T>::ProposalCannotBeUpdated => Ok(()),
+					other_error => Err(other_error),
+				},
+			}?;
+
 			let num_votes =
 				Self::validate_and_commit_reputations(proposal_id, &sender, &reputations)?;
 
@@ -416,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 =
-				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 || proposal_too_old {
-				proposal.state = ProposalState::Cancelled;
+			if proposal_cancelled_by_other {
+				proposal.state =
+					ProposalState::SupersededBy { id: last_approved_proposal_for_action.unwrap().1 }
 			} else {
 				// passing
 				if Self::is_passing(proposal_id)? {
@@ -433,19 +452,24 @@ 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 confirming
+					// not yet confirming
+					} else if proposal_too_old {
+						proposal.state = ProposalState::Rejected;
 					} else {
 						proposal.state = ProposalState::Confirming { since: now };
 					}
+
 				// not passing
-				} else {
-					// confirming
-					if let ProposalState::Confirming { since: _ } = proposal.state {
-						proposal.state = ProposalState::Ongoing;
-					}
+				} else if proposal_too_old {
+					proposal.state = ProposalState::Rejected;
+				} else if let ProposalState::Confirming { since: _ } = proposal.state {
+					proposal.state = ProposalState::Ongoing;
 				}
 			}
 			<Proposals<T>>::insert(proposal_id, &proposal);
@@ -519,7 +543,7 @@ pub mod pallet {
 
 			let turnout_permill = (tally.turnout * 1000).checked_div(electorate).unwrap_or(0);
 			if turnout_permill < T::MinTurnout::get() {
-				return Ok(false);
+				return Ok(false)
 			}
 
 			Self::positive_turnout_bias(electorate, tally.turnout, tally.ayes)
@@ -561,9 +585,7 @@ pub mod pallet {
 impl<T: Config> OnCeremonyPhaseChange for Pallet<T> {
 	fn on_ceremony_phase_change(new_phase: CeremonyPhaseType) {
 		match new_phase {
-			CeremonyPhaseType::Assigning => {},
-			CeremonyPhaseType::Attesting => {},
-			CeremonyPhaseType::Registering => {
+			CeremonyPhaseType::Assigning => {
 				// safe as EnactmentQueue has one key per ProposalActionType and those are bounded
 				<EnactmentQueue<T>>::iter().for_each(|p| {
 					if let Err(e) = Self::enact_proposal(p.1) {
@@ -573,10 +595,13 @@ impl<T: Config> OnCeremonyPhaseChange for Pallet<T> {
 				// remove all keys from the map
 				<EnactmentQueue<T>>::translate::<ProposalIdType, _>(|_, _| None);
 			},
+			CeremonyPhaseType::Attesting => {},
+			CeremonyPhaseType::Registering => {},
 		}
 	}
 }
 
+mod migrations;
 #[cfg(test)]
 mod mock;
 #[cfg(test)]

democracy/src/migrations.rs

@@ -0,0 +1,136 @@
+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::*;
+
+	#[allow(dead_code)]
+	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/mock.rs

@@ -45,9 +45,9 @@ frame_support::construct_runtime!(
 impl dut::Config for TestRuntime {
 	type RuntimeEvent = RuntimeEvent;
 	type MaxReputationCount = ConstU32<10>;
-	// 10 blocks
+	// 10 6s blocks
 	type ConfirmationPeriod = ConstU64<60000>;
-	// 40 blocks
+	// 40 6s blocks
 	type ProposalLifetime = ConstU64<240000>;
 	type MinTurnout = ConstU128<20>; // 2%
 	type WeightInfo = (); // 2%