fix: min fulfillment amount for partial fulfillment (#1570)

* tests: add checks for executed_collect_*

* tests: check for ExecutedDecrease* dispatch

* fix: support partial swaps for FI

* fix: min fulfillment in order-book

* docs: fix

* fix: orderbook benches

* fix: docs

* fix: FI mock tests

* fix: orderbook benches

* docs: remove remnant

libs/mocks/src/token_swaps.rs

@@ -33,18 +33,16 @@ pub mod pallet {
 					T::CurrencyId,
 					T::Balance,
 					T::SellRatio,
-					T::Balance,
 				) -> Result<T::OrderId, DispatchError>
 				+ 'static,
 		) {
-			register_call!(move |(a, b, c, d, e, g)| f(a, b, c, d, e, g));
+			register_call!(move |(a, b, c, d, e)| f(a, b, c, d, e));
 		}
 
 		pub fn mock_update_order(
-			f: impl Fn(T::AccountId, T::OrderId, T::Balance, T::SellRatio, T::Balance) -> DispatchResult
-				+ 'static,
+			f: impl Fn(T::AccountId, T::OrderId, T::Balance, T::SellRatio) -> DispatchResult + 'static,
 		) {
-			register_call!(move |(a, b, c, d, e)| f(a, b, c, d, e));
+			register_call!(move |(a, b, c, d)| f(a, b, c, d));
 		}
 
 		pub fn mock_cancel_order(f: impl Fn(T::OrderId) -> DispatchResult + 'static) {
@@ -79,19 +77,17 @@ pub mod pallet {
 			c: Self::CurrencyId,
 			d: Self::Balance,
 			e: Self::SellRatio,
-			f: Self::Balance,
 		) -> Result<Self::OrderId, DispatchError> {
-			execute_call!((a, b, c, d, e, f))
+			execute_call!((a, b, c, d, e))
 		}
 
 		fn update_order(
 			a: T::AccountId,
 			b: Self::OrderId,
 			c: Self::Balance,
 			d: Self::SellRatio,
-			e: Self::Balance,
 		) -> DispatchResult {
-			execute_call!((a, b, c, d, e))
+			execute_call!((a, b, c, d))
 		}
 
 		fn cancel_order(a: Self::OrderId) -> DispatchResult {

libs/traits/src/lib.rs

@@ -472,11 +472,15 @@ pub trait TokenSwaps<Account> {
 	/// `sell_rate_limit` defines the highest price acceptable for
 	/// `currency_in` currency when buying with `currency_out`. This
 	/// protects order placer if market changes unfavourably for swap order.
-	/// For example, with a `sell_rate_limit` of `3/2` one asset in should never
-	/// cost more than 1.5 units of asset out. Returns `Result` with `OrderId`
-	/// upon successful order creation.
+	/// For example, with a `sell_rate_limit` of `3/2`, one `asset_in`
+	/// should never cost more than 1.5 units of `asset_out`. Returns `Result`
+	/// with `OrderId` upon successful order creation.
 	///
-	/// Example usage with pallet_order_book impl:
+	/// NOTE: The minimum fulfillment amount is implicitly set by the
+	/// implementor.
+	///
+	/// Example usage with `pallet_order_book` impl:
+	/// ```ignore
 	/// OrderBook::place_order(
 	///     {AccountId},
 	///     CurrencyId::ForeignAsset(0),
@@ -485,49 +489,51 @@ pub trait TokenSwaps<Account> {
 	///     Quantity::checked_from_rational(3u32, 2u32).unwrap(),
 	///     100 * FOREIGN_ASSET_0_DECIMALS
 	/// )
-	/// Would return Ok({OrderId})
-	/// and create the following order in storage:
+	/// ```
+	/// Would return `Ok({OrderId}` and create the following order in storage:
+	/// ```ignore
 	/// Order {
 	///     order_id: {OrderId},
 	///     placing_account: {AccountId},
 	///     asset_in_id: CurrencyId::ForeignAsset(0),
 	///     asset_out_id: CurrencyId::ForeignAsset(1),
 	///     buy_amount: 100 * FOREIGN_ASSET_0_DECIMALS,
 	///     initial_buy_amount: 100 * FOREIGN_ASSET_0_DECIMALS,
-	///     sell_rate_limit: Quantity::checked_from_rational(3u32,
-	/// 2u32).unwrap(),     min_fulfillment_amount: 100 *
-	/// FOREIGN_ASSET_0_DECIMALS,     max_sell_amount: 150 *
-	/// FOREIGN_ASSET_1_DECIMALS }
+	///     sell_rate_limit: Quantity::checked_from_rational(3u32, 2u32).unwrap(),
+	///     max_sell_amount: 150 * FOREIGN_ASSET_1_DECIMALS,
+	///     min_fulfillment_amount: 10 * CFG * FOREIGN_ASSET_0_DECIMALS,
+	/// }
+	/// ```
 	fn place_order(
 		account: Account,
 		currency_in: Self::CurrencyId,
 		currency_out: Self::CurrencyId,
 		buy_amount: Self::Balance,
 		sell_rate_limit: Self::SellRatio,
-		min_fulfillment_amount: Self::Balance,
 	) -> Result<Self::OrderId, DispatchError>;
 
 	/// Update an existing active order.
-	/// As with create order `sell_rate_limit` defines the highest price
-	/// acceptable for `currency_in` currency when buying with `currency_out`.
-	/// Returns a Dispatch result.
+	/// As with creating an order, the `sell_rate_limit` defines the highest
+	/// price acceptable for `currency_in` currency when buying with
+	/// `currency_out`. Returns a Dispatch result.
 	///
-	/// This Can fail for various reasons
+	/// NOTE: The minimum fulfillment amount is implicitly set by the
+	/// implementor.
 	///
-	/// E.g. min_fulfillment_amount is lower and
-	///      the system has already fulfilled up to the previous
-	///      one.
+	/// This Can fail for various reasons.
 	///
-	/// Example usage with pallet_order_book impl:
+	/// Example usage with `pallet_order_book` impl:
+	/// ```ignore
 	/// OrderBook::update_order(
 	///     {AccountId},
 	///     {OrderId},
 	///     15 * FOREIGN_ASSET_0_DECIMALS,
 	///     Quantity::checked_from_integer(2u32).unwrap(),
 	///     6 * FOREIGN_ASSET_0_DECIMALS
 	/// )
-	/// Would return Ok(())
-	/// and update the following order in storage:
+	/// ```
+	/// Would return `Ok(())` and update the following order in storage:
+	/// ```ignore
 	/// Order {
 	///     order_id: {OrderId},
 	///     placing_account: {AccountId},
@@ -536,15 +542,15 @@ pub trait TokenSwaps<Account> {
 	///     buy_amount: 15 * FOREIGN_ASSET_0_DECIMALS,
 	///     initial_buy_amount: 100 * FOREIGN_ASSET_0_DECIMALS,
 	///     sell_rate_limit: Quantity::checked_from_integer(2u32).unwrap(),
-	///     min_fulfillment_amount: 6 * FOREIGN_ASSET_0_DECIMALS,
 	///     max_sell_amount: 30 * FOREIGN_ASSET_1_DECIMALS
+	///     min_fulfillment_amount: 10 * CFG * FOREIGN_ASSET_0_DECIMALS,
 	/// }
+	/// ```
 	fn update_order(
 		account: Account,
 		order_id: Self::OrderId,
 		buy_amount: Self::Balance,
 		sell_rate_limit: Self::SellRatio,
-		min_fulfillment_amount: Self::Balance,
 	) -> DispatchResult;
 
 	/// A sanity check that can be used for validating that a trading pair
@@ -596,11 +602,32 @@ pub trait IdentityCurrencyConversion {
 }
 
 /// A trait for trying to convert between two types.
-// TODO: Remove usage for the one from Polkadot once we are on the same version
+// TODO: Remove usage for the one from sp_runtime::traits once we are on
+// the same Polkadot version
 pub trait TryConvert<A, B> {
 	type Error;
 
 	/// Attempt to make conversion. If returning [Result::Err], the inner must
 	/// always be `a`.
 	fn try_convert(a: A) -> Result<B, Self::Error>;
 }
+
+/// Converts a balance value into an asset balance.
+// TODO: Remove usage for the one from frame_support::traits::tokens once we are
+// on the same Polkadot version
+pub trait ConversionToAssetBalance<InBalance, AssetId, AssetBalance> {
+	type Error;
+	fn to_asset_balance(balance: InBalance, asset_id: AssetId)
+		-> Result<AssetBalance, Self::Error>;
+}
+
+/// Converts an asset balance value into balance.
+// TODO: Remove usage for the one from frame_support::traits::tokens once we are
+// on the same Polkadot version
+pub trait ConversionFromAssetBalance<AssetBalance, AssetId, OutBalance> {
+	type Error;
+	fn from_asset_balance(
+		balance: AssetBalance,
+		asset_id: AssetId,
+	) -> Result<OutBalance, Self::Error>;
+}

pallets/foreign-investments/src/hooks.rs

@@ -79,18 +79,28 @@ impl<T: Config> StatusNotificationHook for FulfilledSwapOrderHook<T> {
 					Error::<T>::FulfilledTokenSwapAmountOverflow
 				);
 
-				let invest_swap = SwapOf::<T> {
-					amount: active_invest_swap_amount,
-					..status
-				};
-				let redeem_swap = SwapOf::<T> {
-					amount: status.amount.ensure_sub(active_invest_swap_amount)?,
-					..status
-				};
-
-				// NOTE: Fulfillment of invest swap before redeem one for no particular reason
-				Self::fulfill_invest_swap_order(&info.owner, info.id, invest_swap, false)?;
-				Self::fulfill_redeem_swap_order(&info.owner, info.id, redeem_swap)
+				// Order was fulfilled at least for invest swap amount
+				if status.amount > active_invest_swap_amount {
+					let invest_swap = SwapOf::<T> {
+						amount: active_invest_swap_amount,
+						..status
+					};
+					let redeem_swap = SwapOf::<T> {
+						amount: status.amount.ensure_sub(active_invest_swap_amount)?,
+						..status
+					};
+
+					// NOTE: Fulfillment of invest swap before redeem one for no particular reason.
+					// If we wanted to fulfill the min swap amount, we would have to add support for
+					// oppression of for swap updates to `fulfill_redeem_swap_order` as well in case
+					// redeem_swap.amount < status.amount < invest_swap.amount
+					Self::fulfill_invest_swap_order(&info.owner, info.id, invest_swap, false)?;
+					Self::fulfill_redeem_swap_order(&info.owner, info.id, redeem_swap)
+				}
+				// Order was fulfilled below invest swap amount
+				else {
+					Self::fulfill_invest_swap_order(&info.owner, info.id, status, true)
+				}
 			}
 			_ => {
 				log::debug!("Fulfilled token swap order id {:?} without advancing foreign investment because swap reason does not exist", id);

pallets/foreign-investments/src/impls/mod.rs

@@ -798,8 +798,6 @@ impl<T: Config> Pallet<T> {
 					swap.amount,
 					// The max accepted sell rate is independent of the asset type for now
 					T::DefaultTokenSellRatio::get(),
-					// The minimum fulfillment must be everything
-					swap.amount,
 				)?;
 				ForeignInvestmentInfo::<T>::insert(
 					swap_order_id,
@@ -827,8 +825,6 @@ impl<T: Config> Pallet<T> {
 					swap.amount,
 					// The max accepted sell rate is independent of the asset type for now
 					T::DefaultTokenSellRatio::get(),
-					// The minimum fulfillment must be everything
-					swap.amount,
 				)?;
 				TokenSwapOrderIds::<T>::insert(who, investment_id, swap_order_id);
 				ForeignInvestmentInfo::<T>::insert(

pallets/foreign-investments/src/tests.rs

@@ -22,7 +22,7 @@ mod util {
 		MockInvestment::mock_investment_requires_collect(|_, _| false);
 		MockInvestment::mock_investment(|_, _| Ok(0));
 		MockInvestment::mock_update_investment(|_, _, _| Ok(()));
-		MockTokenSwaps::mock_place_order(move |_, _, _, _, _, _| Ok(order_id));
+		MockTokenSwaps::mock_place_order(move |_, _, _, _, _| Ok(order_id));
 		MockCurrencyConversion::mock_stable_to_stable(move |_, _, _| Ok(amount) /* 1:1 */);
 
 		ForeignInvestment::increase_foreign_investment(
@@ -37,7 +37,7 @@ mod util {
 		MockInvestment::mock_investment_requires_collect(|_, _| unimplemented!("no mock"));
 		MockInvestment::mock_investment(|_, _| unimplemented!("no mock"));
 		MockInvestment::mock_update_investment(|_, _, _| unimplemented!("no mock"));
-		MockTokenSwaps::mock_place_order(|_, _, _, _, _, _| unimplemented!("no mock"));
+		MockTokenSwaps::mock_place_order(|_, _, _, _, _| unimplemented!("no mock"));
 		MockCurrencyConversion::mock_stable_to_stable(|_, _, _| unimplemented!("no mock"));
 	}
 
@@ -92,17 +92,14 @@ mod increase_investment {
 				assert_eq!(amount, 0); // We still do not have the swap done.
 				Ok(())
 			});
-			MockTokenSwaps::mock_place_order(
-				|account_id, curr_in, curr_out, amount, limit, min| {
-					assert_eq!(account_id, USER);
-					assert_eq!(curr_in, POOL_CURR);
-					assert_eq!(curr_out, USER_CURR);
-					assert_eq!(amount, AMOUNT);
-					assert_eq!(limit, DefaultTokenSellRatio::get());
-					assert_eq!(min, AMOUNT);
-					Ok(ORDER_ID)
-				},
-			);
+			MockTokenSwaps::mock_place_order(|account_id, curr_in, curr_out, amount, limit| {
+				assert_eq!(account_id, USER);
+				assert_eq!(curr_in, POOL_CURR);
+				assert_eq!(curr_out, USER_CURR);
+				assert_eq!(amount, AMOUNT);
+				assert_eq!(limit, DefaultTokenSellRatio::get());
+				Ok(ORDER_ID)
+			});
 			MockCurrencyConversion::mock_stable_to_stable(|curr_in, curr_out, amount_out| {
 				assert_eq!(curr_in, POOL_CURR);
 				assert_eq!(curr_out, USER_CURR);
@@ -173,12 +170,11 @@ mod increase_investment {
 					amount: INITIAL_AMOUNT,
 				})
 			});
-			MockTokenSwaps::mock_update_order(|account_id, order_id, amount, limit, min| {
+			MockTokenSwaps::mock_update_order(|account_id, order_id, amount, limit| {
 				assert_eq!(account_id, USER);
 				assert_eq!(order_id, ORDER_ID);
 				assert_eq!(amount, INITIAL_AMOUNT + INCREASE_AMOUNT);
 				assert_eq!(limit, DefaultTokenSellRatio::get());
-				assert_eq!(min, INITIAL_AMOUNT + INCREASE_AMOUNT);
 				Ok(())
 			});
 			MockCurrencyConversion::mock_stable_to_stable(|curr_in, curr_out, amount_out| {
@@ -224,17 +220,14 @@ mod increase_investment {
 				assert_eq!(order_id, ORDER_ID);
 				false
 			});
-			MockTokenSwaps::mock_place_order(
-				|account_id, curr_in, curr_out, amount, limit, min| {
-					assert_eq!(account_id, USER);
-					assert_eq!(curr_in, POOL_CURR);
-					assert_eq!(curr_out, USER_CURR);
-					assert_eq!(amount, INCREASE_AMOUNT);
-					assert_eq!(limit, DefaultTokenSellRatio::get());
-					assert_eq!(min, INCREASE_AMOUNT);
-					Ok(ORDER_ID)
-				},
-			);
+			MockTokenSwaps::mock_place_order(|account_id, curr_in, curr_out, amount, limit| {
+				assert_eq!(account_id, USER);
+				assert_eq!(curr_in, POOL_CURR);
+				assert_eq!(curr_out, USER_CURR);
+				assert_eq!(amount, INCREASE_AMOUNT);
+				assert_eq!(limit, DefaultTokenSellRatio::get());
+				Ok(ORDER_ID)
+			});
 			MockInvestment::mock_update_investment(|_, _, amount| {
 				assert_eq!(amount, 0);
 				Ok(())

pallets/order-book/src/benchmarking.rs

@@ -12,6 +12,7 @@
 
 #![cfg(feature = "runtime-benchmarks")]
 
+use cfg_primitives::CFG;
 use cfg_traits::benchmarking::OrderBookBenchmarkHelper;
 use cfg_types::tokens::{CurrencyId, CustomMetadata};
 use frame_benchmarking::*;
@@ -21,8 +22,8 @@ use sp_runtime::FixedPointNumber;
 
 use super::*;
 
-const AMOUNT_IN: u128 = 1_000_000;
-const AMOUNT_OUT: u128 = 1_000_000_000_000;
+const AMOUNT_IN: u128 = 100 * CFG;
+const AMOUNT_OUT: u128 = 100_000_000 * CFG;
 const BUY_AMOUNT: u128 = 100 * AMOUNT_IN;
 const ASSET_IN: CurrencyId = CurrencyId::ForeignAsset(1);
 const ASSET_OUT: CurrencyId = CurrencyId::ForeignAsset(2);
@@ -44,28 +45,28 @@ benchmarks! {
 	user_update_order {
 		let (account_out, _) = Pallet::<T>::bench_setup_trading_pair(ASSET_IN, ASSET_OUT, 1000 * AMOUNT_IN, 1000 * AMOUNT_OUT, DECIMALS_IN, DECIMALS_OUT);
 
-		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into(), BUY_AMOUNT)?;
+		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into())?;
 
 		}:user_update_order(RawOrigin::Signed(account_out.clone()), order_id, 10 * BUY_AMOUNT, T::SellRatio::saturating_from_integer(1))
 
 	user_cancel_order {
 		let (account_out, _) = Pallet::<T>::bench_setup_trading_pair(ASSET_IN, ASSET_OUT, 1000 * AMOUNT_IN, 1000 * AMOUNT_OUT, DECIMALS_IN, DECIMALS_OUT);
 
-		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into(), BUY_AMOUNT)?;
+		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into())?;
 
 	}:user_cancel_order(RawOrigin::Signed(account_out.clone()), order_id)
 
 	fill_order_full {
 		let (account_out, account_in) = Pallet::<T>::bench_setup_trading_pair(ASSET_IN, ASSET_OUT, 1000 * AMOUNT_IN, 1000 * AMOUNT_OUT, DECIMALS_IN, DECIMALS_OUT);
 
-		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into(), BUY_AMOUNT)?;
+		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into())?;
 
 	}:fill_order_full(RawOrigin::Signed(account_in.clone()), order_id)
 
 	fill_order_partial {
 		let (account_out, account_in) = Pallet::<T>::bench_setup_trading_pair(ASSET_IN, ASSET_OUT, 1000 * AMOUNT_IN, 1000 * AMOUNT_OUT, DECIMALS_IN, DECIMALS_OUT);
 
-		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into(), BUY_AMOUNT / 10)?;
+		let order_id = Pallet::<T>::place_order(account_out.clone(), ASSET_IN, ASSET_OUT, BUY_AMOUNT, T::SellRatio::saturating_from_integer(2).into())?;
 
 	}:fill_order_partial(RawOrigin::Signed(account_in.clone()), order_id, BUY_AMOUNT / 2)