Add trivial EnsureFounder verifier to society (#4615)

* Add trivial EnsureFounder verifier to society

* Fix potential panic

* Keep founder account around.

* Cleanups

* Fix.

* Fix tests

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

substrate/frame/society/src/lib.rs

@@ -305,7 +305,7 @@ pub trait Trait<I=DefaultInstance>: system::Trait {
 	type MaxLockDuration: Get<Self::BlockNumber>;
 
 	/// The origin that is allowed to call `found`.
-	type FounderOrigin: EnsureOrigin<Self::Origin>;
+	type FounderSetOrigin: EnsureOrigin<Self::Origin>;
 
 	/// The origin that is allowed to make suspension judgements.
 	type SuspensionJudgementOrigin: EnsureOrigin<Self::Origin>;
@@ -400,6 +400,10 @@ impl<AccountId: PartialEq, Balance> BidKind<AccountId, Balance> {
 // This module's storage items.
 decl_storage! {
 	trait Store for Module<T: Trait<I>, I: Instance=DefaultInstance> as Society {
+		/// The first member.
+		pub Founder get(founder) build(|config: &GenesisConfig<T, I>| config.members.first().cloned()):
+			Option<T::AccountId>;
+
 		/// The current set of candidates; bidders that are attempting to become members.
 		pub Candidates get(candidates): Vec<Bid<T::AccountId, BalanceOf<T, I>>>;
 
@@ -796,26 +800,26 @@ decl_module! {
 		/// This is done as a discrete action in order to allow for the
 		/// module to be included into a running chain and can only be done once.
 		///
-		/// The dispatch origin for this call must be from the _FounderOrigin_.
+		/// The dispatch origin for this call must be from the _FounderSetOrigin_.
 		///
 		/// Parameters:
 		/// - `founder` - The first member and head of the newly founded society.
 		///
 		/// # <weight>
-		/// - One storage read to check `Head`. O(1)
+		/// - Two storage mutates to set `Head` and `Founder`. O(1)
 		/// - One storage write to add the first member to society. O(1)
-		/// - One storage write to add new Head. O(1)
 		/// - One event.
 		///
 		/// Total Complexity: O(1)
 		/// # </weight>
 		#[weight = SimpleDispatchInfo::FixedNormal(10_000)]
 		fn found(origin, founder: T::AccountId) {
-			T::FounderOrigin::ensure_origin(origin)?;
+			T::FounderSetOrigin::ensure_origin(origin)?;
 			ensure!(!<Head<T, I>>::exists(), Error::<T, I>::AlreadyFounded);
 			// This should never fail in the context of this function...
 			Self::add_member(&founder)?;
 			<Head<T, I>>::put(&founder);
+			<Founder<T, I>>::put(&founder);
 			Self::deposit_event(RawEvent::Founded(founder));
 		}
 		/// Allow suspension judgement origin to make judgement on a suspended member.
@@ -1010,33 +1014,35 @@ decl_error! {
 	pub enum Error for Module<T: Trait<I>, I: Instance> {
 		/// An incorrect position was provided.
 		BadPosition,
-		/// User is not a member
+		/// User is not a member.
 		NotMember,
-		/// User is already a member
+		/// User is already a member.
 		AlreadyMember,
-		/// User is suspended
+		/// User is suspended.
 		Suspended,
-		/// User is not suspended
+		/// User is not suspended.
 		NotSuspended,
-		/// Nothing to payout
+		/// Nothing to payout.
 		NoPayout,
-		/// Society already founded
+		/// Society already founded.
 		AlreadyFounded,
-		/// Not enough in pot to accept candidate
+		/// Not enough in pot to accept candidate.
 		InsufficientPot,
-		/// Member is already vouching or banned from vouching again
+		/// Member is already vouching or banned from vouching again.
 		AlreadyVouching,
-		/// Member is not vouching
+		/// Member is not vouching.
 		NotVouching,
-		/// Cannot remove head
+		/// Cannot remove the head of the chain.
 		Head,
-		/// User has already made a bid
+		/// Cannot remove the founder.
+		Founder,
+		/// User has already made a bid.
 		AlreadyBid,
-		/// User is already a candidate
+		/// User is already a candidate.
 		AlreadyCandidate,
-		/// User is not a candidate
+		/// User is not a candidate.
 		NotCandidate,
-		/// Too many members in the society
+		/// Too many members in the society.
 		MaxMembers,
 	}
 }
@@ -1081,6 +1087,18 @@ decl_event! {
 	}
 }
 
+/// Simple ensure origin struct to filter for the founder account.
+pub struct EnsureFounder<T>(sp_std::marker::PhantomData<T>);
+impl<T: Trait> EnsureOrigin<T::Origin> for EnsureFounder<T> {
+	type Success = T::AccountId;
+	fn try_origin(o: T::Origin) -> Result<Self::Success, T::Origin> {
+		o.into().and_then(|o| match (o, Founder::<T>::get()) {
+			(system::RawOrigin::Signed(ref who), Some(ref f)) if who == f => Ok(who.clone()),
+			(r, _) => Err(T::Origin::from(r)),
+		})
+	}
+}
+
 /// Pick an item at pseudo-random from the slice, given the `rng`. `None` iff the slice is empty.
 fn pick_item<'a, R: RngCore, T>(rng: &mut R, items: &'a [T]) -> Option<&'a T> {
 	if items.is_empty() {
@@ -1201,6 +1219,7 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 	/// removes them from the Members storage item.
 	pub fn remove_member(m: &T::AccountId) -> DispatchResult {
 		ensure!(Self::head() != Some(m.clone()), Error::<T, I>::Head);
+		ensure!(Self::founder() != Some(m.clone()), Error::<T, I>::Founder);
 
 		<Members<T, I>>::mutate(|members|
 			match members.binary_search(&m) {
@@ -1509,8 +1528,10 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 	/// the number of bids would not surpass `MaxMembers` if all were accepted.
 	///
 	/// May be empty.
-	pub fn take_selected(members_len: usize, pot: BalanceOf<T, I>) -> Vec<Bid<T::AccountId, BalanceOf<T, I>>>
+	pub fn take_selected(
-	{
+		members_len: usize,
+		pot: BalanceOf<T, I>
+	) -> Vec<Bid<T::AccountId, BalanceOf<T, I>>> {
 		let max_members = MaxMembers::<I>::get() as usize;
 		// No more than 10 will be returned.
 		let mut max_selections: usize = 10.min(max_members.saturating_sub(members_len));

substrate/frame/society/src/mock.rs

@@ -104,7 +104,7 @@ impl Trait for Test {
 	type MembershipChanged = ();
 	type RotationPeriod = RotationPeriod;
 	type MaxLockDuration = MaxLockDuration;
-	type FounderOrigin = EnsureSignedBy<FounderSetAccount, u128>;
+	type FounderSetOrigin = EnsureSignedBy<FounderSetAccount, u128>;
 	type SuspensionJudgementOrigin = EnsureSignedBy<SuspensionJudgementSetAccount, u128>;
 	type ChallengePeriod = ChallengePeriod;
 }
@@ -133,6 +133,9 @@ impl EnvBuilder {
 				(40, 50),
 				(50, 50),
 				(60, 50),
+				(70, 50),
+				(80, 50),
+				(90, 50),
 			],
 			pot: 0,
 			max_members: 100,

substrate/frame/society/src/tests.rs

@@ -25,6 +25,8 @@ use sp_runtime::traits::BadOrigin;
 #[test]
 fn founding_works() {
 	EnvBuilder::new().with_members(vec![]).execute(|| {
+		// No founder initially.
+		assert_eq!(Society::founder(), None);
 		// Account 1 is set as the founder origin
 		// Account 5 cannot start a society
 		assert_noop!(Society::found(Origin::signed(5), 20), BadOrigin);
@@ -34,6 +36,8 @@ fn founding_works() {
 		assert_eq!(Society::members(), vec![10]);
 		// 10 is the head of the society
 		assert_eq!(Society::head(), Some(10));
+		// ...and also the founder
+		assert_eq!(Society::founder(), Some(10));
 		// Cannot start another society
 		assert_noop!(Society::found(Origin::signed(1), 20), Error::<Test, _>::AlreadyFounded);
 	});
@@ -460,10 +464,11 @@ fn unbid_vouch_works() {
 }
 
 #[test]
-fn head_cannot_be_removed() {
+fn founder_and_head_cannot_be_removed() {
 	EnvBuilder::new().execute(|| {
-		// 10 is the only member and head
+		// 10 is the only member, founder, and head
 		assert_eq!(Society::members(), vec![10]);
+		assert_eq!(Society::founder(), Some(10));
 		assert_eq!(Society::head(), Some(10));
 		// 10 can still accumulate strikes
 		assert_ok!(Society::bid(Origin::signed(20), 0));
@@ -485,16 +490,37 @@ fn head_cannot_be_removed() {
 		run_to_block(32);
 		assert_eq!(Society::members(), vec![10, 50]);
 		assert_eq!(Society::head(), Some(50));
+		// Founder is unchanged
+		assert_eq!(Society::founder(), Some(10));
 
-		// 10 can now be suspended for strikes
+		// 50 can still accumulate strikes
 		assert_ok!(Society::bid(Origin::signed(60), 0));
-		run_to_block(36);
-		// The candidate is rejected, so voting approve will give a strike
-		assert_ok!(Society::vote(Origin::signed(10), 60, true));
 		run_to_block(40);
-		assert_eq!(Strikes::<Test>::get(10), 0);
+		assert_eq!(Strikes::<Test>::get(50), 1);
-		assert_eq!(<SuspendedMembers<Test>>::get(10), Some(()));
+		assert_ok!(Society::bid(Origin::signed(70), 0));
-		assert_eq!(Society::members(), vec![50]);
+		run_to_block(48);
+		assert_eq!(Strikes::<Test>::get(50), 2);
+
+		// Replace the head
+		assert_ok!(Society::bid(Origin::signed(80), 0));
+		run_to_block(52);
+		assert_ok!(Society::vote(Origin::signed(10), 80, true));
+		assert_ok!(Society::vote(Origin::signed(50), 80, true));
+		assert_ok!(Society::defender_vote(Origin::signed(10), true)); // Keep defender around
+		run_to_block(56);
+		assert_eq!(Society::members(), vec![10, 50, 80]);
+		assert_eq!(Society::head(), Some(80));
+		assert_eq!(Society::founder(), Some(10));
+
+		// 50 can now be suspended for strikes
+		assert_ok!(Society::bid(Origin::signed(90), 0));
+		run_to_block(60);
+		// The candidate is rejected, so voting approve will give a strike
+		assert_ok!(Society::vote(Origin::signed(50), 90, true));
+		run_to_block(64);
+		assert_eq!(Strikes::<Test>::get(50), 0);
+		assert_eq!(<SuspendedMembers<Test>>::get(50), Some(()));
+		assert_eq!(Society::members(), vec![10, 80]);
 	});
 }