AuctionId bound check. (#229)

* AuctionId bound check.

* Remove unused code.

* Apply review suggestions; fmt.

auction/src/lib.rs

@@ -17,9 +17,10 @@ use frame_support::{
 use frame_system::{self as system, ensure_signed};
 use orml_traits::{Auction, AuctionHandler, AuctionInfo, Change};
 use sp_runtime::{
-	traits::{AtLeast32Bit, MaybeSerializeDeserialize, Member, One, Zero},
-	DispatchResult,
+	traits::{AtLeast32Bit, Bounded, MaybeSerializeDeserialize, Member, One, Zero},
+	DispatchError, DispatchResult,
 };
+use sp_std::result;
 
 mod mock;
 mod tests;
@@ -31,7 +32,7 @@ pub trait Trait: frame_system::Trait {
 	type Balance: Parameter + Member + AtLeast32Bit + Default + Copy + MaybeSerializeDeserialize;
 
 	/// The auction ID type
-	type AuctionId: Parameter + Member + AtLeast32Bit + Default + Copy + MaybeSerializeDeserialize;
+	type AuctionId: Parameter + Member + AtLeast32Bit + Default + Copy + MaybeSerializeDeserialize + Bounded;
 
 	/// The `AuctionHandler` that allow custom bidding logic and handles auction result
 	type Handler: AuctionHandler<Self::AccountId, Self::Balance, Self::BlockNumber, Self::AuctionId>;
@@ -144,6 +145,7 @@ decl_error! {
 		AuctionNotStarted,
 		BidNotAccepted,
 		InvalidBidPrice,
+		NoAvailableAuctionId,
 	}
 }
 
@@ -180,16 +182,23 @@ impl<T: Trait> Auction<T::AccountId, T::BlockNumber> for Module<T> {
 		Ok(())
 	}
 
-	fn new_auction(start: T::BlockNumber, end: Option<T::BlockNumber>) -> Self::AuctionId {
+	fn new_auction(
+		start: T::BlockNumber,
+		end: Option<T::BlockNumber>,
+	) -> result::Result<Self::AuctionId, DispatchError> {
 		let auction = AuctionInfo { bid: None, start, end };
-		let auction_id = Self::auctions_index();
-		<AuctionsIndex<T>>::mutate(|n| *n += Self::AuctionId::one());
+		let auction_id = <AuctionsIndex<T>>::try_mutate(|n| -> result::Result<Self::AuctionId, DispatchError> {
+			let id = *n;
+			ensure!(id != Self::AuctionId::max_value(), Error::<T>::NoAvailableAuctionId);
+			*n += One::one();
+			Ok(id)
+		})?;
 		<Auctions<T>>::insert(auction_id, auction);
 		if let Some(end_block) = end {
 			<AuctionEndTime<T>>::insert(&end_block, auction_id, ());
 		}
 
-		auction_id
+		Ok(auction_id)
 	}
 
 	fn remove_auction(id: Self::AuctionId) {

auction/src/tests.rs

@@ -3,20 +3,20 @@
 #![cfg(test)]
 
 use super::*;
-use frame_support::{assert_ok, traits::OnFinalize};
-use mock::{AuctionModule, ExtBuilder, Runtime, ALICE};
+use frame_support::{assert_noop, assert_ok, traits::OnFinalize};
+use mock::*;
 
 #[test]
 fn new_auction_should_work() {
 	ExtBuilder::default().build().execute_with(|| {
-		assert_eq!(AuctionModule::new_auction(10, Some(100)), 0);
+		assert_ok!(AuctionModule::new_auction(10, Some(100)), 0);
 	});
 }
 
 #[test]
 fn update_auction_should_work() {
 	ExtBuilder::default().build().execute_with(|| {
-		assert_eq!(AuctionModule::new_auction(10, Some(100)), 0);
+		assert_ok!(AuctionModule::new_auction(10, Some(100)), 0);
 		assert_ok!(AuctionModule::update_auction(
 			0,
 			AuctionInfo {
@@ -31,7 +31,7 @@ fn update_auction_should_work() {
 #[test]
 fn auction_info_should_work() {
 	ExtBuilder::default().build().execute_with(|| {
-		assert_eq!(AuctionModule::new_auction(10, Some(100)), 0);
+		assert_ok!(AuctionModule::new_auction(10, Some(100)), 0);
 		assert_eq!(
 			AuctionModule::auction_info(0),
 			Some(AuctionInfo {
@@ -46,7 +46,7 @@ fn auction_info_should_work() {
 #[test]
 fn bid_should_work() {
 	ExtBuilder::default().build().execute_with(|| {
-		assert_eq!(AuctionModule::new_auction(0, Some(100)), 0);
+		assert_ok!(AuctionModule::new_auction(0, Some(100)), 0);
 		assert_ok!(AuctionModule::bid(Some(ALICE).into(), 0, 20));
 		assert_eq!(
 			AuctionModule::auction_info(0),
@@ -62,7 +62,7 @@ fn bid_should_work() {
 #[test]
 fn bid_should_fail() {
 	ExtBuilder::default().build().execute_with(|| {
-		assert_eq!(AuctionModule::new_auction(10, Some(100)), 0);
+		assert_ok!(AuctionModule::new_auction(10, Some(100)), 0);
 		assert_eq!(
 			AuctionModule::bid(Some(ALICE).into(), 0, 20),
 			Err(Error::<Runtime>::AuctionNotStarted.into())
@@ -73,7 +73,7 @@ fn bid_should_fail() {
 #[test]
 fn remove_auction_should_work() {
 	ExtBuilder::default().build().execute_with(|| {
-		assert_eq!(AuctionModule::new_auction(10, Some(100)), 0);
+		assert_ok!(AuctionModule::new_auction(10, Some(100)), 0);
 		assert_eq!(AuctionModule::auctions_index(), 1);
 		assert_eq!(AuctionModule::auctions(0).is_some(), true);
 		assert_eq!(AuctionModule::auction_end_time(100, 0), Some(()));
@@ -86,9 +86,9 @@ fn remove_auction_should_work() {
 #[test]
 fn cleanup_auction_should_work() {
 	ExtBuilder::default().build().execute_with(|| {
-		assert_eq!(AuctionModule::new_auction(10, Some(100)), 0);
+		assert_ok!(AuctionModule::new_auction(10, Some(100)), 0);
 		assert_eq!(AuctionModule::auctions_index(), 1);
-		assert_eq!(AuctionModule::new_auction(10, Some(50)), 1);
+		assert_ok!(AuctionModule::new_auction(10, Some(50)), 1);
 		assert_eq!(AuctionModule::auctions_index(), 2);
 		assert_eq!(AuctionModule::auctions(0).is_some(), true);
 		assert_eq!(AuctionModule::auctions(1).is_some(), true);
@@ -112,3 +112,14 @@ fn cleanup_auction_should_work() {
 		assert_eq!(<AuctionEndTime<Runtime>>::iter_prefix(100).count(), 0);
 	});
 }
+
+#[test]
+fn cannot_add_new_auction_when_no_available_id() {
+	ExtBuilder::default().build().execute_with(|| {
+		<AuctionsIndex<Runtime>>::put(AuctionId::max_value());
+		assert_noop!(
+			AuctionModule::new_auction(0, None),
+			Error::<Runtime>::NoAvailableAuctionId
+		);
+	});
+}

traits/src/auction.rs

@@ -2,12 +2,13 @@ use crate::Change;
 use codec::FullCodec;
 use codec::{Decode, Encode};
 use sp_runtime::{
-	traits::{AtLeast32Bit, MaybeSerializeDeserialize},
-	DispatchResult, RuntimeDebug,
+	traits::{AtLeast32Bit, Bounded, MaybeSerializeDeserialize},
+	DispatchError, DispatchResult, RuntimeDebug,
 };
 use sp_std::{
 	cmp::{Eq, PartialEq},
 	fmt::Debug,
+	result,
 };
 
 /// Auction info.
@@ -25,7 +26,7 @@ pub struct AuctionInfo<AccountId, Balance, BlockNumber> {
 /// Abstraction over a simple auction system.
 pub trait Auction<AccountId, BlockNumber> {
 	/// The id of an AuctionInfo
-	type AuctionId: FullCodec + Default + Copy + Eq + PartialEq + MaybeSerializeDeserialize + Debug;
+	type AuctionId: FullCodec + Default + Copy + Eq + PartialEq + MaybeSerializeDeserialize + Bounded + Debug;
 	/// The price to bid.
 	type Balance: AtLeast32Bit + FullCodec + Copy + MaybeSerializeDeserialize + Debug + Default;
 
@@ -34,7 +35,7 @@ pub trait Auction<AccountId, BlockNumber> {
 	/// Update the auction info of `id` with `info`
 	fn update_auction(id: Self::AuctionId, info: AuctionInfo<AccountId, Self::Balance, BlockNumber>) -> DispatchResult;
 	/// Create new auction with specific startblock and endblock, return the id of the auction
-	fn new_auction(start: BlockNumber, end: Option<BlockNumber>) -> Self::AuctionId;
+	fn new_auction(start: BlockNumber, end: Option<BlockNumber>) -> result::Result<Self::AuctionId, DispatchError>;
 	/// Remove auction by `id`
 	fn remove_auction(id: Self::AuctionId);
 }