Fix: zero amount order bug (#1816)

* add tests that fail and shouldn't

* add fix

* add extra tests

pallets/foreign-investments/src/tests.rs

@@ -1592,3 +1592,126 @@ mod notifications {
  });
  }
 }
+
+mod zero_amount_order {
+ use super::*;
+
+ #[test]
+ fn when_increase_with_zero() {
+ new_test_ext().execute_with(|| {
+ util::base_configuration();
+
+ assert_ok!(ForeignInvestment::increase_foreign_investment(
+ &USER,
+ INVESTMENT_ID,
+ 0,
+ FOREIGN_CURR
+ ));
+
+ assert_err!(
+ Swaps::order_id(&USER, (INVESTMENT_ID, Action::Investment)),
+ pallet_swaps::Error::<Runtime>::OrderNotFound
+ );
+ });
+ }
+
+ #[test]
+ fn when_increase_after_decrease_but_math_precission() {
+ new_test_ext().execute_with(|| {
+ const FOREIGN_AMOUNT: Balance = 100;
+
+ util::base_configuration();
+
+ MockTokenSwaps::mock_convert_by_market(|to, from, amount_from| {
+ Ok(match (from, to) {
+ (POOL_CURR, FOREIGN_CURR) => amount_from / 3 + 1,
+ (FOREIGN_CURR, POOL_CURR) => amount_from * 3,
+ _ => unreachable!(),
+ })
+ });
+
+ assert_ok!(ForeignInvestment::increase_foreign_investment(
+ &USER,
+ INVESTMENT_ID,
+ FOREIGN_AMOUNT,
+ FOREIGN_CURR
+ ));
+
+ util::fulfill_last_swap(Action::Investment, FOREIGN_AMOUNT);
+
+ assert!(Swaps::order_id(&USER, (INVESTMENT_ID, Action::Investment)).is_err());
+
+ assert_ok!(ForeignInvestment::decrease_foreign_investment(
+ &USER,
+ INVESTMENT_ID,
+ FOREIGN_AMOUNT,
+ FOREIGN_CURR
+ ));
+
+ MockDecreaseInvestHook::mock_notify_status_change(|_, msg| {
+ assert_eq!(
+ msg,
+ ExecutedForeignDecreaseInvest {
+ amount_decreased: FOREIGN_AMOUNT + 1,
+ foreign_currency: FOREIGN_CURR,
+ amount_remaining: FOREIGN_AMOUNT,
+ }
+ );
+ Ok(())
+ });
+
+ assert_ok!(ForeignInvestment::increase_foreign_investment(
+ &USER,
+ INVESTMENT_ID,
+ FOREIGN_AMOUNT + 1,
+ FOREIGN_CURR
+ ));
+
+ assert_err!(
+ Swaps::order_id(&USER, (INVESTMENT_ID, Action::Investment)),
+ pallet_swaps::Error::<Runtime>::OrderNotFound
+ );
+ });
+ }
+
+ #[test]
+ fn when_increase_fulfill_is_notified() {
+ new_test_ext().execute_with(|| {
+ util::base_configuration();
+
+ assert_ok!(ForeignInvestment::increase_foreign_investment(
+ &USER,
+ INVESTMENT_ID,
+ AMOUNT,
+ FOREIGN_CURR
+ ));
+
+ util::fulfill_last_swap(Action::Investment, 0);
+ });
+ }
+
+ #[test]
+ fn when_decrease_fulfill_is_notified() {
+ new_test_ext().execute_with(|| {
+ util::base_configuration();
+
+ assert_ok!(ForeignInvestment::increase_foreign_investment(
+ &USER,
+ INVESTMENT_ID,
+ AMOUNT,
+ FOREIGN_CURR
+ ));
+
+ util::fulfill_last_swap(Action::Investment, AMOUNT);
+
+ assert_ok!(ForeignInvestment::decrease_foreign_investment(
+ &USER,
+ INVESTMENT_ID,
+ AMOUNT,
+ FOREIGN_CURR
+ ));
+
+ util::fulfill_last_swap(Action::Investment, 0);
+ });
+ }
+}

pallets/order-book/src/tests.rs

@@ -463,6 +463,47 @@ fn correct_order_details() {
  });
 }
 
+#[test]
+fn fulfill_zero_amount_order() {
+ new_test_ext().execute_with(|| {
+ let order_id = <OrderBook as TokenSwaps<AccountId>>::place_order(
+ FROM,
+ CURRENCY_B,
+ CURRENCY_A,
+ 0,
+ OrderRatio::Custom(DEFAULT_RATIO),
+ )
+ .unwrap();
+
+ util::expect_notification(order_id, 0, 0, 0);
+
+ assert_ok!(OrderBook::fill_order(
+ RuntimeOrigin::signed(FROM),
+ order_id,
+ 0
+ ));
+ });
+}
+
+#[test]
+fn close_zero_amount_order() {
+ new_test_ext().execute_with(|| {
+ let order_id = <OrderBook as TokenSwaps<AccountId>>::place_order(
+ FROM,
+ CURRENCY_B,
+ CURRENCY_A,
+ 0,
+ OrderRatio::Custom(DEFAULT_RATIO),
+ )
+ .unwrap();
+
+ assert_ok!(OrderBook::cancel_order(
+ RuntimeOrigin::signed(FROM),
+ order_id
+ ));
+ });
+}
+
 mod market {
  use super::*;
 

pallets/swaps/src/lib.rs

@@ -145,20 +145,24 @@ pub mod pallet {
  ) -> Result<(SwapStatus<T::Balance>, Option<T::OrderId>), DispatchError> {
  match over_order_id {
  None => {
- let order_id = T::OrderBook::place_order(
- who.clone(),
- new_swap.currency_in,
- new_swap.currency_out,
- new_swap.amount_out,
- OrderRatio::Market,
- )?;
+ let order_id = if !new_swap.amount_out.is_zero() {
+ Some(T::OrderBook::place_order(
+ who.clone(),
+ new_swap.currency_in,
+ new_swap.currency_out,
+ new_swap.amount_out,
+ OrderRatio::Market,
+ )?)
+ } else {
+ None
+ };
 
  Ok((
  SwapStatus {
  swapped: T::Balance::zero(),
  pending: new_swap.amount_out,
  },
- Some(order_id),
+ order_id,
  ))
  }
  Some(order_id) => {
@@ -228,20 +232,24 @@ pub mod pallet {
  let amount_to_swap =
  new_swap.amount_out.ensure_sub(inverse_swap_amount_in)?;
 
- let order_id = T::OrderBook::place_order(
- who.clone(),
- new_swap.currency_in,
- new_swap.currency_out,
- amount_to_swap,
- OrderRatio::Market,
- )?;
+ let order_id = if !amount_to_swap.is_zero() {
+ Some(T::OrderBook::place_order(
+ who.clone(),
+ new_swap.currency_in,
+ new_swap.currency_out,
+ amount_to_swap,
+ OrderRatio::Market,
+ )?)
+ } else {
+ None
+ };
 
  Ok((
  SwapStatus {
  swapped: inverse_swap.amount_out,
  pending: amount_to_swap,
  },
- Some(order_id),
+ order_id,
  ))
  }
  }

pallets/swaps/src/tests.rs

@@ -490,3 +490,86 @@ mod fulfill {
  });
  }
 }
+
+mod zero_amount_order {
+ use super::*;
+
+ #[test]
+ fn when_apply_over_no_swap() {
+ new_test_ext().execute_with(|| {
+ MockTokenSwaps::mock_place_order(|_, _, _, _, _| {
+ panic!("this mock should not be called");
+ });
+
+ assert_ok!(
+ <Swaps as TSwaps<AccountId>>::apply_swap(
+ &USER,
+ SWAP_ID,
+ Swap {
+ currency_in: CURRENCY_B,
+ currency_out: CURRENCY_A,
+ amount_out: 0,
+ },
+ ),
+ SwapStatus {
+ swapped: 0,
+ pending: 0,
+ }
+ );
+
+ assert_eq!(OrderIdToSwapId::<Runtime>::get(ORDER_ID), None);
+ assert_eq!(SwapIdToOrderId::<Runtime>::get((USER, SWAP_ID)), None);
+ });
+ }
+
+ #[test]
+ fn when_apply_over_smaller_inverse_swap_but_math_precission() {
+ const AMOUNT_A: Balance = 100;
+
+ new_test_ext().execute_with(|| {
+ MockTokenSwaps::mock_convert_by_market(|to, from, amount_from| match (from, to) {
+ (CURRENCY_B, CURRENCY_A) => Ok(amount_from * 3),
+ (CURRENCY_A, CURRENCY_B) => Ok(amount_from / 3 + 1),
+ _ => unreachable!(),
+ });
+ MockTokenSwaps::mock_get_order_details(|_| {
+ // Inverse swap
+ Some(OrderInfo {
+ swap: Swap {
+ currency_in: CURRENCY_A,
+ currency_out: CURRENCY_B,
+ amount_out: AMOUNT_A / 3,
+ },
+ ratio: OrderRatio::Market,
+ })
+ });
+ MockTokenSwaps::mock_cancel_order(|_| Ok(()));
+
+ MockTokenSwaps::mock_place_order(|_, _, _, amount, _| {
+ assert_eq!(amount, 0);
+ panic!("this mock should not be called");
+ });
+
+ Swaps::update_id(&USER, SWAP_ID, Some(ORDER_ID)).unwrap();
+
+ assert_ok!(
+ <Swaps as TSwaps<AccountId>>::apply_swap(
+ &USER,
+ SWAP_ID,
+ Swap {
+ currency_out: CURRENCY_A,
+ currency_in: CURRENCY_B,
+ amount_out: AMOUNT_A - 1,
+ },
+ ),
+ SwapStatus {
+ swapped: AMOUNT_A / 3,
+ pending: 0,
+ }
+ );
+
+ assert_eq!(OrderIdToSwapId::<Runtime>::get(ORDER_ID), None);
+ assert_eq!(SwapIdToOrderId::<Runtime>::get((USER, SWAP_ID)), None);
+ });
+ }
+}