From 1bf8d7425a79b58dc7f002b70bdd3d30f29924e5 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Mon, 16 Dec 2024 14:20:36 +1100 Subject: [PATCH] chore: address review --- state-chain/custom-rpc/src/lib.rs | 8 +- ...pc__test__boost_details_serialization.snap | 4 +- .../cf-ingress-egress/src/boost_pool.rs | 8 +- .../cf-ingress-egress/src/boost_pool/tests.rs | 89 +++++++++++++++---- .../pallets/cf-ingress-egress/src/lib.rs | 28 +++--- .../pallets/cf-ingress-egress/src/tests.rs | 8 +- .../cf-ingress-egress/src/tests/boost.rs | 8 +- state-chain/runtime/src/lib.rs | 4 +- state-chain/runtime/src/runtime_apis.rs | 2 +- 9 files changed, 109 insertions(+), 50 deletions(-) diff --git a/state-chain/custom-rpc/src/lib.rs b/state-chain/custom-rpc/src/lib.rs index c5bf9f2dad..eadaadf9b9 100644 --- a/state-chain/custom-rpc/src/lib.rs +++ b/state-chain/custom-rpc/src/lib.rs @@ -565,7 +565,7 @@ mod boost_pool_rpc { available_amounts: Vec, deposits_pending_finalization: Vec, pending_withdrawals: Vec, - network_fee_deduction_percents: Percent, + network_fee_deduction_percent: Percent, } impl BoostPoolDetailsRpc { @@ -603,7 +603,7 @@ mod boost_pool_rpc { pending_deposits, }) .collect(), - network_fee_deduction_percents: details.network_fee_deduction_percents, + network_fee_deduction_percent: details.network_fee_deduction_percent, } } } @@ -2424,7 +2424,7 @@ mod test { (1, BTreeMap::from([(ID_1.clone(), OwedAmount { total: 1_000, fee: 50 })])), ]), pending_withdrawals: Default::default(), - network_fee_deduction_percents: Percent::from_percent(40), + network_fee_deduction_percent: Percent::from_percent(40), } } @@ -2442,7 +2442,7 @@ mod test { (ID_1.clone(), BTreeSet::from([0])), (ID_2.clone(), BTreeSet::from([0])), ]), - network_fee_deduction_percents: Percent::from_percent(0), + network_fee_deduction_percent: Percent::from_percent(0), } } diff --git a/state-chain/custom-rpc/src/snapshots/custom_rpc__test__boost_details_serialization.snap b/state-chain/custom-rpc/src/snapshots/custom_rpc__test__boost_details_serialization.snap index 5e9feffd63..a3c25337b5 100644 --- a/state-chain/custom-rpc/src/snapshots/custom_rpc__test__boost_details_serialization.snap +++ b/state-chain/custom-rpc/src/snapshots/custom_rpc__test__boost_details_serialization.snap @@ -38,7 +38,7 @@ expression: val } ], "pending_withdrawals": [], - "network_fee_deduction_percents": 40 + "network_fee_deduction_percent": 40 }, { "fee_tier": 30, @@ -74,6 +74,6 @@ expression: val ] } ], - "network_fee_deduction_percents": 0 + "network_fee_deduction_percent": 0 } ] diff --git a/state-chain/pallets/cf-ingress-egress/src/boost_pool.rs b/state-chain/pallets/cf-ingress-egress/src/boost_pool.rs index dd40af3b6a..1415c0d26b 100644 --- a/state-chain/pallets/cf-ingress-egress/src/boost_pool.rs +++ b/state-chain/pallets/cf-ingress-egress/src/boost_pool.rs @@ -152,7 +152,7 @@ where AccountId: PartialEq + core::fmt::Debug, { pub unlocked_funds: Vec<(AccountId, C::ChainAmount)>, - pub amount_credited: C::ChainAmount, + pub amount_credited_to_boosters: C::ChainAmount, } impl BoostPool @@ -250,8 +250,8 @@ where (provided_amount, fee) }; - // NOTE: only a portion of the boost fee goes to the boost pool, - // the rest is charged as network fee: + // NOTE: before the boost fee is credited to the boost pool, a portion + // of it is deducted as network fee: let network_fee = network_fee_deduction * u128::from(fee_amount); let boost_pool_fee = fee_amount.saturating_sub(ScaledAmount::from(network_fee)); @@ -393,7 +393,7 @@ where DepositFinalisationOutcomeForPool { unlocked_funds, - amount_credited: amount_credited.into_chain_amount(), + amount_credited_to_boosters: amount_credited.into_chain_amount(), } } diff --git a/state-chain/pallets/cf-ingress-egress/src/boost_pool/tests.rs b/state-chain/pallets/cf-ingress-egress/src/boost_pool/tests.rs index b30941c5ba..9627deca9a 100644 --- a/state-chain/pallets/cf-ingress-egress/src/boost_pool/tests.rs +++ b/state-chain/pallets/cf-ingress-egress/src/boost_pool/tests.rs @@ -1,6 +1,6 @@ use super::*; use cf_chains::Ethereum; -use cf_primitives::{AssetAmount, EthAmount, FLIPPERINOS_PER_FLIP}; +use cf_primitives::{AssetAmount, EthAmount, FLIPPERINOS_PER_FLIP, MAX_BASIS_POINTS}; use sp_std::collections::btree_set::BTreeSet; @@ -172,17 +172,64 @@ fn boosting_with_fees() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), - DepositFinalisationOutcomeForPool { amount_credited: 1010, unlocked_funds: vec![] } + DepositFinalisationOutcomeForPool { + amount_credited_to_boosters: 1010, + unlocked_funds: vec![] + } ); check_pool(&pool, [(BOOSTER_1, 1003), (BOOSTER_2, 2006)]); } +#[test] +fn boosting_with_max_network_fee_deduction() { + const BOOST_FEE_BPS: u16 = 100; + const INIT_BOOSTER_AMOUNT: u128 = 2000; + const NETWORK_FEE_PORTION_PERCENT: u8 = 100; + + let mut pool = TestPool::new(BOOST_FEE_BPS); + + pool.add_funds(BOOSTER_1, INIT_BOOSTER_AMOUNT); + + check_pool(&pool, [(BOOSTER_1, INIT_BOOSTER_AMOUNT)]); + + const PROVIDED_AMOUNT: u128 = 1000; + const FULL_BOOST_FEE: u128 = + (PROVIDED_AMOUNT * BOOST_FEE_BPS as u128) / MAX_BASIS_POINTS as u128; + + const DEPOSIT_AMOUNT: u128 = PROVIDED_AMOUNT + FULL_BOOST_FEE; + + // NOTE: Full 1% boost fee is charged from the deposit + assert_eq!( + pool.provide_funds_for_boosting( + BOOST_1, + DEPOSIT_AMOUNT, + Percent::from_percent(NETWORK_FEE_PORTION_PERCENT) + ), + Ok((DEPOSIT_AMOUNT, FULL_BOOST_FEE)) + ); + + // Booster's contribution is recorded, but they earn 0 fees: + check_pending_boosts(&pool, [(BOOST_1, vec![(BOOSTER_1, 1000, 0)])]); + + assert_eq!( + pool.process_deposit_as_finalised(BOOST_1), + DepositFinalisationOutcomeForPool { + amount_credited_to_boosters: 0, + unlocked_funds: vec![] + } + ); + + // No change in the boost pool after deposit is finalised: + check_pool(&pool, [(BOOSTER_1, INIT_BOOSTER_AMOUNT)]); +} + #[test] fn boosting_with_fees_including_network_fee_portion() { const NETWORK_FEE_PORTION_PERCENT: u8 = 30; + const BOOST_FEE_BPS: u16 = 100; - let mut pool = TestPool::new(100); // 1% + let mut pool = TestPool::new(BOOST_FEE_BPS); pool.add_funds(BOOSTER_1, 1000); pool.add_funds(BOOSTER_2, 2000); @@ -190,7 +237,8 @@ fn boosting_with_fees_including_network_fee_portion() { check_pool(&pool, [(BOOSTER_1, 1000), (BOOSTER_2, 2000)]); const PROVIDED_AMOUNT: u128 = 1000; - const FULL_BOOST_FEE: u128 = 10; + const FULL_BOOST_FEE: u128 = + (PROVIDED_AMOUNT * BOOST_FEE_BPS as u128) / MAX_BASIS_POINTS as u128; const DEPOSIT_AMOUNT: u128 = PROVIDED_AMOUNT + FULL_BOOST_FEE; @@ -211,8 +259,9 @@ fn boosting_with_fees_including_network_fee_portion() { const NETWORK_FEE_FROM_BOOST: u128 = FULL_BOOST_FEE * NETWORK_FEE_PORTION_PERCENT as u128 / 100; - // NOTE: we subtract 1 to account for a rounding "error" (in real code any remaining fee will be - // used as network fee, so all atomic units will be accounted for) + // Sanity check: network fee and boosters fee should make up the full boost fee. + // Note that we subtract 1 to account for a rounding "error" (in real code any + // remaining fee will be used as network fee, so all atomic units will be accounted for). assert_eq!(TOTAL_BOOSTERS_FEE, FULL_BOOST_FEE - NETWORK_FEE_FROM_BOOST - 1); // The recorded amounts include fees @@ -230,7 +279,7 @@ fn boosting_with_fees_including_network_fee_portion() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: PROVIDED_AMOUNT + TOTAL_BOOSTERS_FEE, + amount_credited_to_boosters: PROVIDED_AMOUNT + TOTAL_BOOSTERS_FEE, unlocked_funds: vec![] } ); @@ -273,7 +322,7 @@ fn adding_funds_during_pending_withdrawal_from_same_booster() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: DEPOSIT_AMOUNT, + amount_credited_to_boosters: DEPOSIT_AMOUNT, unlocked_funds: vec![] } ); @@ -296,7 +345,7 @@ fn withdrawing_funds_before_finalisation() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: 1000, + amount_credited_to_boosters: 1000, unlocked_funds: vec![(BOOSTER_1, 500)] } ); @@ -323,7 +372,7 @@ fn adding_funds_with_pending_withdrawals() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: 1000, + amount_credited_to_boosters: 1000, unlocked_funds: vec![(BOOSTER_1, 500)] } ); @@ -393,7 +442,7 @@ fn partially_losing_pending_withdrawals() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: 500, + amount_credited_to_boosters: 500, unlocked_funds: vec![(BOOSTER_1, 250)] } ); @@ -436,7 +485,7 @@ fn booster_joins_then_funds_lost() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: 500, + amount_credited_to_boosters: 500, unlocked_funds: vec![(BOOSTER_1, 250)] } ); @@ -484,7 +533,7 @@ fn booster_joins_between_boosts() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: 500, + amount_credited_to_boosters: 500, unlocked_funds: vec![(BOOSTER_1, 250)] }, ); @@ -506,7 +555,10 @@ fn booster_joins_between_boosts() { let mut pool = pool.clone(); assert_eq!( pool.process_deposit_as_finalised(BOOST_2), - DepositFinalisationOutcomeForPool { amount_credited: 1000, unlocked_funds: vec![] } + DepositFinalisationOutcomeForPool { + amount_credited_to_boosters: 1000, + unlocked_funds: vec![] + } ); check_pool(&pool, [(BOOSTER_2, 1010), (BOOSTER_3, 2014)]); check_pending_boosts(&pool, []); @@ -531,7 +583,7 @@ fn small_rewards_accumulate() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), DepositFinalisationOutcomeForPool { - amount_credited: SMALL_DEPOSIT, + amount_credited_to_boosters: SMALL_DEPOSIT, unlocked_funds: vec![] } ); @@ -549,7 +601,7 @@ fn small_rewards_accumulate() { assert_eq!( pool.process_deposit_as_finalised(prewitnessed_deposit_id), DepositFinalisationOutcomeForPool { - amount_credited: SMALL_DEPOSIT, + amount_credited_to_boosters: SMALL_DEPOSIT, unlocked_funds: vec![] } ); @@ -580,7 +632,10 @@ fn use_max_available_amount() { assert_eq!( pool.process_deposit_as_finalised(BOOST_1), - DepositFinalisationOutcomeForPool { amount_credited: 1_010_101, unlocked_funds: vec![] } + DepositFinalisationOutcomeForPool { + amount_credited_to_boosters: 1_010_101, + unlocked_funds: vec![] + } ); check_pool(&pool, [(BOOSTER_1, 1_010_301)]); diff --git a/state-chain/pallets/cf-ingress-egress/src/lib.rs b/state-chain/pallets/cf-ingress-egress/src/lib.rs index 6ce99e24ac..8ebb636569 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -285,7 +285,7 @@ pub enum PalletConfigUpdate, I: 'static> { lifetime: TargetChainBlockNumber, }, SetNetworkFeeDeductionFromBoost { - deduction_percents: Percent, + deduction_percent: Percent, }, } @@ -726,7 +726,7 @@ pub mod pallet { /// The fraction of the network fee that is deducted from the boost fee. #[pallet::storage] - pub type NetworkFeeDeductionFromBoostPercents, I: 'static = ()> = + pub type NetworkFeeDeductionFromBoostPercent, I: 'static = ()> = StorageValue<_, Percent, ValueQuery>; #[pallet::event] @@ -898,7 +898,7 @@ pub mod pallet { short_affiliate_id: AffiliateShortId, }, NetworkFeeDeductionFromBoostSet { - deduction_percents: Percent, + deduction_percent: Percent, }, } @@ -1309,11 +1309,11 @@ pub mod pallet { DepositChannelLifetime::::set(lifetime); Self::deposit_event(Event::::DepositChannelLifetimeSet { lifetime }); }, - PalletConfigUpdate::SetNetworkFeeDeductionFromBoost { deduction_percents } => { - NetworkFeeDeductionFromBoostPercents::::set(deduction_percents); + PalletConfigUpdate::SetNetworkFeeDeductionFromBoost { deduction_percent } => { + NetworkFeeDeductionFromBoostPercent::::set(deduction_percent); Self::deposit_event(Event::::NetworkFeeDeductionFromBoostSet { - deduction_percents, + deduction_percent, }); }, } @@ -1781,7 +1781,7 @@ impl, I: 'static> Pallet { "Boost tiers should be in ascending order" ); - let network_fee_portion = NetworkFeeDeductionFromBoostPercents::::get(); + let network_fee_portion = NetworkFeeDeductionFromBoostPercent::::get(); for boost_tier in sorted_boost_tiers { if boost_tier > max_boost_fee_bps { @@ -2345,16 +2345,19 @@ impl, I: 'static> Pallet { }; if let Some((prewitnessed_deposit_id, used_pools)) = maybe_boost_to_process { - let mut total_amount_credited: TargetChainAmount = 0u32.into(); + let mut total_amount_credited_to_boosters: TargetChainAmount = 0u32.into(); // Note that ingress fee is not payed here, as it has already been payed at the time // of boosting for boost_tier in used_pools { BoostPools::::mutate(asset, boost_tier, |maybe_pool| { if let Some(pool) = maybe_pool { - let DepositFinalisationOutcomeForPool { unlocked_funds, amount_credited } = - pool.process_deposit_as_finalised(prewitnessed_deposit_id); + let DepositFinalisationOutcomeForPool { + unlocked_funds, + amount_credited_to_boosters, + } = pool.process_deposit_as_finalised(prewitnessed_deposit_id); - total_amount_credited += amount_credited; + total_amount_credited_to_boosters + .saturating_accrue(amount_credited_to_boosters); for (booster_id, finalised_withdrawn_amount) in unlocked_funds { T::Balance::credit_account( @@ -2368,7 +2371,8 @@ impl, I: 'static> Pallet { } // Any excess amount is charged as network fee: - let network_fee_from_boost = deposit_amount.saturating_sub(total_amount_credited); + let network_fee_from_boost = + deposit_amount.saturating_sub(total_amount_credited_to_boosters); let network_fee_swap_request_id = if network_fee_from_boost > 0u32.into() { // NOTE: if asset is FLIP, we shouldn't need to swap, but it should still work, and diff --git a/state-chain/pallets/cf-ingress-egress/src/tests.rs b/state-chain/pallets/cf-ingress-egress/src/tests.rs index e8df861ded..1ca0e4dc3a 100644 --- a/state-chain/pallets/cf-ingress-egress/src/tests.rs +++ b/state-chain/pallets/cf-ingress-egress/src/tests.rs @@ -6,7 +6,7 @@ use crate::{ ChannelOpeningFee, CrossChainMessage, DepositAction, DepositChannelLifetime, DepositChannelLookup, DepositChannelPool, DepositIgnoredReason, DepositWitness, DisabledEgressAssets, EgressDustLimit, Event as PalletEvent, Event, FailedForeignChainCall, - FailedForeignChainCalls, FetchOrTransfer, MinimumDeposit, NetworkFeeDeductionFromBoostPercents, + FailedForeignChainCalls, FetchOrTransfer, MinimumDeposit, NetworkFeeDeductionFromBoostPercent, Pallet, PalletConfigUpdate, PalletSafeMode, PrewitnessedDepositIdCounter, ScheduledEgressCcm, ScheduledEgressFetchOrTransfer, VaultDepositWitness, }; @@ -1481,7 +1481,7 @@ fn can_update_all_config_items() { lifetime: NEW_DEPOSIT_CHANNEL_LIFETIME }, PalletConfigUpdate::SetNetworkFeeDeductionFromBoost { - deduction_percents: NETWORK_FEE_DEDUCTION + deduction_percent: NETWORK_FEE_DEDUCTION } ] .try_into() @@ -1493,7 +1493,7 @@ fn can_update_all_config_items() { assert_eq!(MinimumDeposit::::get(EthAsset::Flip), NEW_MIN_DEPOSIT_FLIP); assert_eq!(MinimumDeposit::::get(EthAsset::Eth), NEW_MIN_DEPOSIT_ETH); assert_eq!(DepositChannelLifetime::::get(), NEW_DEPOSIT_CHANNEL_LIFETIME); - assert_eq!(NetworkFeeDeductionFromBoostPercents::::get(), NETWORK_FEE_DEDUCTION); + assert_eq!(NetworkFeeDeductionFromBoostPercent::::get(), NETWORK_FEE_DEDUCTION); // Check that the events were emitted assert_events_eq!( @@ -1511,7 +1511,7 @@ fn can_update_all_config_items() { lifetime: NEW_DEPOSIT_CHANNEL_LIFETIME }), RuntimeEvent::IngressEgress(Event::NetworkFeeDeductionFromBoostSet { - deduction_percents: NETWORK_FEE_DEDUCTION + deduction_percent: NETWORK_FEE_DEDUCTION }), ); diff --git a/state-chain/pallets/cf-ingress-egress/src/tests/boost.rs b/state-chain/pallets/cf-ingress-egress/src/tests/boost.rs index ae6aa9d19e..30243c718e 100644 --- a/state-chain/pallets/cf-ingress-egress/src/tests/boost.rs +++ b/state-chain/pallets/cf-ingress-egress/src/tests/boost.rs @@ -1055,7 +1055,7 @@ fn taking_network_fee_from_boost_fee() { // we get a non-zero amount of the input asset, and we schedule a swap to FLIP as // network fee. - use crate::NetworkFeeDeductionFromBoostPercents; + use crate::NetworkFeeDeductionFromBoostPercent; new_test_ext().execute_with(|| { const ASSET: EthAsset = EthAsset::Eth; @@ -1077,7 +1077,7 @@ fn taking_network_fee_from_boost_fee() { // First check that with a zero network fee portion, no network fee is collected: { assert_eq!( - NetworkFeeDeductionFromBoostPercents::::get(), + NetworkFeeDeductionFromBoostPercent::::get(), Percent::from_percent(0) ); let _ = prewitness_deposit(deposit_address, ASSET, DEPOSIT_AMOUNT); @@ -1108,11 +1108,11 @@ fn taking_network_fee_from_boost_fee() { assert_ok!(Pallet::::update_pallet_config( RuntimeOrigin::root(), bounded_vec![PalletConfigUpdate::SetNetworkFeeDeductionFromBoost { - deduction_percents: Percent::from_percent(20) + deduction_percent: Percent::from_percent(20) }] )); assert_eq!( - NetworkFeeDeductionFromBoostPercents::::get(), + NetworkFeeDeductionFromBoostPercent::::get(), Percent::from_percent(20) ); let _ = prewitness_deposit(deposit_address, ASSET, DEPOSIT_AMOUNT); diff --git a/state-chain/runtime/src/lib.rs b/state-chain/runtime/src/lib.rs index e8d9294973..65f0a41b4e 100644 --- a/state-chain/runtime/src/lib.rs +++ b/state-chain/runtime/src/lib.rs @@ -2100,7 +2100,7 @@ impl_runtime_apis! { fn boost_pools_details(asset: TargetChainAsset::) -> BTreeMap where Runtime: pallet_cf_ingress_egress::Config { - let network_fee_deduction_percents = pallet_cf_ingress_egress::NetworkFeeDeductionFromBoostPercents::::get(); + let network_fee_deduction_percent = pallet_cf_ingress_egress::NetworkFeeDeductionFromBoostPercent::::get(); pallet_cf_ingress_egress::BoostPools::::iter_prefix(asset).map(|(tier, pool)| { ( @@ -2114,7 +2114,7 @@ impl_runtime_apis! { ) }).collect(), pending_withdrawals: pool.get_pending_withdrawals().clone(), - network_fee_deduction_percents, + network_fee_deduction_percent, } ) }).collect() diff --git a/state-chain/runtime/src/runtime_apis.rs b/state-chain/runtime/src/runtime_apis.rs index a81e44dea9..d93457e91b 100644 --- a/state-chain/runtime/src/runtime_apis.rs +++ b/state-chain/runtime/src/runtime_apis.rs @@ -123,7 +123,7 @@ pub struct BoostPoolDetails { pub pending_boosts: BTreeMap>>, pub pending_withdrawals: BTreeMap>, - pub network_fee_deduction_percents: Percent, + pub network_fee_deduction_percent: Percent, } #[derive(Encode, Decode, Eq, PartialEq, TypeInfo)]