Address feedback

pallets/marketplace/Cargo.toml

@@ -35,10 +35,6 @@ sp-core = { workspace = true, default-features = false }
 pallet-balances = { workspace = true, default-features = false }
 pallet-timestamp = { workspace = true, default-features = false }
 
-[dev-dependencies]
-sp-io = { workspace = true, default-features = false }
-
-
 [features]
 default = ["std"]
 std = [

pallets/marketplace/src/benchmarking.rs

@@ -13,8 +13,8 @@ use frame_support::{
 use pallet_nfts::{CollectionConfig, CollectionSettings, ItemConfig, MintSettings, Pallet as Nfts};
 
 use sp_core::{
+	crypto::Pair,
 	ecdsa::{Pair as EthereumPair, Signature},
-	Pair,
 };
 
 const SEED: u32 = 0;
@@ -85,8 +85,8 @@ pub mod benchmarks {
 	use account::{AccountId20, EthereumSignature, EthereumSigner};
 	use pallet_timestamp::Pallet as Timestamp;
 	use parity_scale_codec::Encode;
-	use scale_info::prelude::vec;
-	use sp_core::keccak_256;
+	use scale_info::prelude::vec::Vec;
+	use sp_core::hashing::keccak_256;
 	use sp_runtime::traits::IdentifyAccount;
 
 	fn create_valid_order<T: Config>(

pallets/marketplace/src/lib.rs

@@ -36,7 +36,7 @@ pub mod pallet {
 
 	use sp_runtime::{
 		traits::{CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, IdentifyAccount, Verify},
-		BoundedVec, DispatchError,
+		BoundedVec, DispatchError, Saturating,
 	};
 	use sp_std::vec::Vec;
 
@@ -104,24 +104,19 @@ pub mod pallet {
 	}
 
 	#[pallet::storage]
-	#[pallet::getter(fn authority)]
 	pub type Authority<T: Config> = StorageValue<_, T::AccountId, OptionQuery>;
 
 	#[pallet::storage]
-	#[pallet::getter(fn fee_signer)]
 	pub type FeeSigner<T: Config> = StorageValue<_, T::AccountId, OptionQuery>;
 
 	#[pallet::storage]
-	#[pallet::getter(fn nonces)]
 	pub type Nonces<T: Config> =
 		StorageMap<_, Identity, BoundedVec<u8, T::NonceStringLimit>, bool, ValueQuery>;
 
 	#[pallet::storage]
-	#[pallet::getter(fn payout_address)]
 	pub type PayoutAddress<T: Config> = StorageValue<_, T::AccountId, OptionQuery>;
 
 	#[pallet::storage]
-	#[pallet::getter(fn asks)]
 	pub type Asks<T: Config> = StorageDoubleMap<
 		_,
 		Blake2_128Concat,
@@ -132,15 +127,14 @@ pub mod pallet {
 	>;
 
 	#[pallet::storage]
-	#[pallet::getter(fn bids)]
 	pub type Bids<T: Config> = StorageNMap<
 		_,
 		(
 			NMapKey<Blake2_128Concat, T::CollectionId>,
 			NMapKey<Blake2_128Concat, T::ItemId>,
 			NMapKey<Blake2_128Concat, BalanceOf<T>>,
 		),
-		Bid<T::AccountId, T::Moment, BalanceOf<T>>,
+		Bid<T::AccountId, BalanceOf<T>, T::Moment>,
 	>;
 
 	#[pallet::event]
@@ -320,7 +314,8 @@ pub mod pallet {
 			ensure!(order.price.clone() >= T::MaxBasisPoints::get(), Error::<T>::InvalidPrice);
 			ensure!(
 				order.expires_at.clone()
-					> pallet_timestamp::Pallet::<T>::get() + T::MinOrderDuration::get(),
+					> pallet_timestamp::Pallet::<T>::get()
+						.saturating_add(T::MinOrderDuration::get()),
 				Error::<T>::InvalidExpiration
 			);
 			ensure!(
@@ -338,7 +333,6 @@ pub mod pallet {
 				order.signature_data.nonce.clone(),
 			);
 			Self::verify_fee_signer_signature(&message.encode(), order.signature_data)?;
-			//--------------------------------------------
 
 			Self::deposit_event(Event::OrderCreated {
 				who: who.clone(),
@@ -360,14 +354,14 @@ pub mod pallet {
 					pallet_nfts::Pallet::<T>::disable_transfer(&order.collection, &order.item)
 						.map_err(|_| Error::<T>::ItemAlreadyLocked)?;
 
-					if Self::valid_match_exists_for(
+					if let Some(exec_order) = Self::valid_match_exists_for(
 						OrderType::Ask,
 						&order.collection,
 						&order.item,
 						&order.price,
 					) {
 						Self::execute_order(
-							OrderType::Ask,
+							exec_order,
 							who,
 							order.collection,
 							order.item,
@@ -407,14 +401,14 @@ pub mod pallet {
 					)
 					.map_err(|_| Error::<T>::InsufficientFunds)?;
 
-					if Self::valid_match_exists_for(
+					if let Some(exec_order) = Self::valid_match_exists_for(
 						OrderType::Bid,
 						&order.collection,
 						&order.item,
 						&order.price,
 					) {
 						Self::execute_order(
-							OrderType::Bid,
+							exec_order,
 							who,
 							order.collection,
 							order.item,
@@ -515,34 +509,35 @@ pub mod pallet {
 			collection: &T::CollectionId,
 			item: &T::ItemId,
 			price: &BalanceOf<T>,
-		) -> bool {
+		) -> Option<ExecOrder<T::AccountId, BalanceOf<T>, T::Moment>> {
 			let timestamp = pallet_timestamp::Pallet::<T>::get();
+
 			match order_type {
 				OrderType::Ask => {
 					if let Some(bid) = Bids::<T>::get((collection, item, price)) {
 						if timestamp >= bid.expiration {
-							return false;
+							return None;
 						};
+						Some(ExecOrder::Bid(bid))
 					} else {
-						return false;
+						return None;
 					}
 				},
 				OrderType::Bid => {
 					if let Some(ask) = Asks::<T>::get(collection.clone(), item.clone()) {
 						if timestamp >= ask.expiration {
-							return false;
+							return None;
 						};
+						Some(ExecOrder::Ask(ask))
 					} else {
-						return false;
+						return None;
 					}
 				},
-			};
-
-			true
+			}
 		}
 
 		pub fn execute_order(
-			order_type: OrderType,
+			exec_order: ExecOrder<T::AccountId, BalanceOf<T>, T::Moment>,
 			who: T::AccountId,
 			collection: T::CollectionId,
 			item: T::ItemId,
@@ -554,28 +549,17 @@ pub mod pallet {
 			let seller_fee: BalanceOf<T>;
 			let buyer_fee: BalanceOf<T>;
 
-			match order_type {
-				OrderType::Ask => {
-					let bid = Bids::<T>::get((collection, item, price))
-						.ok_or(Error::<T>::OrderNotFound)?;
+			match exec_order {
+				ExecOrder::Bid(bid) => {
 					ensure!(who.clone() != bid.buyer.clone(), Error::<T>::BuyerIsSeller);
-					ensure!(
-						bid.expiration >= pallet_timestamp::Pallet::<T>::get(),
-						Error::<T>::OrderExpired
-					);
 
 					seller = who;
 					buyer = bid.buyer;
 					seller_fee = *fee;
 					buyer_fee = bid.fee;
 				},
-				OrderType::Bid => {
-					let ask = Asks::<T>::get(collection, item).ok_or(Error::<T>::OrderNotFound)?;
+				ExecOrder::Ask(ask) => {
 					ensure!(who.clone() != ask.seller.clone(), Error::<T>::BuyerIsSeller);
-					ensure!(
-						ask.expiration >= pallet_timestamp::Pallet::<T>::get(),
-						Error::<T>::OrderExpired
-					);
 
 					seller = ask.seller;
 					buyer = who;

pallets/marketplace/src/tests.rs

@@ -150,7 +150,7 @@ mod force_set_authority {
 	fn force_set_authoity_works() {
 		new_test_ext().execute_with(|| {
 			assert_ok!(Marketplace::force_set_authority(RuntimeOrigin::root(), account(1)));
-			assert!(Marketplace::authority() == Some(account(1)));
+			assert!(Authority::<Test>::get() == Some(account(1)));
 		})
 	}
 
@@ -191,7 +191,7 @@ mod set_fee_signer {
 				RuntimeOrigin::signed(account(1)),
 				account(2)
 			));
-			assert!(Marketplace::fee_signer() == Some(account(2)));
+			assert!(FeeSigner::<Test>::get() == Some(account(2)));
 		})
 	}
 
@@ -245,7 +245,7 @@ mod set_payout_address {
 				RuntimeOrigin::signed(account(1)),
 				account(2)
 			));
-			assert!(Marketplace::payout_address() == Some(account(2)));
+			assert!(PayoutAddress::<Test>::get() == Some(account(2)));
 		})
 	}
 
@@ -604,7 +604,7 @@ mod create_ask {
 				fee: order.fee_percent,
 			};
 
-			assert!(Marketplace::asks(0, 0) == Some(ask));
+			assert!(Asks::<Test>::get(0, 0) == Some(ask));
 			assert!(!Nfts::can_transfer(&0, &0));
 		})
 	}
@@ -720,7 +720,7 @@ mod create_bid {
 						.saturating_sub(initial_reserved)
 				) == Marketplace::calc_bid_payment(&order.price, &order.fee_percent).ok()
 			);
-			assert!(Marketplace::bids((0, 0, order.price)) == Some(bid));
+			assert!(Bids::<Test>::get((0, 0, order.price)) == Some(bid));
 		})
 	}
 
@@ -926,7 +926,7 @@ mod execute_ask_with_existing_bid {
 				Execution::AllowCreation
 			));
 
-			let payout_address = Marketplace::payout_address().unwrap();
+			let payout_address = PayoutAddress::<Test>::get().unwrap();
 			let payout_address_balance_before = Balances::balance(&payout_address);
 			let seller_balance_before = Balances::balance(&seller);
 			let buyer_reserved_balance_before =
@@ -1106,7 +1106,7 @@ mod execute_ask_with_existing_bid {
 				Execution::AllowCreation
 			));
 
-			let payout_address = Marketplace::payout_address().unwrap();
+			let payout_address = PayoutAddress::<Test>::get().unwrap();
 			let payout_address_balance_before = Balances::balance(&payout_address);
 			let seller_balance_before = Balances::balance(&seller);
 			let buyer_reserved_balance_before =
@@ -1187,7 +1187,7 @@ mod execute_bid_with_existing_ask {
 
 			Balances::set_balance(&buyer, 1000000);
 
-			let payout_address = Marketplace::payout_address().unwrap();
+			let payout_address = PayoutAddress::<Test>::get().unwrap();
 			let payout_address_balance_before = Balances::balance(&payout_address);
 			let seller_balance_before = Balances::balance(&seller);
 			let buyer_balance_before = Balances::balance(&buyer);
@@ -1264,7 +1264,7 @@ mod execute_bid_with_existing_ask {
 
 			Balances::set_balance(&buyer, 1000000);
 
-			let payout_address = Marketplace::payout_address().unwrap();
+			let payout_address = PayoutAddress::<Test>::get().unwrap();
 			let payout_address_balance_before = Balances::balance(&payout_address);
 			let seller_balance_before = Balances::balance(&seller);
 			let buyer_balance_before = Balances::balance(&buyer);
@@ -1374,7 +1374,7 @@ mod cancel_ask {
 				0
 			));
 
-			assert!(Marketplace::asks(0, 0) == None);
+			assert!(Asks::<Test>::get(0, 0) == None);
 			assert!(Nfts::can_transfer(&0, &0));
 		})
 	}
@@ -1395,7 +1395,7 @@ mod cancel_ask {
 				0
 			));
 
-			assert!(Marketplace::asks(0, 0) == None);
+			assert!(Asks::<Test>::get(0, 0) == None);
 			assert!(Nfts::can_transfer(&0, &0));
 		})
 	}
@@ -1471,7 +1471,7 @@ mod cancel_bid {
 			));
 
 			let fee = <Test as Config>::MaxBasisPoints::get();
-			assert!(Marketplace::asks(0, 0) == None);
+			assert!(Asks::<Test>::get(0, 0) == None);
 
 			let bid_payment = Marketplace::calc_bid_payment(&price, &fee).unwrap_or_default();
 			assert!(
@@ -1503,7 +1503,7 @@ mod cancel_bid {
 			));
 
 			let fee = <Test as Config>::MaxBasisPoints::get();
-			assert!(Marketplace::asks(0, 0) == None);
+			assert!(Asks::<Test>::get(0, 0) == None);
 
 			let bid_payment = Marketplace::calc_bid_payment(&price, &fee).unwrap_or_default();
 			assert!(

pallets/marketplace/src/types.rs

@@ -16,7 +16,7 @@ pub struct Ask<AccountId, Amount, Expiration> {
 }
 
 #[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, TypeInfo, MaxEncodedLen)]
-pub struct Bid<AccountId, Expiration, Amount> {
+pub struct Bid<AccountId, Amount, Expiration> {
 	pub buyer: AccountId,
 	pub expiration: Expiration,
 	pub fee: Amount,
@@ -28,6 +28,12 @@ pub enum OrderType {
 	Bid,
 }
 
+#[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, TypeInfo)]
+pub enum ExecOrder<AccountId, Amount, Expiration> {
+	Ask(Ask<AccountId, Amount, Expiration>),
+	Bid(Bid<AccountId, Amount, Expiration>),
+}
+
 #[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, TypeInfo)]
 pub struct Order<CollectionId, ItemId, Amount, Expiration, OffchainSignature, BoundedString> {
 	pub order_type: OrderType,

runtime/testnet/src/lib.rs

@@ -612,6 +612,8 @@ impl pallet_marketplace::Config for Runtime {
 	type Signature = Signature;
 	type Signer = <Signature as Verify>::Signer;
 	type WeightInfo = pallet_marketplace::weights::SubstrateWeight<Runtime>;
+	#[cfg(feature = "runtime-benchmarks")]
+	type BenchmarkHelper = ();
 }
 
 parameter_types! {