Merge pull request #875 from galacticcouncil/fix_otc_settlements

fix: otc settlements

pallets/otc-settlements/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = 'pallet-otc-settlements'
-version = '1.0.2'
+version = '1.0.3'
 description = 'A pallet with offchain worker closing OTC arbs'
 authors = ['GalacticCouncil']
 edition = '2021'

pallets/otc-settlements/README.md

@@ -9,7 +9,8 @@ by any user using signed origin. In the former case, the block producer doesn't
 ## Notes
 If the OTC order is partially fillable, the pallet tries to close the arbitrage opportunity by finding the amount that 
 aligns the OTC and the Omnipool prices. Executing this trade needs to be profitable, but we are not trying to maximize 
-the profit.
+the profit. If the pallet couldn't find the amount that closes the arb, the amount that reduces the size of the arb is 
+used.
 In the case of not partially fillable OTC orders, the pallet tries to maximize the profit.
 
 ## Dispatachable functions

pallets/otc-settlements/src/lib.rs

@@ -25,26 +25,28 @@
 //! ## Notes
 //! If the OTC order is partially fillable, the pallet tries to close the arbitrage opportunity by finding the amount that
 //! aligns the OTC and the Omnipool prices. Executing this trade needs to be profitable, but we are not trying to maximize
-//! the profit.
+//! the profit. If the pallet couldn't find the amount that closes the arb, the amount that reduces the size of the arb is used.
 //! In the case of not partially fillable OTC orders, the pallet tries to maximize the profit.
 //!
 //! ## Dispatachable functions
 //! * `settle_otc_order` -  Executes a trade between an OTC order and some route.
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
+#[cfg(not(feature = "runtime-benchmarks"))]
+use frame_system::RawOrigin;
+
 use frame_support::{
 	pallet_prelude::*,
 	traits::{
 		fungibles::{Inspect, Mutate},
 		tokens::{Fortitude, Precision, Preservation},
 	},
-	PalletId,
+	transactional, PalletId,
 };
 use frame_system::{
 	offchain::{SendTransactionTypes, SubmitTransaction},
 	pallet_prelude::{BlockNumberFor, OriginFor},
-	RawOrigin,
 };
 use hydradx_traits::router::{
 	AmmTradeWeights, AmountInAndOut, AssetPair, RouteProvider, RouteSpotPriceProvider, RouterT, Trade,
@@ -221,7 +223,7 @@ pub mod pallet {
 		///
 		/// Executes a trade between an OTC order and some route.
 		/// If the OTC order is partially fillable, the extrinsic fails if the existing arbitrage
-		/// opportunity is not closed after the trade.
+		/// opportunity is not closed or reduced after the trade.
 		/// If the OTC order is not partially fillable, fails if there is no profit after the trade.
 		///
 		/// `Origin` calling this extrinsic is not paying or receiving anything.
@@ -250,7 +252,9 @@ pub mod pallet {
 			amount: Balance,
 			route: Vec<Trade<AssetIdOf<T>>>,
 		) -> DispatchResult {
-			Self::settle_otc(otc_id, amount, route)
+			// `is_execution` is set to `true`, so both full and partial closing of arbs is allowed.
+			// If set to `false`, an arb needs to be fully closed.
+			Self::settle_otc(otc_id, amount, route, true)
 		}
 	}
 }
@@ -280,15 +284,23 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Executes two trades: asset_a -> OTC -> asset_b, and asset_b -> Router -> asset_a.
 	///
-	/// If the OTC order is partially fillable, the extrinsic fails if the existing arbitrage
+	/// If the OTC order is partially fillable and `is_execution` is set to `false`, the extrinsic fails if the existing arbitrage
 	/// opportunity is not closed after the trade and if there is no profit after the trade.
+	/// If the OTC order is partially fillable and `is_execution` is set to `true`, arbs are allowed to be partially closed.
 	/// If the OTC order is not partially fillable, fails only if there is no profit after the trade.
 	///
 	/// Parameters:
 	/// - `otc_id`: ID of the OTC order with existing arbitrage opportunity.
 	/// - `amount`: Amount necessary to close the arb.
 	/// - `route`: The route we trade against. Required for the fee calculation.
-	pub fn settle_otc(otc_id: OrderId, amount: Balance, route: Vec<Trade<AssetIdOf<T>>>) -> DispatchResult {
+	/// - `is_execution`: When enabled, test for the price precision is disabled.
+	#[transactional]
+	pub fn settle_otc(
+		otc_id: OrderId,
+		amount: Balance,
+		route: Vec<Trade<AssetIdOf<T>>>,
+		is_execution: bool,
+	) -> DispatchResult {
 		log::debug!(
 			target: "offchain_worker::settle_otc",
 			"calling settle_otc(): otc_id: {:?} amount: {:?} route: {:?}", otc_id, amount, route);
@@ -368,7 +380,7 @@ impl<T: Config> Pallet<T> {
 
 		// Compare OTC and Router price.
 		// In the case of fully fillable orders, the resulting price is not important.
-		if otc.partially_fillable {
+		if !is_execution && otc.partially_fillable {
 			let price_diff = {
 				if otc_price > router_price_after {
 					otc_price.saturating_sub(router_price_after)
@@ -382,7 +394,10 @@ impl<T: Config> Pallet<T> {
 				.ok_or(ArithmeticError::Overflow)?;
 			if price_diff > price_precision {
 				ensure!(router_price_after <= otc_price, Error::<T>::TradeAmountTooHigh);
-				ensure!(router_price_after >= otc_price, Error::<T>::TradeAmountTooLow);
+				// partially fillable OTC can be fully filled if the arb is reduced
+				if amount != otc.amount_in {
+					ensure!(router_price_after >= otc_price, Error::<T>::TradeAmountTooLow);
+				}
 			}
 		}
 
@@ -559,7 +574,7 @@ impl<T: Config> Pallet<T> {
 			log::debug!(
 			target: "offchain_worker::settle_otcs::binary_search",
 				"\nsell_amt: {:?}\nsell_amt_up: {:?}\nsell_amt_down: {:?}", sell_amt, sell_amt_up, sell_amt_down);
-			match Self::settle_otc_order(RawOrigin::None.into(), otc_id, sell_amt, route.to_vec()) {
+			match Self::settle_otc(otc_id, sell_amt, route.to_vec(), false) {
 				Ok(_) => {
 					log::debug!(
 					target: "offchain_worker::settle_otcs",
@@ -590,12 +605,21 @@ impl<T: Config> Pallet<T> {
 					}
 				}
 			}
+			// no more values to test
 			if sell_amt_down == sell_amt_up {
-				return None;
+				break;
 			}
 			sell_amt = (sell_amt_up.checked_add(sell_amt_down)).and_then(|value| value.checked_div(2))?;
 		}
-		None
+		// execute with the latest min value
+		if sell_amt_down != T::MinTradingLimit::get() {
+			match Self::settle_otc(otc_id, sell_amt_down, route.to_vec(), true) {
+				Ok(_) => Some(sell_amt_down),
+				Err(_) => None,
+			}
+		} else {
+			None
+		}
 	}
 
 	/// Calculates the price (asset_out/asset_in) after subtracting the OTC fee from the amount_out.

pallets/otc-settlements/src/mock.rs

@@ -414,7 +414,7 @@ impl Default for ExtBuilder {
 			endowed_accounts: vec![
 				(ALICE, HDX, 1_000_000_000_000 * ONE),
 				(ALICE, LRNA, 1_000_000_000_000 * ONE),
-				(ALICE, DAI, 1_000_000_000_000 * ONE),
+				(ALICE, DAI, 1_000_000_000_000_000_000 * ONE),
 				(ALICE, DOT, 1_000_000_000_000 * ONE),
 				(ALICE, KSM, 1_000_000_000_000 * ONE),
 				(ALICE, BTC, 1_000_000_000_000 * ONE),

pallets/otc-settlements/src/tests.rs

@@ -250,7 +250,7 @@ fn profit_should_be_transferred_to_treasury_when_nonzero_initial_pallet_balance(
 }
 
 #[test]
-fn existing_arb_opportunity_should_trigger_trade() {
+fn existing_arb_opportunity_should_trigger_trade_when_correct_amount_can_be_found() {
 	let (mut ext, _) = ExtBuilder::default().build();
 	ext.execute_with(|| {
 		assert_ok!(OTC::place_order(
@@ -300,6 +300,62 @@ fn existing_arb_opportunity_should_trigger_trade() {
 	});
 }
 
+#[test]
+fn existing_arb_opportunity_should_trigger_trade_when_partially_fillable_otc_can_be_fully_filled() {
+	let (mut ext, _) = ExtBuilder::default().build();
+	ext.execute_with(|| {
+		assert_ok!(OTC::place_order(
+			RuntimeOrigin::signed(ALICE),
+			HDX, // otc asset_in
+			DAI, // otc asset_out
+			100 * ONE,
+			205 * ONE,
+			true,
+		));
+
+		// get otc price
+		let otc_id = 0;
+		let otc = <pallet_otc::Orders<Test>>::get(otc_id).unwrap();
+		let otc_price = calculate_otc_price(&otc);
+
+		// get trade price
+		let route = Router::get_route(AssetPair {
+			asset_in: otc.asset_out,
+			asset_out: otc.asset_in,
+		});
+		let initial_router_price = Router::spot_price_with_fee(&route).unwrap();
+
+		// verify that there's an arb opportunity
+		assert!(otc_price > initial_router_price);
+
+		let hdx_total_issuance = Currencies::total_issuance(HDX);
+		let dai_total_issuance = Currencies::total_issuance(DAI);
+
+		assert!(Currencies::free_balance(HDX, &OtcSettlements::account_id()) == 0);
+		assert!(Currencies::free_balance(DAI, &OtcSettlements::account_id()) == 0);
+
+		<OtcSettlements as Hooks<BlockNumberFor<Test>>>::offchain_worker(System::block_number());
+
+		// verify that the arb is still there, but smaller than before
+		let final_router_price = Router::spot_price_with_fee(&route).unwrap();
+		assert!(otc_price > final_router_price);
+		assert!(final_router_price > initial_router_price);
+
+		assert_eq!(hdx_total_issuance, Currencies::total_issuance(HDX));
+		assert_eq!(dai_total_issuance, Currencies::total_issuance(DAI));
+
+		// total issuance of tokens should not change
+		assert!(Currencies::free_balance(HDX, &OtcSettlements::account_id()) == 0);
+		assert!(Currencies::free_balance(DAI, &OtcSettlements::account_id()) == 0);
+
+		expect_last_events(vec![Event::Executed {
+			asset_id: HDX,
+			profit: 1_444_117_874_415,
+		}
+		.into()]);
+	});
+}
+
 #[test]
 fn existing_arb_opportunity_should_trigger_trade_when_otc_is_not_partially_fillable() {
 	let (mut ext, _) = ExtBuilder::default().build();
@@ -562,7 +618,7 @@ fn trade_should_not_be_triggered_when_there_is_no_arb_opportunity() {
 }
 
 #[test]
-fn trade_should_not_be_triggered_when_optimal_amount_not_found() {
+fn trade_should_be_triggered_when_optimal_amount_not_found_but_arb_can_be_reduced() {
 	let (mut ext, _) = ExtBuilder::default().build();
 	ext.execute_with(|| {
 		assert_ok!(OTC::place_order(
@@ -595,9 +651,7 @@ fn trade_should_not_be_triggered_when_optimal_amount_not_found() {
 		assert!(Currencies::free_balance(HDX, &OtcSettlements::account_id()) == 0);
 		assert!(Currencies::free_balance(DAI, &OtcSettlements::account_id()) == 0);
 
-		assert_storage_noop!(<OtcSettlements as Hooks<BlockNumberFor<Test>>>::offchain_worker(
-			System::block_number()
-		));
+		<OtcSettlements as Hooks<BlockNumberFor<Test>>>::offchain_worker(System::block_number());
 
 		assert_eq!(hdx_total_issuance, Currencies::total_issuance(HDX));
 		assert_eq!(dai_total_issuance, Currencies::total_issuance(DAI));
@@ -608,6 +662,43 @@ fn trade_should_not_be_triggered_when_optimal_amount_not_found() {
 	});
 }
 
+#[test]
+fn trade_should_not_be_triggered_when_amount_not_found() {
+	let (mut ext, _) = ExtBuilder::default().build();
+	ext.execute_with(|| {
+		assert_ok!(OTC::place_order(
+			RuntimeOrigin::signed(ALICE),
+			HDX, // otc asset_in
+			DAI, // otc asset_out
+			1_000 * ONE,
+			800_000_000_000_000_001 * ONE,
+			true,
+		));
+
+		// get otc price
+		let otc_id = 0;
+		let otc = <pallet_otc::Orders<Test>>::get(otc_id).unwrap();
+		let otc_price = calculate_otc_price(&otc);
+
+		// get trade price
+		let route = Router::get_route(AssetPair {
+			asset_in: otc.asset_out,
+			asset_out: otc.asset_in,
+		});
+		let router_price = Router::spot_price_with_fee(&route).unwrap();
+
+		// verify that there's an arb opportunity
+		assert!(otc_price > router_price);
+
+		assert!(Currencies::free_balance(HDX, &OtcSettlements::account_id()) == 0);
+		assert!(Currencies::free_balance(DAI, &OtcSettlements::account_id()) == 0);
+
+		assert_storage_noop!(<OtcSettlements as Hooks<BlockNumberFor<Test>>>::offchain_worker(
+			System::block_number()
+		));
+	});
+}
+
 #[test]
 fn test_offchain_worker_unsigned_transaction_submission() {
 	let (mut ext, pool_state) = ExtBuilder::default().build();

runtime/hydradx/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "hydradx-runtime"
-version = "254.0.0"
+version = "255.0.0"
 authors = ["GalacticCouncil"]
 edition = "2021"
 license = "Apache 2.0"

runtime/hydradx/src/lib.rs

@@ -113,7 +113,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("hydradx"),
 	impl_name: create_runtime_str!("hydradx"),
 	authoring_version: 1,
-	spec_version: 254,
+	spec_version: 255,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,