From dae31f2018593b60dbf1d96ec96cdc35c374bb9e Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 8 Jun 2021 16:20:53 +0200 Subject: [PATCH] fix tests --- .../src/benchmarking.rs | 24 ++++---- .../election-provider-multi-phase/src/lib.rs | 6 ++ .../src/signed.rs | 58 ++++++++++--------- 3 files changed, 48 insertions(+), 40 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index d6d53ff97e6b2..7988163e98f65 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -273,20 +273,16 @@ frame_benchmarking::benchmarks! { MultiPhase::::on_initialize_open_signed().expect("should be ok to start signed phase"); >::put(1); - >::mutate(|outer_queue| { - let mut queue = sp_std::mem::take(outer_queue); - queue = queue.try_mutate(|queue| { - for i in 0..c { - let solution = RawSolution { - score: [(10_000_000 + i).into(), 0, 0], - ..Default::default() - }; - let signed_submission = SignedSubmission { solution, ..Default::default() }; - queue.insert(signed_submission); - } - }).unwrap(); - *outer_queue = queue; - }); + let mut signed_submissions = SignedSubmissions::::get(); + for i in 0..c { + let solution = RawSolution { + score: [(10_000_000 + i).into(), 0, 0], + ..Default::default() + }; + let signed_submission = SignedSubmission { solution, ..Default::default() }; + signed_submissions.insert(signed_submission); + } + signed_submissions.put(); let caller = frame_benchmarking::whitelisted_caller(); T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance() * 10u32.into()); diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index a05d8af7d56c9..ba26c50c3f163 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -888,6 +888,12 @@ pub mod pallet { let (maybe_deposit, ejected_a_solution) = Self::insert_submission(&who, &mut signed_submissions, solution, size); + // it's an error if we neither inserted nor removed any submissions + ensure!( + (None, false) != (maybe_deposit, ejected_a_solution), + Error::::SignedQueueFull, + ); + signed_submissions.put(); if let Some(deposit_amount) = maybe_deposit { // collect deposit. Thereafter, the function cannot fail. diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 1298554729505..0855e91b5ba17 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -26,6 +26,7 @@ use codec::{Encode, Decode, HasCompact}; use frame_support::{ storage::bounded_btree_map::BoundedBTreeMap, traits::{Currency, Get, OnUnbalanced, ReservableCurrency}, + DebugNoBound, }; use sp_arithmetic::traits::SaturatedConversion; use sp_npos_elections::{is_score_better, CompactSolution, ElectionScore}; @@ -99,9 +100,25 @@ pub type SubmissionIndicesOf = /// Mask type which pretends to be a set of `SignedSubmissionOf`, while in fact delegating to the /// actual implementations in `SignedSubmissionIndices`, `SignedSubmissionsMap`, and /// `SignedSubmissionNextIndex`. +#[derive(DebugNoBound)] pub struct SignedSubmissions(SubmissionIndicesOf); impl SignedSubmissions { + /// Get the signed submissions from storage. + pub fn get() -> Self { + SignedSubmissions(SignedSubmissionIndices::::get()) + } + + /// Put the signed submissions back into storage. + pub fn put(self) { + SignedSubmissionIndices::::put(self.0) + } + + /// Iterate through the set of signed submissions in order of increasing score. + pub fn iter(&self) -> impl '_ + Iterator> { + self.0.iter().map(|(_score, idx)| SignedSubmissionsMap::::get(idx)) + } + /// Empty the set of signed submissions, returning an iterator of signed submissions pub fn drain(&mut self) -> impl Iterator> { self.0.clear(); @@ -183,14 +200,6 @@ impl SignedSubmissions { } } -// ensure that we update the storage once we're done with this wrapper struct. -impl Drop for SignedSubmissions { - fn drop(&mut self) { - let indices = sp_std::mem::take(&mut self.0); - SignedSubmissionIndices::::put(indices); - } -} - impl Deref for SignedSubmissions { type Target = SubmissionIndicesOf; @@ -202,7 +211,7 @@ impl Deref for SignedSubmissions { impl Pallet { /// `Self` accessor for `SignedSubmission`. pub fn signed_submissions() -> SignedSubmissions { - SignedSubmissions(SignedSubmissionIndices::::get()) + SignedSubmissions::::get() } /// Finish the signed phase. Process the signed submissions from best to worse until a valid one @@ -264,6 +273,8 @@ impl Pallet { debug_assert!(_remaining.is_zero()); } + all_submissions.put(); + (found_solution, weight) } @@ -542,11 +553,10 @@ mod tests { assert_eq!( MultiPhase::signed_submissions() - .into_iter() - .rev() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![9, 8, 7, 6, 5] + vec![5, 6, 7, 8, 9] ); // better. @@ -556,11 +566,10 @@ mod tests { // the one with score 5 was rejected, the new one inserted. assert_eq!( MultiPhase::signed_submissions() - .into_iter() - .rev() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![20, 9, 8, 7, 6] + vec![6, 7, 8, 9, 20] ); }) } @@ -582,11 +591,10 @@ mod tests { assert_eq!( MultiPhase::signed_submissions() - .into_iter() - .rev() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![9, 8, 7, 6, 4], + vec![4, 6, 7, 8, 9], ); // better. @@ -596,11 +604,10 @@ mod tests { // the one with score 5 was rejected, the new one inserted. assert_eq!( MultiPhase::signed_submissions() - .into_iter() - .rev() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![9, 8, 7, 6, 5], + vec![5, 6, 7, 8, 9], ); }) } @@ -642,11 +649,10 @@ mod tests { } assert_eq!( MultiPhase::signed_submissions() - .into_iter() - .rev() + .iter() .map(|s| s.solution.score[0]) .collect::>(), - vec![7, 6, 5] + vec![5, 6, 7] ); // 5 is not accepted. This will only cause processing with no benefit. @@ -688,8 +694,8 @@ mod tests { assert_ok!(submit_with_witness(Origin::signed(9999), solution_9999)); assert_eq!( - MultiPhase::signed_submissions().iter().rev().map(|x| x.who).collect::>(), - vec![999, 99, 9999] + MultiPhase::signed_submissions().iter().map(|x| x.who).collect::>(), + vec![9999, 99, 999] ); // _some_ good solution was stored.