Additional zero bid logic for Society pallet (#4604)

* Select only one zero bid, better ordering of new bids

* Select zero bid as head

* Update comment

* Update frame/society/src/tests.rs

* Add new test, logic not updated yet

* Implement `put_bid` to order same value bids

Note that this extra logic currently does not do anything per the implementation of `binary_search` in Rust.

substrate/frame/society/src/lib.rs

@@ -1108,7 +1108,36 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 		const MAX_BID_COUNT: usize = 1000;
 
 		match bids.binary_search_by(|bid| bid.value.cmp(&value)) {
-			Ok(pos) | Err(pos) => bids.insert(pos, Bid {
+			// Insert new elements after the existing ones. This ensures new bids
+			// with the same bid value are further down the list than existing ones.
+			Ok(pos) => {
+				let different_bid = bids.iter()
+					// Easily extract the index we are on
+					.enumerate()
+					// Skip ahead to the suggested position
+					.skip(pos)
+					// Keep skipping ahead until the position changes
+					.skip_while(|(_, x)| x.value <= bids[pos].value)
+					// Get the element when things changed
+					.next();
+				// If the element is not at the end of the list, insert the new element
+				// in the spot.
+				if let Some((p, _)) = different_bid {
+					bids.insert(p, Bid {
+						value,
+						who: who.clone(),
+						kind: bid_kind,
+					});
+				// If the element is at the end of the list, push the element on the end.
+				} else {
+					bids.push(Bid {
+						value,
+						who: who.clone(),
+						kind: bid_kind,
+					});
+				}
+			},
+			Err(pos) => bids.insert(pos, Bid {
 				value,
 				who: who.clone(),
 				kind: bid_kind,
@@ -1264,7 +1293,7 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 					// We track here the total_approvals so that every candidate has a unique range
 					// of numbers from 0 to `total_approvals` with length `approval_count` so each
 					// candidate is proportionally represented when selecting a "primary" below.
-					Some((candidate, total_approvals))
+					Some((candidate, total_approvals, value))
 				} else {
 					// Suspend Candidate
 					<SuspendedCandidates<T, I>>::insert(&candidate, (value, kind));
@@ -1299,8 +1328,8 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 				// select one as primary, randomly chosen from the accepted, weighted by approvals. 
 				// Choose a random number between 0 and `total_approvals`
 				let primary_point = pick_usize(&mut rng, total_approvals - 1);
-				// Find the user who falls on that point
-				let primary = accepted.iter().find(|e| e.1 > primary_point)
+				// Find the zero bid or the user who falls on that point
+				let primary = accepted.iter().find(|e| e.2.is_zero() || e.1 > primary_point)
 					.expect("e.1 of final item == total_approvals; \
 						worst case find will always return that item; qed")
 					.0.clone();
@@ -1484,24 +1513,57 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 	{
 		let max_members = MaxMembers::<I>::get() as usize;
 		// No more than 10 will be returned.
-		let max_selections: usize = 10.min(max_members.saturating_sub(members_len));
+		let mut max_selections: usize = 10.min(max_members.saturating_sub(members_len));
 
-		// Get the number of left-most bidders whose bids add up to less than `pot`.
-		let mut bids = <Bids<T, I>>::get();
-		let taken = bids.iter()
-			.scan(<BalanceOf<T, I>>::zero(), |total, bid| {
-				*total = total.saturating_add(bid.value);
-				Some((*total, bid.who.clone(), bid.kind.clone()))
-			})
-			.take(max_selections)
-			.take_while(|x| pot >= x.0)
-			.count();
+		if max_selections > 0 {
+			// Get the number of left-most bidders whose bids add up to less than `pot`.
+			let mut bids = <Bids<T, I>>::get();
 
-		// No need to reset Bids if we're not taking anything.
-		if taken > 0 {
-			<Bids<T, I>>::put(&bids[taken..]);
+			// The list of selected candidates
+			let mut selected = Vec::new();
+			
+			if bids.len() > 0 {
+				// Can only select at most the length of bids
+				max_selections = max_selections.min(bids.len());
+				// Number of selected bids so far
+				let mut count = 0;
+				// Check if we have already selected a candidate with zero bid
+				let mut zero_selected = false;
+				// A running total of the cost to onboard these bids
+				let mut total_cost: BalanceOf<T, I> = Zero::zero();
+
+				bids.retain(|bid| {
+					if count < max_selections {
+						// Handle zero bids. We only want one of them.
+						if bid.value.is_zero() {
+							// Select only the first zero bid
+							if !zero_selected {
+								selected.push(bid.clone());
+								zero_selected = true;
+								count += 1;
+								return false
+							}
+						} else {
+							total_cost += bid.value;
+							// Select only as many users as the pot can support.
+							if total_cost <= pot {
+								selected.push(bid.clone());
+								count += 1;
+								return false
+							}
+						}
+					}
+					true
+				});
+
+				// No need to reset Bids if we're not taking anything.
+				if count > 0 {
+					<Bids<T, I>>::put(bids);
+				}
+			}
+			selected
+		} else {
+			vec![]
 		}
-		bids.truncate(taken);
-		bids
 	}
 }

substrate/frame/society/src/tests.rs

@@ -742,3 +742,68 @@ fn max_limits_work() {
 		assert_eq!(Society::candidates().len(), 10);
 	});
 }
+
+#[test]
+fn zero_bid_works() {
+	// This tests:
+	// * Only one zero bid is selected.
+	// * That zero bid is placed as head when accepted.
+	EnvBuilder::new().execute(|| {
+		// Users make bids of various amounts
+		assert_ok!(Society::bid(Origin::signed(60), 400));
+		assert_ok!(Society::bid(Origin::signed(50), 300));
+		assert_ok!(Society::bid(Origin::signed(30), 0));
+		assert_ok!(Society::bid(Origin::signed(20), 0));
+		assert_ok!(Society::bid(Origin::signed(40), 0));
+
+		// Rotate period
+		run_to_block(4);
+		// Pot is 1000 after "PeriodSpend"
+		assert_eq!(Society::pot(), 1000);
+		assert_eq!(Balances::free_balance(Society::account_id()), 10_000);
+		// Choose smallest bidding users whose total is less than pot, with only one zero bid.
+		assert_eq!(Society::candidates(), vec![
+			create_bid(0, 30, BidKind::Deposit(25)),
+			create_bid(300, 50, BidKind::Deposit(25)),
+			create_bid(400, 60, BidKind::Deposit(25)),
+		]);
+		assert_eq!(<Bids<Test>>::get(), vec![
+			create_bid(0, 20, BidKind::Deposit(25)),
+			create_bid(0, 40, BidKind::Deposit(25)),
+		]);
+		// A member votes for these candidates to join the society
+		assert_ok!(Society::vote(Origin::signed(10), 30, true));
+		assert_ok!(Society::vote(Origin::signed(10), 50, true));
+		assert_ok!(Society::vote(Origin::signed(10), 60, true));
+		run_to_block(8);
+		// Candidates become members after a period rotation
+		assert_eq!(Society::members(), vec![10, 30, 50, 60]);
+		// The zero bid is selected as head
+		assert_eq!(Society::head(), Some(30));
+	});
+}
+
+#[test]
+fn bids_ordered_correctly() {
+	// This tests that bids with the same value are placed in the list ordered
+	// with bidders who bid first earlier on the list.
+	EnvBuilder::new().execute(|| {
+		for i in 0..5 {
+			for j in 0..5 {
+				// Give them some funds
+				let _ = Balances::make_free_balance_be(&(100 + (i * 5 + j) as u128), 1000);
+				assert_ok!(Society::bid(Origin::signed(100 + (i * 5 + j) as u128), j));
+			}
+		}
+
+		let mut final_list = Vec::new();
+
+		for j in 0..5 {
+			for i in 0..5 {
+				final_list.push(create_bid(j, 100 + (i * 5 + j) as u128,  BidKind::Deposit(25)));
+			}
+		}
+
+		assert_eq!(<Bids<Test>>::get(), final_list);
+	});
+}