From 6169fe9f942807d6061cf00770be9319cce19300 Mon Sep 17 00:00:00 2001 From: mrq Date: Tue, 9 Jun 2026 22:50:01 +0200 Subject: [PATCH 1/3] stop bricking referral claims on un-convertible dust --- Cargo.lock | 4 ++-- pallets/referrals/Cargo.toml | 2 +- pallets/referrals/src/lib.rs | 14 ++++-------- pallets/referrals/src/tests.rs | 21 ++++++++++++++++++ pallets/referrals/src/tests/claim.rs | 33 ++++++++++++++++++++++++++++ runtime/hydradx/Cargo.toml | 2 +- runtime/hydradx/src/lib.rs | 2 +- 7 files changed, 63 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 82dcef6699..b636e13d3b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5780,7 +5780,7 @@ dependencies = [ [[package]] name = "hydradx-runtime" -version = "426.0.0" +version = "427.0.0" dependencies = [ "cumulus-pallet-aura-ext", "cumulus-pallet-parachain-system", @@ -10388,7 +10388,7 @@ dependencies = [ [[package]] name = "pallet-referrals" -version = "1.6.0" +version = "1.6.1" dependencies = [ "frame-benchmarking", "frame-support", diff --git a/pallets/referrals/Cargo.toml b/pallets/referrals/Cargo.toml index 81555e732c..2cf39dc97c 100644 --- a/pallets/referrals/Cargo.toml +++ b/pallets/referrals/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pallet-referrals" -version = "1.6.0" +version = "1.6.1" authors = ['GalacticCouncil'] edition = "2021" license = "Apache-2.0" diff --git a/pallets/referrals/src/lib.rs b/pallets/referrals/src/lib.rs index 4c18ab2625..aa67f28b88 100644 --- a/pallets/referrals/src/lib.rs +++ b/pallets/referrals/src/lib.rs @@ -482,21 +482,15 @@ pub mod pallet { let who = ensure_signed(origin)?; for (asset_id, _) in PendingConversions::::iter() { let asset_balance = T::Currency::balance(asset_id.clone(), &Self::pot_account_id()); - let r = T::Convert::convert( + // Best-effort, matching `on_idle`: a slice that can't be converted (e.g. below the + // converter's min trading limit) is skipped, not fatal — the funds stay in the pot + // and the entry is dropped so it can't block this or anyone else's claim. + let _ = 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::::ConversionMinTradingAmountNotReached.into() - && error != Error::::ConversionZeroAmountReceived.into() - { - return Err(error); - } - } PendingConversions::::remove(asset_id); } let referrer_shares = ReferrerShares::::take(&who); diff --git a/pallets/referrals/src/tests.rs b/pallets/referrals/src/tests.rs index 3de7827923..c839098ef6 100644 --- a/pallets/referrals/src/tests.rs +++ b/pallets/referrals/src/tests.rs @@ -70,6 +70,10 @@ thread_local! { pub static TIER_VOLUME: RefCell>> = RefCell::new(HashMap::default()); pub static TIER_REWARDS: RefCell> = RefCell::new(HashMap::default()); pub static SEED_AMOUNT: RefCell = RefCell::new(Balance::zero()); + // Mirrors the runtime `MinTradingLimit`: `AssetConvert` rejects sub-minimum amounts with a + // non-referrals error, exactly as `ConvertViaOmnipool` -> `Omnipool::sell` does in production. + // 0 disables the gate so existing tests are unaffected. + pub static CONVERT_MIN_AMOUNT: RefCell = RefCell::new(Balance::zero()); } construct_runtime!( @@ -218,6 +222,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 +268,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 +378,13 @@ impl Convert for AssetConvert { asset_to: AssetId, amount: Balance, ) -> Result { + let min_amount = CONVERT_MIN_AMOUNT.with(|v| *v.borrow()); + if amount > 0 && amount < min_amount { + // Production parity: the omnipool-backed converter rejects sub-`MinTradingLimit` + // amounts with `pallet_omnipool::Error::InsufficientTradingAmount` — an error that + // is *not* one of the two referrals-specific variants `claim_rewards` tolerates. + return Err(DispatchError::Other("InsufficientTradingAmount")); + } let price = CONVERSION_RATE .with(|v| v.borrow().get(&(asset_to, asset_from)).copied()) .ok_or(Error::::ConversionMinTradingAmountNotReached)?; diff --git a/pallets/referrals/src/tests/claim.rs b/pallets/referrals/src/tests/claim.rs index 0f680ccb02..f0072a708b 100644 --- a/pallets/referrals/src/tests/claim.rs +++ b/pallets/referrals/src/tests/claim.rs @@ -74,6 +74,39 @@ fn claim_rewards_should_remove_assets_from_the_list_when_not_successful() { }); } +// Regression: a sub-`MinTradingLimit` dust balance of a non-reward asset in `PendingConversions` +// must not block reward claims. The converter rejects the dust with a non-referrals error +// (`InsufficientTradingAmount`), which the claim loop only tolerates for its own two legacy +// variants — so today it reverts the whole claim instead of skipping the dust like `on_idle` +// does. This test asserts the desired self-healing behaviour and currently FAILS, proving the bug. +#[test] +fn claim_rewards_should_succeed_when_pending_asset_balance_is_below_min_trading_limit() { + ExtBuilder::default() + .with_endowed_accounts(vec![ + (Pallet::::pot_account_id(), HDX, 20_000_000_000_000), + // 500 raw units of DAI — below the converter's min trading limit (1_000). + (Pallet::::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::::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::::count(), 0); + }); +} + #[test] fn claim_rewards_should_calculate_correct_portion_when_claimed() { ExtBuilder::default() diff --git a/runtime/hydradx/Cargo.toml b/runtime/hydradx/Cargo.toml index 42cb4ede0d..bc8a98828c 100644 --- a/runtime/hydradx/Cargo.toml +++ b/runtime/hydradx/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "hydradx-runtime" -version = "426.0.0" +version = "427.0.0" authors = ["GalacticCouncil"] edition = "2021" license = "Apache 2.0" diff --git a/runtime/hydradx/src/lib.rs b/runtime/hydradx/src/lib.rs index 3da81766b4..a1046eebef 100644 --- a/runtime/hydradx/src/lib.rs +++ b/runtime/hydradx/src/lib.rs @@ -129,7 +129,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: Cow::Borrowed("hydradx"), impl_name: Cow::Borrowed("hydradx"), authoring_version: 1, - spec_version: 426, + spec_version: 427, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, From c09e025f280274dba66c16fef64cbf4d84d89544 Mon Sep 17 00:00:00 2001 From: mrq Date: Tue, 9 Jun 2026 22:54:54 +0200 Subject: [PATCH 2/3] shorter comments, fmt --- pallets/referrals/src/lib.rs | 5 ++--- pallets/referrals/src/tests.rs | 8 ++------ pallets/referrals/src/tests/claim.rs | 6 +----- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/pallets/referrals/src/lib.rs b/pallets/referrals/src/lib.rs index aa67f28b88..bcf049cbbc 100644 --- a/pallets/referrals/src/lib.rs +++ b/pallets/referrals/src/lib.rs @@ -482,9 +482,8 @@ pub mod pallet { let who = ensure_signed(origin)?; for (asset_id, _) in PendingConversions::::iter() { let asset_balance = T::Currency::balance(asset_id.clone(), &Self::pot_account_id()); - // Best-effort, matching `on_idle`: a slice that can't be converted (e.g. below the - // converter's min trading limit) is skipped, not fatal — the funds stay in the pot - // and the entry is dropped so it can't block this or anyone else's claim. + // 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. let _ = T::Convert::convert( Self::pot_account_id(), asset_id.clone(), diff --git a/pallets/referrals/src/tests.rs b/pallets/referrals/src/tests.rs index c839098ef6..283968fe9b 100644 --- a/pallets/referrals/src/tests.rs +++ b/pallets/referrals/src/tests.rs @@ -70,9 +70,7 @@ thread_local! { pub static TIER_VOLUME: RefCell>> = RefCell::new(HashMap::default()); pub static TIER_REWARDS: RefCell> = RefCell::new(HashMap::default()); pub static SEED_AMOUNT: RefCell = RefCell::new(Balance::zero()); - // Mirrors the runtime `MinTradingLimit`: `AssetConvert` rejects sub-minimum amounts with a - // non-referrals error, exactly as `ConvertViaOmnipool` -> `Omnipool::sell` does in production. - // 0 disables the gate so existing tests are unaffected. + // Mirrors runtime `MinTradingLimit`: reject sub-minimum amounts. 0 = disabled. pub static CONVERT_MIN_AMOUNT: RefCell = RefCell::new(Balance::zero()); } @@ -380,9 +378,7 @@ impl Convert for AssetConvert { ) -> Result { let min_amount = CONVERT_MIN_AMOUNT.with(|v| *v.borrow()); if amount > 0 && amount < min_amount { - // Production parity: the omnipool-backed converter rejects sub-`MinTradingLimit` - // amounts with `pallet_omnipool::Error::InsufficientTradingAmount` — an error that - // is *not* one of the two referrals-specific variants `claim_rewards` tolerates. + // Like `ConvertViaOmnipool`: sub-min amounts fail with a non-referrals error. return Err(DispatchError::Other("InsufficientTradingAmount")); } let price = CONVERSION_RATE diff --git a/pallets/referrals/src/tests/claim.rs b/pallets/referrals/src/tests/claim.rs index f0072a708b..03d01fb1c5 100644 --- a/pallets/referrals/src/tests/claim.rs +++ b/pallets/referrals/src/tests/claim.rs @@ -74,11 +74,7 @@ fn claim_rewards_should_remove_assets_from_the_list_when_not_successful() { }); } -// Regression: a sub-`MinTradingLimit` dust balance of a non-reward asset in `PendingConversions` -// must not block reward claims. The converter rejects the dust with a non-referrals error -// (`InsufficientTradingAmount`), which the claim loop only tolerates for its own two legacy -// variants — so today it reverts the whole claim instead of skipping the dust like `on_idle` -// does. This test asserts the desired self-healing behaviour and currently FAILS, proving the bug. +// 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() From 9bed6c4a359dba826fa3af6d24179695577b50c2 Mon Sep 17 00:00:00 2001 From: mrq Date: Tue, 9 Jun 2026 22:59:46 +0200 Subject: [PATCH 3/3] emit ConversionFailed instead of swallowing silently --- pallets/referrals/src/lib.rs | 23 +++++++++++++++++++---- pallets/referrals/src/tests/claim.rs | 5 +++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/pallets/referrals/src/lib.rs b/pallets/referrals/src/lib.rs index bcf049cbbc..4434e5dec3 100644 --- a/pallets/referrals/src/lib.rs +++ b/pallets/referrals/src/lib.rs @@ -298,6 +298,11 @@ pub mod pallet { from: AssetAmount, to: AssetAmount, }, + /// A pending asset could not be converted; skipped, not blocking. + ConversionFailed { + asset_id: T::AssetId, + reason: DispatchError, + }, /// Rewards claimed. Claimed { who: T::AccountId, @@ -484,12 +489,17 @@ pub mod pallet { let asset_balance = T::Currency::balance(asset_id.clone(), &Self::pot_account_id()); // 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. - 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::::remove(asset_id); } let referrer_shares = ReferrerShares::::take(&who); @@ -616,12 +626,17 @@ pub mod pallet { for asset_id in PendingConversions::::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::::remove(asset_id); actual_converts += 1; } diff --git a/pallets/referrals/src/tests/claim.rs b/pallets/referrals/src/tests/claim.rs index 03d01fb1c5..44e063f2d4 100644 --- a/pallets/referrals/src/tests/claim.rs +++ b/pallets/referrals/src/tests/claim.rs @@ -100,6 +100,11 @@ fn claim_rewards_should_succeed_when_pending_asset_balance_is_below_min_trading_ 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::::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 + ))); }); }