diff --git a/frame/society/src/lib.rs b/frame/society/src/lib.rs index 005746aeb4937..27051e4caf8d3 100644 --- a/frame/society/src/lib.rs +++ b/frame/society/src/lib.rs @@ -1108,7 +1108,36 @@ impl, I: Instance> Module { 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, I: Instance> Module { // 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 >::insert(&candidate, (value, kind)); @@ -1299,8 +1328,8 @@ impl, I: Instance> Module { // 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, I: Instance> Module { { let max_members = MaxMembers::::get() as usize; // No more than 10 will be returned. - let 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 = >::get(); - let taken = bids.iter() - .scan(>::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(); - - // No need to reset Bids if we're not taking anything. - if taken > 0 { - >::put(&bids[taken..]); + let mut max_selections: usize = 10.min(max_members.saturating_sub(members_len)); + + if max_selections > 0 { + // Get the number of left-most bidders whose bids add up to less than `pot`. + let mut bids = >::get(); + + // 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 = 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 { + >::put(bids); + } + } + selected + } else { + vec![] } - bids.truncate(taken); - bids } } diff --git a/frame/society/src/tests.rs b/frame/society/src/tests.rs index 61bb1fd232cc0..7d3ee06a2abeb 100644 --- a/frame/society/src/tests.rs +++ b/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!(>::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!(>::get(), final_list); + }); +}