Addresses PR review comments

Co-authored-by: Liam Aharon <[email protected]>

frame/election-provider-multi-phase/src/lib.rs

@@ -1258,7 +1258,7 @@ pub mod pallet {
 
 	/// Current best solution, signed or unsigned, queued to be returned upon `elect`.
 	///
-	/// Invariant: Always sorted by score.
+	/// Always sorted by score.
 	#[pallet::storage]
 	#[pallet::getter(fn queued_solution)]
 	pub type QueuedSolution<T: Config> =
@@ -1589,10 +1589,15 @@ impl<T: Config> Pallet<T> {
 	// - [`DesiredTargets`] exist IFF [`Snapshot`] is present.
 	// - [`SnapshotMetadata`] exist IFF [`Snapshot`] is present.
 	fn try_state_snapshot() -> Result<(), &'static str> {
-		let set = <DesiredTargets<T>>::get().is_some() && <DesiredTargets<T>>::get().is_some();
-
-		match <Snapshot<T>>::take().is_some() {
-            true if !set => Err("If the snapshot exists, the desired targets and snapshot metadata must also exist."),
+		let set = <DesiredTargets<T>>::get().is_some() && <SnapshotMetadata<T>>::get().is_some();
+
+		match <Snapshot<T>>::take() {
+            Some(_) if !set => Err("If the snapshot exists, the desired targets and snapshot metadata must also exist."),
+            None if set => {
+                println!("desired targets: {:?}", <DesiredTargets<T>>::get());
+                println!("snapshot metadata: {:?}", <SnapshotMetadata<T>>::get());
+                Err("If the snapshot does not exists, the desired targets and snapshot metadata should also not exists")
+            },
             _ => Ok(()),
 		}
 	}
@@ -1722,6 +1727,9 @@ mod feasibility_check {
 				MultiPhase::feasibility_check(solution, COMPUTE),
 				FeasibilityError::SnapshotUnavailable
 			);
+
+			// restore noop snapshot to ensure post-test `try_state` checks pass.
+			<Snapshot<Runtime>>::set(Some(Default::default()));
 		})
 	}
 

frame/elections-phragmen/src/lib.rs

@@ -1208,8 +1208,8 @@ impl<T: Config> Pallet<T> {
 		Self::try_state_members_approval_stake()
 	}
 
-	// [`Members`] state checks. Invariants:
-	//  - Members are always sorted based on account ID.
+	/// [`Members`] state checks. Invariants:
+	///  - Members are always sorted based on account ID.
 	fn try_state_members() -> Result<(), &'static str> {
 		let mut members = Members::<T>::get().clone();
 		members.sort_by_key(|m| m.who.clone());
@@ -1222,9 +1222,17 @@ impl<T: Config> Pallet<T> {
 	}
 
 	// [`RunnersUp`] state checks. Invariants:
-	//  - Elements are sorted based on rank.
+	//  - Elements are sorted based on weight (worst to best).
 	fn try_state_runners_up() -> Result<(), &'static str> {
-		Ok(())
+		let mut sorted = RunnersUp::<T>::get();
+		// worst stake first
+		sorted.sort_by(|a, b| a.stake.cmp(&b.stake));
+
+		if RunnersUp::<T>::get() == sorted {
+			Ok(())
+		} else {
+			Err("try_state checks: Runners Up must always be sorted by stake (worst to best)")
+		}
 	}
 
 	// [`Candidates`] state checks. Invariants: