Merge pull request #1474 from galacticcouncil/fix/referrals-claim-dust-revert

fix: don't revert claim_rewards on un-convertible dust

Author: enthusiastmartin

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "pallet-referrals"
-version = "1.6.0"
+version = "1.6.1"
 authors = ['GalacticCouncil']
 edition = "2021"
 license = "Apache-2.0"

pallets/referrals/Cargo.toml

@@ -298,6 +298,11 @@ pub mod pallet {
 			from: AssetAmount<T::AssetId>,
 			to: AssetAmount<T::AssetId>,
 		},
+		/// A pending asset could not be converted; skipped, not blocking.
+		ConversionFailed {
+			asset_id: T::AssetId,
+			reason: DispatchError,
+		},
 		/// Rewards claimed.
 		Claimed {
 			who: T::AccountId,
@@ -482,20 +487,18 @@ pub mod pallet {
 			let who = ensure_signed(origin)?;
 			for (asset_id, _) in PendingConversions::<T>::iter() {
 				let asset_balance = T::Currency::balance(asset_id.clone(), &Self::pot_account_id());
-				let r = T::Convert::convert(
+				// Best-effort, like `on_idle`: skip an un-convertible slice rather than revert the
+				// whole claim. Funds stay in the pot and re-queue on the next fee.
+				if let Err(reason) = T::Convert::convert(
 					Self::pot_account_id(),
 					asset_id.clone(),
 					T::RewardAsset::get(),
 					asset_balance,
-				);
-				if let Err(error) = r {
-					// We allow these errors to continue claiming as the current amount of asset that needed to be converted
-					// has very low impact on the rewards.
-					if error != Error::<T>::ConversionMinTradingAmountNotReached.into()
-						&& error != Error::<T>::ConversionZeroAmountReceived.into()
-					{
-						return Err(error);
-					}
+				) {
+					Self::deposit_event(Event::ConversionFailed {
+						asset_id: asset_id.clone(),
+						reason,
+					});
 				}
 				PendingConversions::<T>::remove(asset_id);
 			}
@@ -623,12 +626,17 @@ pub mod pallet {
 			for asset_id in PendingConversions::<T>::iter_keys().take(max_converts as usize) {
 				let asset_balance = T::Currency::balance(asset_id.clone(), &Self::pot_account_id());
 				// remove the asset_id from PendingConversions even when the conversion fails
-				let _ = T::Convert::convert(
+				if let Err(reason) = T::Convert::convert(
 					Self::pot_account_id(),
 					asset_id.clone(),
 					T::RewardAsset::get(),
 					asset_balance,
-				);
+				) {
+					Self::deposit_event(Event::ConversionFailed {
+						asset_id: asset_id.clone(),
+						reason,
+					});
+				}
 				PendingConversions::<T>::remove(asset_id);
 				actual_converts += 1;
 			}

pallets/referrals/src/lib.rs

@@ -70,6 +70,8 @@ thread_local! {
 	pub static TIER_VOLUME: RefCell<HashMap<Level, Option<Balance>>> = RefCell::new(HashMap::default());
 	pub static TIER_REWARDS: RefCell<HashMap<Level, FeeDistribution>> = RefCell::new(HashMap::default());
 	pub static SEED_AMOUNT: RefCell<Balance> = RefCell::new(Balance::zero());
+	// Mirrors runtime `MinTradingLimit`: reject sub-minimum amounts. 0 = disabled.
+	pub static CONVERT_MIN_AMOUNT: RefCell<Balance> = RefCell::new(Balance::zero());
 }
 
 construct_runtime!(
@@ -218,6 +220,10 @@ impl Default for ExtBuilder {
 			v.borrow_mut().clear();
 		});
 
+		CONVERT_MIN_AMOUNT.with(|v| {
+			*v.borrow_mut() = 0u128;
+		});
+
 		Self {
 			endowed_accounts: vec![(ALICE, HDX, INITIAL_ALICE_BALANCE)],
 			referrer_shares: vec![],
@@ -260,6 +266,12 @@ impl ExtBuilder {
 		});
 		self
 	}
+	pub fn with_convert_min_amount(self, amount: Balance) -> Self {
+		CONVERT_MIN_AMOUNT.with(|v| {
+			*v.borrow_mut() = amount;
+		});
+		self
+	}
 	pub fn with_seed_amount(self, amount: Balance) -> Self {
 		SEED_AMOUNT.with(|v| {
 			let mut m = v.borrow_mut();
@@ -364,6 +376,11 @@ impl Convert<AccountId, AssetId, Balance> for AssetConvert {
 		asset_to: AssetId,
 		amount: Balance,
 	) -> Result<Balance, Self::Error> {
+		let min_amount = CONVERT_MIN_AMOUNT.with(|v| *v.borrow());
+		if amount > 0 && amount < min_amount {
+			// Like `ConvertViaOmnipool`: sub-min amounts fail with a non-referrals error.
+			return Err(DispatchError::Other("InsufficientTradingAmount"));
+		}
 		let price = CONVERSION_RATE
 			.with(|v| v.borrow().get(&(asset_to, asset_from)).copied())
 			.ok_or(Error::<Test>::ConversionMinTradingAmountNotReached)?;

pallets/referrals/src/tests.rs

@@ -74,6 +74,40 @@ fn claim_rewards_should_remove_assets_from_the_list_when_not_successful() {
 		});
 }
 
+// Regression: sub-`MinTradingLimit` dust in `PendingConversions` must not block reward claims.
+#[test]
+fn claim_rewards_should_succeed_when_pending_asset_balance_is_below_min_trading_limit() {
+	ExtBuilder::default()
+		.with_endowed_accounts(vec![
+			(Pallet::<Test>::pot_account_id(), HDX, 20_000_000_000_000),
+			// 500 raw units of DAI — below the converter's min trading limit (1_000).
+			(Pallet::<Test>::pot_account_id(), DAI, 500),
+		])
+		.with_referrer_shares(vec![(BOB, 5_000_000_000_000), (ALICE, 15_000_000_000_000)])
+		.with_assets(vec![DAI])
+		// Price present so the converter reaches the min-amount check (not the no-price branch,
+		// whose error the claim loop happens to tolerate).
+		.with_conversion_price((HDX, DAI), EmaPrice::new(1_000_000_000_000, 1_000_000_000_000_000_000))
+		.with_convert_min_amount(1_000)
+		.build()
+		.execute_with(|| {
+			assert_eq!(PendingConversions::<Test>::count(), 1);
+
+			// Bob can still claim his share despite the un-convertible dust asset.
+			assert_ok!(Referrals::claim_rewards(RuntimeOrigin::signed(BOB)));
+
+			// Bob received his portion of the HDX reward reserve.
+			assert_eq!(Tokens::free_balance(HDX, &BOB), 5_000_000_000_000);
+			// The dust entry is dropped rather than left to block future claims.
+			assert_eq!(PendingConversions::<Test>::count(), 0);
+			// The skipped conversion is surfaced as an event, not silently swallowed.
+			assert!(System::events().iter().any(|r| matches!(
+				&r.event,
+				RuntimeEvent::Referrals(Event::ConversionFailed { asset_id, .. }) if *asset_id == DAI
+			)));
+		});
+}
+
 #[test]
 fn claim_rewards_should_calculate_correct_portion_when_claimed() {
 	ExtBuilder::default()