From 03c7e58bf453b91683ae911fa7bb8e04bc16d9a6 Mon Sep 17 00:00:00 2001
From: Shaopeng Wang <spxwang@gmail.com>
Date: Thu, 9 Jul 2020 14:34:35 +1200
Subject: [PATCH] AuctionId bound check. (#229)
* AuctionId bound check.
* Remove unused code.
* Apply review suggestions; fmt.
---
auction/src/lib.rs | 23 ++++++++++++++++-------
auction/src/tests.rs | 31 +++++++++++++++++++++----------
traits/src/auction.rs | 9 +++++----
3 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/auction/src/lib.rs b/auction/src/lib.rs
index 9baa698..773db9f 100644
--- a/auction/src/lib.rs
+++ b/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) {
diff --git a/auction/src/tests.rs b/auction/src/tests.rs
index 7076dbd..5c071e5 100644
--- a/auction/src/tests.rs
+++ b/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
+ );
+ });
+}
diff --git a/traits/src/auction.rs b/traits/src/auction.rs
index e1a02e6..9fbaba4 100644
--- a/traits/src/auction.rs
+++ b/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);
}
--
GitLab