Fix tests and phragmen fallback. Avoid double-controlling

core/sr-std/src/lib.rs

@@ -45,7 +45,6 @@ pub mod prelude {
 	pub use crate::boxed::Box;
 	pub use crate::cmp::{Eq, PartialEq};
 	pub use crate::clone::Clone;
-	pub use crate::alloc::prelude::*;
 
 	// Re-export `vec!` macro here, but not in `std` mode, since
 	// std's prelude already brings `vec!` into the scope.

srml/staking/src/lib.rs

@@ -534,6 +534,10 @@ decl_module! {
 
 			let controller = T::Lookup::lookup(controller)?;
 
+			if <Ledger<T>>::exists(&controller) {
+				return Err("controller already paired")
+			}
+
 			// You're auto-bonded forever, here. We might improve this by only bonding when
 			// you actually validate/nominate.
 			<Bonded<T>>::insert(&stash, controller.clone());
@@ -665,6 +669,25 @@ decl_module! {
 			<Payee<T>>::insert(&controller, payee);
 		}
 
+		/// (Re-)set the payment target for a controller.
+		///
+		/// Effects will be felt at the beginning of the next era.
+		///
+		/// The dispatch origin for this call must be _Signed_ by the controller, not the stash.
+		fn set_controller(origin, controller: <T::Lookup as StaticLookup>::Source) {
+			let stash = ensure_signed(origin)?;
+			let old_controller = Self::bonded(&stash).ok_or("not a stash")?;
+			let controller = T::Lookup::lookup(controller)?;
+			if <Ledger<T>>::exists(&controller) {
+				return Err("controller already paired")
+			}
+			if controller != old_controller {
+				<Bonded<T>>::insert(&stash, &controller);
+				if let Some(l) = <Ledger<T>>::take(&old_controller) { <Ledger<T>>::insert(&controller, l) };
+				<Payee<T>>::insert(&controller, <Payee<T>>::take(&old_controller));
+			}
+		}
+
 		/// Set the number of sessions in an era.
 		fn set_sessions_per_era(#[compact] new: T::BlockNumber) {
 			<NextSessionsPerEra<T>>::put(new);
@@ -887,7 +910,7 @@ impl<T: Trait> Module<T> {
 		let nominators = || <Nominators<T>>::enumerate();
 		let stash_of = |w: &T::AccountId| -> BalanceOf<T> { Self::stash_balance(w) };
 		let min_validator_count = Self::minimum_validator_count() as usize;
-		let elected_candidates = elect::<T, _, _, _, _>(
+		let maybe_elected_candidates = elect::<T, _, _, _, _>(
 			rounds,
 			validators,
 			nominators,
@@ -900,31 +923,40 @@ impl<T: Trait> Module<T> {
 			}
 		);
 
-		// Clear Stakers and reduce their slash_count.
-		for v in <session::Module<T>>::validators().iter() {
-			<Stakers<T>>::remove(v);
-			let slash_count = <SlashCount<T>>::take(v);
-			if slash_count > 1 {
-				<SlashCount<T>>::insert(v, slash_count - 1);
+		if let Some(elected_candidates) = maybe_elected_candidates {
+			// Clear Stakers and reduce their slash_count.
+			for v in <session::Module<T>>::validators().iter() {
+				<Stakers<T>>::remove(v);
+				let slash_count = <SlashCount<T>>::take(v);
+				if slash_count > 1 {
+					<SlashCount<T>>::insert(v, slash_count - 1);
+				}
 			}
-		}
 
-		// Populate Stakers and figure out the minimum stake behind a slot.
-		let mut slot_stake = elected_candidates[0].exposure.total;
-		for c in &elected_candidates {
-			if c.exposure.total < slot_stake {
-				slot_stake = c.exposure.total;
+			// Populate Stakers and figure out the minimum stake behind a slot.
+			let mut slot_stake = elected_candidates[0].exposure.total;
+			for c in &elected_candidates {
+				if c.exposure.total < slot_stake {
+					slot_stake = c.exposure.total;
+				}
+				<Stakers<T>>::insert(c.who.clone(), c.exposure.clone());
 			}
-			<Stakers<T>>::insert(c.who.clone(), c.exposure.clone());
-		}
-		<SlotStake<T>>::put(&slot_stake);
+			<SlotStake<T>>::put(&slot_stake);
 
-		// Set the new validator set.
-		<session::Module<T>>::set_validators(
-			&elected_candidates.into_iter().map(|i| i.who).collect::<Vec<_>>()
-		);
+			// Set the new validator set.
+			<session::Module<T>>::set_validators(
+				&elected_candidates.into_iter().map(|i| i.who).collect::<Vec<_>>()
+			);
 
-		slot_stake
+			slot_stake
+		} else {
+			// There were not enough candidates for even our minimal level of functionality.
+			// This is bad.
+			// We should probably disable all functionality except for block production
+			// and let the chain keep producing blocks until we can decide on a sufficiently
+			// substantial set.
+			Self::slot_stake()
+		}
 	}
 
 	/// Call when a validator is determined to be offline. `count` is the

srml/staking/src/phragmen.rs

@@ -36,7 +36,7 @@ pub struct ElectionConfig<Balance: HasCompact> {
 
 // Wrapper around validation candidates some metadata.
 #[derive(Clone, Encode, Decode, Default)]
-#[cfg_attr(feature = "std", derive(Debug, Default))]
+#[cfg_attr(feature = "std", derive(Debug))]
 pub struct Candidate<AccountId, Balance: HasCompact> {
 	// The validator's account
 	pub who: AccountId,
@@ -96,7 +96,7 @@ pub fn elect<T: Trait + 'static, FR, FN, FV, FS>(
 		stash_of: FS,
 		minimum_validator_count: usize,
 		config: ElectionConfig<BalanceOf<T>>,
-) -> Vec<Candidate<T::AccountId, BalanceOf<T>>> where
+) -> Option<Vec<Candidate<T::AccountId, BalanceOf<T>>>> where
 	FR: Fn() -> usize,
 	FV: Fn() -> Box<dyn Iterator<
 		Item =(T::AccountId, ValidatorPrefs<BalanceOf<T>>)
@@ -119,9 +119,6 @@ pub fn elect<T: Trait + 'static, FR, FN, FV, FS>(
 		}
 	}).collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();
 
-	// Just to be used when we are below minimum validator count
-	let original_candidates = candidates.clone();
-
 	// 1.1- Add phantom votes.
 	let mut nominators: Vec<Nominator<T::AccountId, BalanceOf<T>>> = Vec::with_capacity(candidates.len());
 	candidates.iter_mut().enumerate().for_each(|(idx, c)| {
@@ -266,10 +263,10 @@ pub fn elect<T: Trait + 'static, FR, FN, FV, FS>(
 			}
 		} else {
 			// if we have less than minimum, use the previous validator set.
-			elected_candidates = original_candidates;
+			return None
 		}
 	}
-	elected_candidates
+	Some(elected_candidates)
 }
 
 pub fn equalise<T: Trait + 'static>(

srml/staking/src/tests.rs

@@ -562,15 +562,15 @@ fn no_candidate_emergency_condition() {
 		assert_eq!(Staking::validator_count(), 15);
 
 		// initial validators 
-		assert_eq!(Session::validators(), vec![40, 30, 20, 10]);
+		assert_eq!(Session::validators(), vec![10, 20, 30, 40]);
 
 		// trigger era
 		System::set_block_number(1);
 		Session::check_rotate_session(System::block_number());
 		assert_eq!(Staking::current_era(), 1);
 
 		// No one nominates => no one has a proper vote => no change
-		assert_eq!(Session::validators(), vec![40, 30, 20, 10]);
+		assert_eq!(Session::validators(), vec![10, 20, 30, 40]);
 	});
 }
 
@@ -756,20 +756,38 @@ fn double_staking_should_fail() {
 	with_externalities(&mut ExtBuilder::default()
 		.sessions_per_era(2)
 		.build(),
-	|| {
-		let arbitrary_value = 5;
-		System::set_block_number(1);
-		// 2 = controller, 1 stashed => ok
-		assert_ok!(Staking::bond(Origin::signed(1), 2, arbitrary_value, RewardDestination::default()));
-		// 2 = controller, 3 stashed (Note that 2 is reused.) => ok
-		assert_ok!(Staking::bond(Origin::signed(3), 2, arbitrary_value, RewardDestination::default()));
-		// 4 = not used so far, 1 stashed => not allowed.
-		assert_noop!(Staking::bond(Origin::signed(1), 4, arbitrary_value, RewardDestination::default()), "stash already bonded");
-		// 1 = stashed => attempting to nominate should fail.
-		assert_noop!(Staking::nominate(Origin::signed(1), vec![1]), "not a controller");
-		// 2 = controller  => nominating should work.
-		assert_ok!(Staking::nominate(Origin::signed(2), vec![1]));
-	});
+		|| {
+			let arbitrary_value = 5;
+			System::set_block_number(1);
+			// 2 = controller, 1 stashed => ok
+			assert_ok!(Staking::bond(Origin::signed(1), 2, arbitrary_value, RewardDestination::default()));
+			// 4 = not used so far, 1 stashed => not allowed.
+			assert_noop!(Staking::bond(Origin::signed(1), 4, arbitrary_value, RewardDestination::default()), "stash already bonded");
+			// 1 = stashed => attempting to nominate should fail.
+			assert_noop!(Staking::nominate(Origin::signed(1), vec![1]), "not a controller");
+			// 2 = controller  => nominating should work.
+			assert_ok!(Staking::nominate(Origin::signed(2), vec![1]));
+		});
+}
+
+#[test]
+fn double_controlling_should_fail() {
+	// should test (in the same order):
+	// * an account already bonded as controller CAN be reused as the controller of another account.
+	// * an account already bonded as stash cannot be the controller of another account.
+	// * an account already bonded as stash cannot nominate.
+	// * an account already bonded as controller can nominate.
+	with_externalities(&mut ExtBuilder::default()
+		.sessions_per_era(2)
+		.build(),
+		|| {
+			let arbitrary_value = 5;
+			System::set_block_number(1);
+			// 2 = controller, 1 stashed => ok
+			assert_ok!(Staking::bond(Origin::signed(1), 2, arbitrary_value, RewardDestination::default()));
+			// 2 = controller, 3 stashed (Note that 2 is reused.) => no-op
+			assert_noop!(Staking::bond(Origin::signed(3), 2, arbitrary_value, RewardDestination::default()), "controller already paired");
+		});
 }
 
 #[test]
@@ -1522,6 +1540,8 @@ fn phragmen_election_works_example_2() {
 			}
 		);
 
+		let winners = winners.unwrap();
+
 		// 10 and 30 must be the winners
 		assert_eq!(winners.iter().map(|w| w.who).collect::<Vec<BalanceOf<Test>>>(), vec![10, 30]);