Merge pull request #1427 from galacticcouncil/fix/fallback-price-in-global-limit

fix: add fallback price for global rate limiter

Author: enthusiastmartin

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "runtime-integration-tests"
-version = "1.80.1"
+version = "1.81.0"
 description = "Integration tests"
 authors = ["GalacticCouncil"]
 edition = "2021"

integration-tests/Cargo.toml

@@ -1528,3 +1528,99 @@ fn inbound_xcm_incomplete_message_still_accounts_egress() {
 		);
 	});
 }
+
+#[test]
+fn polkadot_xcm_execute_withdraw_external_asset_succeeds_when_oracle_cannot_price_route() {
+	TestNet::reset();
+	Hydra::execute_with(|| {
+		init_global_withdraw_limit_params();
+
+		assert_ok!(CircuitBreaker::set_asset_category(
+			hydradx_runtime::RuntimeOrigin::root(),
+			DOT,
+			Some(GlobalAssetCategory::External)
+		));
+		// XCM asset transactor needs a location -> asset id mapping for DOT.
+		assert_ok!(hydradx_runtime::AssetRegistry::set_location(DOT, DOT_ASSET_LOCATION));
+		assert_ok!(hydradx_runtime::MultiTransactionPayment::add_currency(
+			hydradx_runtime::RuntimeOrigin::root(),
+			DOT,
+			FixedU128::from_rational(50, 100),
+		));
+		pallet_transaction_multi_payment::AcceptedCurrencyPrice::<hydradx_runtime::Runtime>::insert(
+			DOT,
+			FixedU128::from_rational(50, 100),
+		);
+
+		let alice: AccountId = ALICE.into();
+		let amount = 10 * UNITS;
+		let alice_dot_before = Currencies::free_balance(DOT, &alice);
+		assert!(alice_dot_before >= amount);
+
+		//Act
+		let message = xcm_message_withdraw_deposit(Location::parent(), amount);
+		let call = RuntimeCall::PolkadotXcm(pallet_xcm::Call::execute {
+			message: Box::new(VersionedXcm::from(message)),
+			max_weight: Weight::from_parts(1_000_000_000_000, 0),
+		});
+
+		assert_ok!(call.dispatch(hydradx_runtime::RuntimeOrigin::signed(ALICE.into())));
+
+		//Assert
+		assert!(
+			Currencies::free_balance(DOT, &alice) <= alice_dot_before - amount,
+			"DOT must have been withdrawn from Alice for the outbound XCM; before={alice_dot_before}, after={}",
+			Currencies::free_balance(DOT, &alice)
+		);
+	});
+}
+
+#[test]
+fn withdraw_succeeds_for_asset_in_overrides_but_not_in_accepted_currencies() {
+	TestNet::reset();
+	Hydra::execute_with(|| {
+		init_global_withdraw_limit_params();
+		init_omnipool_with_oracle_for_block_10();
+
+		assert_ok!(CircuitBreaker::set_asset_category(
+			hydradx_runtime::RuntimeOrigin::root(),
+			DOT,
+			Some(GlobalAssetCategory::External),
+		));
+		assert_ok!(hydradx_runtime::AssetRegistry::set_location(DOT, DOT_ASSET_LOCATION));
+
+		assert!(
+			!pallet_transaction_multi_payment::AcceptedCurrencies::<hydradx_runtime::Runtime>::contains_key(DOT),
+			"precondition: DOT must not be an accepted fee currency",
+		);
+		assert!(
+			pallet_transaction_multi_payment::AcceptedCurrencyPrice::<hydradx_runtime::Runtime>::get(DOT).is_none(),
+			"precondition: DOT must not be in the multi-payment price cache",
+		);
+
+		let alice: AccountId = ALICE.into();
+		let amount = 10 * UNITS;
+		let alice_dot_before = Currencies::free_balance(DOT, &alice);
+		assert!(alice_dot_before >= amount);
+		let acc_before = CircuitBreaker::withdraw_limit_accumulator().0;
+
+		// Act
+		assert_ok!(Currencies::withdraw(
+			DOT,
+			&alice,
+			amount,
+			frame_support::traits::ExistenceRequirement::AllowDeath,
+		));
+
+		// Assert
+		assert_eq!(
+			Currencies::free_balance(DOT, &alice),
+			alice_dot_before - amount,
+			"DOT balance must reflect the withdraw",
+		);
+		assert!(
+			CircuitBreaker::withdraw_limit_accumulator().0 > acc_before,
+			"Global accumulator must increase — only the ConvertBalance fallback could have priced this withdraw",
+		);
+	});
+}

integration-tests/src/global_withdraw_limit.rs

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

runtime/hydradx/Cargo.toml

@@ -429,7 +429,7 @@ impl pallet_currencies::Config for Runtime {
 	type ReserveAccount = ReserveAccount;
 	type GetNativeCurrencyId = NativeAssetId;
 	type RegistryInspect = AssetRegistry;
-	type EgressHandler = circuit_breaker::WithdrawLimitHandler<NativeAssetId>;
+	type EgressHandler = circuit_breaker::WithdrawLimitHandler;
 	type WeightInfo = weights::pallet_currencies::HydraWeight<Runtime>;
 }
 

runtime/hydradx/src/assets.rs

@@ -7,33 +7,39 @@ use pallet_asset_registry::AssetType;
 use pallet_circuit_breaker::types::EgressOperationKind;
 use pallet_circuit_breaker::GlobalAssetCategory;
 use primitives::Balance;
+use sp_runtime::helpers_128bit::multiply_by_rational_with_rounding;
 use sp_runtime::traits::Convert;
-use sp_runtime::DispatchResult;
+use sp_runtime::{DispatchResult, FixedPointNumber, FixedU128, Rounding};
 use sp_std::marker::PhantomData;
 
-pub struct WithdrawLimitHandler<RC>(PhantomData<RC>);
-impl<RC: Get<AssetId>> AssetWithdrawHandler<AccountId, AssetId, Balance> for WithdrawLimitHandler<RC> {
-	type OnWithdraw = OnWithdrawHook<RC>;
-	type OnDeposit = OnDepositHook<RC>;
-	type OnTransfer = OnTransferHook<RC>;
+pub struct WithdrawLimitHandler;
+impl AssetWithdrawHandler<AccountId, AssetId, Balance> for WithdrawLimitHandler {
+	type OnWithdraw = OnWithdrawHook;
+	type OnDeposit = OnDepositHook;
+	type OnTransfer = OnTransferHook;
 }
 
-pub struct WithdrawCircuitBreaker<ReferenceCurrencyId>(PhantomData<ReferenceCurrencyId>);
-impl<ReferenceCurrencyId: Get<AssetId>> WithdrawCircuitBreaker<ReferenceCurrencyId> {
+pub struct WithdrawCircuitBreaker;
+impl WithdrawCircuitBreaker {
 	fn convert_to_hdx(asset_id: AssetId, amount: Balance) -> Result<Balance, DispatchError> {
-		let ref_currency = ReferenceCurrencyId::get();
+		let ref_currency = NativeAssetId::get();
 		if asset_id == ref_currency {
 			return Ok(amount);
 		}
 
-		let (converted, _) = ConvertBalance::<TenMinutesOraclePrice, XykPaymentAssetSupport, DotAssetId>::convert((
-			asset_id,
-			ref_currency,
-			amount,
-		))
-		.ok_or(pallet_circuit_breaker::Error::<Runtime>::FailedToConvertAsset)?;
-
-		Ok(converted)
+		MultiTransactionPayment::currency_price(asset_id)
+			.and_then(|price| {
+				multiply_by_rational_with_rounding(amount, FixedU128::DIV, price.into_inner(), Rounding::Up)
+			})
+			.or_else(|| {
+				ConvertBalance::<TenMinutesOraclePrice, XykPaymentAssetSupport, DotAssetId>::convert((
+					asset_id,
+					ref_currency,
+					amount,
+				))
+				.map(|(converted, _)| converted)
+			})
+			.ok_or_else(|| pallet_circuit_breaker::Error::<Runtime>::FailedToConvertAsset.into())
 	}
 
 	pub fn global_asset_category(asset_id: AssetId) -> Option<GlobalAssetCategory> {
@@ -97,22 +103,18 @@ impl<ReferenceCurrencyId: Get<AssetId>> WithdrawCircuitBreaker<ReferenceCurrency
 	}
 }
 
-pub struct OnWithdrawHook<RC>(PhantomData<RC>);
-impl<RC: Get<AssetId>> orml_traits::Handler<(AssetId, Balance)> for OnWithdrawHook<RC> {
+pub struct OnWithdrawHook;
+impl orml_traits::Handler<(AssetId, Balance)> for OnWithdrawHook {
 	fn handle(t: &(AssetId, Balance)) -> DispatchResult {
 		// `who` is not used: in XCM path all withdrawals go to buffer regardless of origin;
 		// in non-XCM path Withdraw is always accounted for both Local and External assets regardless of who.
 		let (asset_id, amount) = t;
 
-		if !WithdrawCircuitBreaker::<RC>::should_account_withdraw_operation(
-			*asset_id,
-			EgressOperationKind::Withdraw,
-			None,
-		) {
+		if !WithdrawCircuitBreaker::should_account_withdraw_operation(*asset_id, EgressOperationKind::Withdraw, None) {
 			return Ok(());
 		}
 
-		let amount_ref_currency = WithdrawCircuitBreaker::<RC>::convert_to_hdx(*asset_id, *amount)?;
+		let amount_ref_currency = WithdrawCircuitBreaker::convert_to_hdx(*asset_id, *amount)?;
 
 		if let Some(mut buffer) = pallet_circuit_breaker::XcmEgressBuffer::<Runtime>::get() {
 			buffer.0 = buffer.0.saturating_add(amount_ref_currency);
@@ -125,8 +127,8 @@ impl<RC: Get<AssetId>> orml_traits::Handler<(AssetId, Balance)> for OnWithdrawHo
 	}
 }
 
-pub struct OnTransferHook<RC>(PhantomData<RC>);
-impl<RC: Get<AssetId>> orml_traits::currency::OnTransfer<AccountId, AssetId, Balance> for OnTransferHook<RC> {
+pub struct OnTransferHook;
+impl orml_traits::currency::OnTransfer<AccountId, AssetId, Balance> for OnTransferHook {
 	fn on_transfer(asset_id: AssetId, from: &AccountId, to: &AccountId, amount: Balance) -> DispatchResult {
 		let is_from_egress = CircuitBreaker::is_account_egress(from).is_some();
 		let is_to_egress = CircuitBreaker::is_account_egress(to).is_some();
@@ -135,13 +137,10 @@ impl<RC: Get<AssetId>> orml_traits::currency::OnTransfer<AccountId, AssetId, Bal
 			return Ok(());
 		}
 
-		let try_convert = || WithdrawCircuitBreaker::<RC>::convert_to_hdx(asset_id, amount);
+		let try_convert = || WithdrawCircuitBreaker::convert_to_hdx(asset_id, amount);
 
-		if WithdrawCircuitBreaker::<RC>::should_account_withdraw_operation(
-			asset_id,
-			EgressOperationKind::Transfer,
-			Some(to),
-		) {
+		if WithdrawCircuitBreaker::should_account_withdraw_operation(asset_id, EgressOperationKind::Transfer, Some(to))
+		{
 			let amount_ref_currency = try_convert()?;
 			pallet_circuit_breaker::Pallet::<Runtime>::note_egress(amount_ref_currency)?;
 		}
@@ -151,7 +150,7 @@ impl<RC: Get<AssetId>> orml_traits::currency::OnTransfer<AccountId, AssetId, Bal
 		// accounts are never ingress — no tokens arrived from another chain)
 		if is_from_egress
 			&& matches!(
-				WithdrawCircuitBreaker::<RC>::global_asset_category(asset_id),
+				WithdrawCircuitBreaker::global_asset_category(asset_id),
 				Some(GlobalAssetCategory::Local)
 			) {
 			if let Ok(amount_ref_currency) = try_convert() {
@@ -162,8 +161,8 @@ impl<RC: Get<AssetId>> orml_traits::currency::OnTransfer<AccountId, AssetId, Bal
 	}
 }
 
-pub struct OnDepositHook<RC>(PhantomData<RC>);
-impl<RC: Get<AssetId>> orml_traits::Handler<(AssetId, Balance, Option<AccountId>)> for OnDepositHook<RC> {
+pub struct OnDepositHook;
+impl orml_traits::Handler<(AssetId, Balance, Option<AccountId>)> for OnDepositHook {
 	fn handle(t: &(AssetId, Balance, Option<AccountId>)) -> DispatchResult {
 		let (asset_id, amount, maybe_dest) = t;
 
@@ -176,14 +175,14 @@ impl<RC: Get<AssetId>> orml_traits::Handler<(AssetId, Balance, Option<AccountId>
 		// Outside XCM, use the stricter should_account_deposit_operation check which,
 		// for Local assets, requires the source to be an egress account.
 		if buffer_active {
-			if WithdrawCircuitBreaker::<RC>::global_asset_category(*asset_id).is_none() {
+			if WithdrawCircuitBreaker::global_asset_category(*asset_id).is_none() {
 				return Ok(());
 			}
-		} else if !WithdrawCircuitBreaker::<RC>::should_account_deposit_operation(*asset_id, maybe_dest.clone()) {
+		} else if !WithdrawCircuitBreaker::should_account_deposit_operation(*asset_id, maybe_dest.clone()) {
 			return Ok(());
 		}
 
-		let Ok(amount_ref_currency) = WithdrawCircuitBreaker::<RC>::convert_to_hdx(*asset_id, *amount) else {
+		let Ok(amount_ref_currency) = WithdrawCircuitBreaker::convert_to_hdx(*asset_id, *amount) else {
 			return Ok(());
 		};
 

runtime/hydradx/src/circuit_breaker.rs

@@ -128,7 +128,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: Cow::Borrowed("hydradx"),
 	impl_name: Cow::Borrowed("hydradx"),
 	authoring_version: 1,
-	spec_version: 413,
+	spec_version: 414,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,

runtime/hydradx/src/lib.rs

@@ -690,9 +690,8 @@ impl TransactAsset for LocalAssetTransactor {
 				CurrencyIdConvert::convert(what.clone()),
 				IsNativeConcrete::<AssetId, CurrencyIdConvert>::matches_fungible(what),
 			) {
-				crate::circuit_breaker::WithdrawCircuitBreaker::<NativeAssetId>::ensure_inbound_xcm_withdraw_can_proceed(
-					asset_id,
-					amount,
+				crate::circuit_breaker::WithdrawCircuitBreaker::ensure_inbound_xcm_withdraw_can_proceed(
+					asset_id, amount,
 				)
 				.map_err(|e| XcmError::FailedToTransactAsset(e.into()))?;
 			}