Fix Crowdloan Dissolve and Add Auction Cancel (#2665)

* Check fund depositor calls dissolve

* add auction cancel too

* use drain api rather than `iter` + `take`

* add test and benchmarks

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=auctions --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

Co-authored-by: Parity Benchmarking Bot <admin@parity.io>

runtime/common/src/auctions.rs

@@ -26,7 +26,7 @@ use frame_support::{
 	weights::{DispatchClass, Weight},
 };
 use primitives::v1::Id as ParaId;
-use frame_system::ensure_signed;
+use frame_system::{ensure_signed, ensure_root};
 use crate::slot_range::{SlotRange, SLOT_RANGE_COUNT};
 use crate::traits::{Leaser, LeaseError, Auctioneer};
 use parity_scale_codec::Decode;
@@ -37,6 +37,7 @@ type BalanceOf<T> = <<<T as Config>::Leaser as Leaser>::Currency as Currency<<T
 pub trait WeightInfo {
 	fn new_auction() -> Weight;
 	fn bid() -> Weight;
+	fn cancel_auction() -> Weight;
 	fn on_initialize() -> Weight;
 }
 
@@ -44,6 +45,7 @@ pub struct TestWeightInfo;
 impl WeightInfo for TestWeightInfo {
 	fn new_auction() -> Weight { 0 }
 	fn bid() -> Weight { 0 }
+	fn cancel_auction() -> Weight { 0 }
 	fn on_initialize() -> Weight { 0 }
 }
 
@@ -264,6 +266,20 @@ decl_module! {
 			let who = ensure_signed(origin)?;
 			Self::handle_bid(who, para, auction_index, first_slot, last_slot, amount)?;
 		}
+
+		/// Cancel an in-progress auction.
+		///
+		/// Can only be called by Root origin.
+		#[weight = T::WeightInfo::cancel_auction()]
+		pub fn cancel_auction(origin) {
+			ensure_root(origin)?;
+			// Unreserve all bids.
+			for ((bidder, _), amount) in ReservedAmounts::<T>::drain() {
+				CurrencyOf::<T>::unreserve(&bidder, amount);
+			}
+			Winning::<T>::remove_all();
+			AuctionInfo::<T>::kill();
+		}
 	}
 }
 
@@ -490,8 +506,7 @@ impl<T: Config> Module<T> {
 		// First, unreserve all amounts that were reserved for the bids. We will later re-reserve the
 		// amounts from the bidders that ended up being assigned the slot so there's no need to
 		// special-case them here.
-		for ((bidder, para), amount) in ReservedAmounts::<T>::iter() {
-			ReservedAmounts::<T>::take((bidder.clone(), para));
+		for ((bidder, _), amount) in ReservedAmounts::<T>::drain() {
 			CurrencyOf::<T>::unreserve(&bidder, amount);
 		}
 
@@ -1311,6 +1326,25 @@ mod tests {
 			]));
 		});
 	}
+
+	#[test]
+	fn can_cancel_auction() {
+		new_test_ext().execute_with(|| {
+			run_to_block(1);
+			assert_ok!(Auctions::new_auction(Origin::signed(6), 5, 1));
+			assert_ok!(Auctions::bid(Origin::signed(1), 0.into(), 1, 1, 4, 1));
+			assert_eq!(Balances::reserved_balance(1), 1);
+			assert_eq!(Balances::free_balance(1), 9);
+
+			assert_noop!(Auctions::cancel_auction(Origin::signed(6)), BadOrigin);
+			assert_ok!(Auctions::cancel_auction(Origin::root()));
+
+			assert!(AuctionInfo::<Test>::get().is_none());
+			assert_eq!(Balances::reserved_balance(1), 0);
+			assert_eq!(ReservedAmounts::<Test>::iter().count(), 0);
+			assert_eq!(Winning::<Test>::iter().count(), 0);
+		});
+	}
 }
 
 #[cfg(feature = "runtime-benchmarks")]
@@ -1318,7 +1352,7 @@ mod benchmarking {
 	use super::{*, Module as Auctions};
 	use frame_system::RawOrigin;
 	use frame_support::traits::OnInitialize;
-	use sp_runtime::traits::Bounded;
+	use sp_runtime::{traits::Bounded, SaturatedConversion};
 
 	use frame_benchmarking::{benchmarks, whitelisted_caller, account, impl_benchmark_test_suite};
 
@@ -1330,6 +1364,39 @@ mod benchmarking {
 		assert_eq!(event, &system_event);
 	}
 
+	fn fill_winners<T: Config>(lease_period_index: LeasePeriodOf<T>) {
+		let auction_index = AuctionCounter::get();
+		let minimum_balance = CurrencyOf::<T>::minimum_balance();
+
+		for n in 1 ..= SLOT_RANGE_COUNT as u32 {
+			let bidder = account("bidder", n, 0);
+			CurrencyOf::<T>::make_free_balance_be(&bidder, BalanceOf::<T>::max_value());
+
+			let (start, end) = match n {
+				1  => (0u32, 0u32),
+				2  => (0, 1),
+				3  => (0, 2),
+				4  => (0, 3),
+				5  => (1, 1),
+				6  => (1, 2),
+				7  => (1, 3),
+				8  => (2, 2),
+				9  => (2, 3),
+				10 => (3, 3),
+				_ => panic!("test not meant for this"),
+			};
+
+			assert!(Auctions::<T>::bid(
+				RawOrigin::Signed(bidder).into(),
+				ParaId::from(n),
+				auction_index,
+				lease_period_index + start.into(), // First Slot
+				lease_period_index + end.into(), // Last slot
+				minimum_balance.saturating_mul(n.into()), // Amount
+			).is_ok());
+		}
+	}
+
 	benchmarks! {
 		where_clause { where T: pallet_babe::Config }
 
@@ -1388,37 +1455,8 @@ mod benchmarking {
 			let lease_period_index = LeasePeriodOf::<T>::zero();
 			let now = frame_system::Pallet::<T>::block_number();
 			Auctions::<T>::new_auction(RawOrigin::Root.into(), duration, lease_period_index)?;
-			let auction_index = AuctionCounter::get();
 
-			let minimum_balance = CurrencyOf::<T>::minimum_balance();
-
-			for n in 1 ..= SLOT_RANGE_COUNT as u32 {
-				let bidder = account("bidder", n, 0);
-				CurrencyOf::<T>::make_free_balance_be(&bidder, BalanceOf::<T>::max_value());
-
-				let (start, end) = match n {
-					1  => (0u32, 0u32),
-					2  => (0, 1),
-					3  => (0, 2),
-					4  => (0, 3),
-					5  => (1, 1),
-					6  => (1, 2),
-					7  => (1, 3),
-					8  => (2, 2),
-					9  => (2, 3),
-					10 => (3, 3),
-					_ => panic!("test not meant for this"),
-				};
-
-				Auctions::<T>::bid(
-					RawOrigin::Signed(bidder).into(),
-					ParaId::from(n),
-					auction_index,
-					lease_period_index + start.into(), // First Slot
-					lease_period_index + end.into(), // Last slot
-					minimum_balance.saturating_mul(n.into()), // Amount
-				)?;
-			}
+			fill_winners::<T>(lease_period_index);
 
 			for winner in Winning::<T>::get(T::BlockNumber::from(0u32)).unwrap().iter() {
 				assert!(winner.is_some());
@@ -1438,8 +1476,34 @@ mod benchmarking {
 		}: {
 			Auctions::<T>::on_initialize(duration + now + T::EndingPeriod::get());
 		} verify {
+			let auction_index = AuctionCounter::get();
 			assert_last_event::<T>(RawEvent::AuctionClosed(auction_index).into());
 		}
+
+		// Worst case: 10 bidders taking all wining spots, and winning data is full.
+		cancel_auction {
+			// Create a new auction
+			let duration: T::BlockNumber = 99u32.into();
+			let lease_period_index = LeasePeriodOf::<T>::zero();
+			let now = frame_system::Pallet::<T>::block_number();
+			Auctions::<T>::new_auction(RawOrigin::Root.into(), duration, lease_period_index)?;
+
+			fill_winners::<T>(lease_period_index);
+
+			let winning_data = Winning::<T>::get(T::BlockNumber::from(0u32)).unwrap();
+			for winner in winning_data.iter() {
+				assert!(winner.is_some());
+			}
+
+			// Make winning map full
+			for i in 0u32 .. T::EndingPeriod::get().saturated_into() {
+				Winning::<T>::insert(T::BlockNumber::from(i), winning_data.clone());
+			}
+			assert!(AuctionInfo::<T>::get().is_some());
+		}: _(RawOrigin::Root)
+		verify {
+			assert!(AuctionInfo::<T>::get().is_none());
+		}
 	}
 
 	impl_benchmark_test_suite!(

runtime/common/src/crowdloan.rs

@@ -468,12 +468,15 @@ decl_module! {
 		/// This places any deposits that were not withdrawn into the treasury.
 		#[weight = T::WeightInfo::dissolve(T::RemoveKeysLimit::get())]
 		pub fn dissolve(origin, #[compact] index: ParaId) -> DispatchResultWithPostInfo {
-			ensure_signed(origin)?;
+			let who = ensure_signed(origin)?;
 
 			let fund = Self::funds(index).ok_or(Error::<T>::InvalidParaId)?;
 			let now = frame_system::Pallet::<T>::block_number();
 			let dissolution = fund.end.saturating_add(T::RetirementPeriod::get());
-			ensure!((fund.retiring && now >= dissolution) || fund.raised.is_zero(), Error::<T>::NotReadyToDissolve);
+
+			let can_dissolve = (fund.retiring && now >= dissolution) ||
+				(fund.raised.is_zero() && who == fund.depositor);
+			ensure!(can_dissolve, Error::<T>::NotReadyToDissolve);
 
 			// Try killing the crowdloan child trie
 			match Self::crowdloan_kill(fund.trie_index) {
@@ -1166,7 +1169,7 @@ mod tests {
 		new_test_ext().execute_with(|| {
 			let para = new_para();
 
-			// Set up two crowdloans
+			// Set up a crowdloan
 			assert_ok!(Crowdloan::create(Origin::signed(1), para, 1000, 1, 1, 9, None));
 			// Fund crowdloans.
 			assert_ok!(Crowdloan::contribute(Origin::signed(2), para, 100, None));
@@ -1175,6 +1178,8 @@ mod tests {
 			assert_noop!(Crowdloan::dissolve(Origin::signed(2), para), Error::<Test>::NotReadyToDissolve);
 
 			assert_ok!(Crowdloan::withdraw(Origin::signed(2), 2, para));
+			// Only crowdloan creator can dissolve when raised is zero.
+			assert_noop!(Crowdloan::dissolve(Origin::signed(2), para), Error::<Test>::NotReadyToDissolve);
 			assert_ok!(Crowdloan::dissolve(Origin::signed(1), para));
 			assert_eq!(Balances::free_balance(1), 1000);
 		});

runtime/common/src/integration_tests.rs

@@ -759,7 +759,7 @@ fn basic_swap_works() {
 		assert_eq!(Balances::free_balance(&crowdloan_account), 0);
 
 		// Dissolve returns the balance of the person who put a deposit for crowdloan
-		assert_ok!(Crowdloan::dissolve(Origin::signed(2), ParaId::from(2)));
+		assert_ok!(Crowdloan::dissolve(Origin::signed(1), ParaId::from(2)));
 		assert_eq!(Balances::reserved_balance(&1), 0);
 		assert_eq!(Balances::reserved_balance(&2), 500 + 20 * 2 * 1);
 

runtime/westend/src/weights/auctions.rs

@@ -16,7 +16,7 @@
 //! Autogenerated weights for auctions
 //!
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 3.0.0
-//! DATE: 2021-03-14, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! DATE: 2021-03-23, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("westend-dev"), DB CACHE: 128
 
 // Executed Command:
@@ -44,18 +44,23 @@ use sp_std::marker::PhantomData;
 pub struct WeightInfo<T>(PhantomData<T>);
 impl<T: frame_system::Config> auctions::WeightInfo for WeightInfo<T> {
 	fn new_auction() -> Weight {
-		(24_105_000 as Weight)
+		(24_619_000 as Weight)
 			.saturating_add(T::DbWeight::get().reads(2 as Weight))
 			.saturating_add(T::DbWeight::get().writes(2 as Weight))
 	}
 	fn bid() -> Weight {
-		(110_317_000 as Weight)
+		(113_354_000 as Weight)
 			.saturating_add(T::DbWeight::get().reads(7 as Weight))
 			.saturating_add(T::DbWeight::get().writes(4 as Weight))
 	}
 	fn on_initialize() -> Weight {
-		(602_940_000 as Weight)
-			.saturating_add(T::DbWeight::get().reads(75 as Weight))
-			.saturating_add(T::DbWeight::get().writes(71 as Weight))
+		(3_095_173_000 as Weight)
+			.saturating_add(T::DbWeight::get().reads(625 as Weight))
+			.saturating_add(T::DbWeight::get().writes(621 as Weight))
+	}
+	fn cancel_auction() -> Weight {
+		(841_845_000 as Weight)
+			.saturating_add(T::DbWeight::get().reads(21 as Weight))
+			.saturating_add(T::DbWeight::get().writes(621 as Weight))
 	}
 }