From c4e1727c4e095c2b98dc5b1e097dc3ed0b8d8fe3 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Mon, 24 Feb 2025 12:09:21 +0100 Subject: [PATCH 01/10] feat: update diff fees function --- pallets/idn-manager/src/impls.rs | 33 ++-- pallets/idn-manager/src/lib.rs | 1 + pallets/idn-manager/src/tests/fee_examples.rs | 167 ++++++++++-------- pallets/idn-manager/src/tests/pallet.rs | 20 ++- pallets/idn-manager/src/traits.rs | 29 ++- 5 files changed, 168 insertions(+), 82 deletions(-) diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index 6a2d390..2f4673e 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -17,7 +17,9 @@ //! # Implementations of public traits use crate::{ - self as pallet_idn_manager, traits::FeesError, HoldReason, Subscription, SubscriptionTrait, + self as pallet_idn_manager, + traits::{DiffFees, FeesDirection, FeesError}, + HoldReason, Subscription, SubscriptionTrait, }; use codec::Encode; use frame_support::{ @@ -28,8 +30,8 @@ use frame_support::{ }, }; use sp_arithmetic::traits::Unsigned; -use sp_runtime::{AccountId32, Saturating}; -use sp_std::marker::PhantomData; +use sp_runtime::{traits::Zero, AccountId32, Saturating}; +use sp_std::{cmp::Ordering, marker::PhantomData}; impl SubscriptionTrait for Subscription @@ -61,7 +63,7 @@ impl< T: Get, B: Get, S: SubscriptionTrait, - BlockNumber, + BlockNumber: Saturating + Ord, Balances: Mutate, > pallet_idn_manager::FeesManager for FeesManagerImpl @@ -73,12 +75,23 @@ where let base_fee = B::get(); base_fee.saturating_mul(amount.into()) } - fn calculate_refund_fees( - _init_amount: BlockNumber, - current_amount: BlockNumber, - ) -> Balances::Balance { - // in this case of a linear function, the refund's is the same as the fees' - Self::calculate_subscription_fees(current_amount) + fn calculate_diff_fees( + old_amount: BlockNumber, + new_amount: BlockNumber, + ) -> DiffFees { + let mut direction = FeesDirection::None; + let fees = match new_amount.cmp(&old_amount) { + Ordering::Greater => { + direction = FeesDirection::Hold; + Self::calculate_subscription_fees(new_amount.saturating_sub(old_amount)) + }, + Ordering::Less => { + direction = FeesDirection::Release; + Self::calculate_subscription_fees(old_amount.saturating_sub(new_amount)) + }, + Ordering::Equal => Zero::zero(), + }; + DiffFees { fees, direction } } fn collect_fees( fees: Balances::Balance, diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index 3543a51..bb5fecc 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -356,6 +356,7 @@ pub mod pallet { sub.details.frequency = frequency; sub.details.updated_at = frame_system::Pallet::::block_number(); // TODO implement a way to refund or take the difference in fees https://github.com/ideal-lab5/idn-sdk/issues/104 + // Self::manage_fees_and_deposit() Self::deposit_event(Event::SubscriptionUpdated { sub_id }); Ok(()) }) diff --git a/pallets/idn-manager/src/tests/fee_examples.rs b/pallets/idn-manager/src/tests/fee_examples.rs index 116cecb..986aaf8 100644 --- a/pallets/idn-manager/src/tests/fee_examples.rs +++ b/pallets/idn-manager/src/tests/fee_examples.rs @@ -18,19 +18,33 @@ //! //! This module contains examples of fee calculators that can be used in the IDN Manager pallet. -use crate::traits::FeesManager; +use crate::traits::{DiffFees, FeesDirection, FeesManager}; +use sp_runtime::traits::Zero; +use sp_std::cmp::Ordering; /// Linear fee calculator with no discount pub struct LinearFeeCalculator; +const BASE_FEE: u32 = 100; + impl FeesManager for LinearFeeCalculator { fn calculate_subscription_fees(amount: u32) -> u32 { - let base_fee = 100u32; - base_fee.saturating_mul(amount.into()) + BASE_FEE.saturating_mul(amount.into()) } - fn calculate_refund_fees(_init_amount: u32, current_amount: u32) -> u32 { - // in this case of a linear function, the refund's is the same as the fees' - Self::calculate_subscription_fees(current_amount) + fn calculate_diff_fees(old_amount: u32, new_amount: u32) -> DiffFees { + let mut direction = FeesDirection::None; + let fees = match new_amount.cmp(&old_amount) { + Ordering::Greater => { + direction = FeesDirection::Hold; + Self::calculate_subscription_fees(new_amount.saturating_sub(old_amount)) + }, + Ordering::Less => { + direction = FeesDirection::Release; + Self::calculate_subscription_fees(old_amount.saturating_sub(new_amount)) + }, + Ordering::Equal => Zero::zero(), + }; + DiffFees { fees, direction } } fn collect_fees(fees: u32, _: ()) -> Result> { // In this case, we don't need to do anything with the fees, so we just return them @@ -43,8 +57,6 @@ pub struct SteppedTieredFeeCalculator; impl FeesManager for SteppedTieredFeeCalculator { fn calculate_subscription_fees(amount: u32) -> u32 { - let base_fee = 100u32; - // Define tier boundaries and their respective discount rates (in basis points) const TIERS: [(u32, u32); 5] = [ (1, 0), // 0-10: 0% discount @@ -68,7 +80,7 @@ impl FeesManager for SteppedTieredFeeCalculator { (amount.min(next_tier_start.saturating_sub(1)) - current_tier_start + 1) .min(remaining_amount); - let tier_fee = base_fee + let tier_fee = BASE_FEE .saturating_mul(credits_in_tier) .saturating_mul((10_000 - current_tier_discount) as u32) .saturating_div(10_000); @@ -79,12 +91,23 @@ impl FeesManager for SteppedTieredFeeCalculator { total_fee } - // The discount for the `used_amount` (`init_amount` - `current_amount`) is taken from the lower - // tieres (those with less discounts) - fn calculate_refund_fees(init_amount: u32, current_amount: u32) -> u32 { - let used_amount = init_amount - current_amount; - Self::calculate_subscription_fees(init_amount) - - Self::calculate_subscription_fees(used_amount) + + fn calculate_diff_fees(old_amount: u32, new_amount: u32) -> DiffFees { + let old_fees = Self::calculate_subscription_fees(old_amount); + let new_fees = Self::calculate_subscription_fees(new_amount); + let mut direction = FeesDirection::None; + let fees = match new_fees.cmp(&old_fees) { + Ordering::Greater => { + direction = FeesDirection::Hold; + new_fees - old_fees + }, + Ordering::Less => { + direction = FeesDirection::Release; + old_fees - new_fees + }, + Ordering::Equal => Zero::zero(), + }; + DiffFees { fees, direction } } fn collect_fees(fees: u32, _: ()) -> Result> { // In this case, we don't need to do anything with the fees, so we just return them @@ -102,45 +125,49 @@ mod tests { assert_eq!(LinearFeeCalculator::calculate_subscription_fees(10), 1_000); assert_eq!(LinearFeeCalculator::calculate_subscription_fees(100), 10_000); } - /// Test when no subscription credits were used. + + /// Thest when a subscription is fully used before being killed or the update does not change + /// the amount. #[test] - fn test_calculate_refund_linear_fees_no_usage() { - let init_amount: u32 = 50; - let current_amount: u32 = 50; // no credits used - // In this case the refund should equal the fee for the entire subscription. - let refund = LinearFeeCalculator::calculate_refund_fees(init_amount, current_amount); - let expected = LinearFeeCalculator::calculate_subscription_fees(init_amount) - - LinearFeeCalculator::calculate_subscription_fees(0); // used_amount is 0 + fn test_calculate_release_linear_fees_no_diff() { + let old_amount: u32 = 50; + let new_amount: u32 = 50; // no credits diff + let refund = LinearFeeCalculator::calculate_diff_fees(old_amount, new_amount); + // there should be no refund as all credit has been used, or no difference in the update + let expected = 0; assert_eq!( - refund, expected, - "Refund must equal entire subscription fee when no credits were used" + refund, + DiffFees { fees: expected, direction: FeesDirection::None }, + "There should be no release when no diff in amount" ); } - /// Test for a partial usage scenario. + /// Test when a subscription is reduced or killed. #[test] - fn test_calculate_refund_linear_fees_partial_usage() { - let init_amount: u32 = 50; - let current_amount: u32 = 30; // 20 credits used - let refund = LinearFeeCalculator::calculate_refund_fees(init_amount, current_amount); - let used_amount = init_amount - current_amount; // 20 - let expected = LinearFeeCalculator::calculate_subscription_fees(init_amount) - - LinearFeeCalculator::calculate_subscription_fees(used_amount); + fn test_calculate_release_linear_fees_lower_diff() { + let old_amount: u32 = 50; + let new_amount: u32 = 30; // 20 credits used + let refund = LinearFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let expected = 20 * BASE_FEE; assert_eq!( - refund, expected, - "Refund must equal the fee difference between full subscription and the used portion" + refund, + DiffFees { fees: expected, direction: FeesDirection::Release }, + "There should be a release when new amount is lower than old amount" ); } - /// Test when the subscription is fully used. + /// Test when the subscription is extended. #[test] - fn test_calculate_refund_linear_fees_full_usage() { - let init_amount: u32 = 50; - let current_amount: u32 = 0; // all credits used - let refund = LinearFeeCalculator::calculate_refund_fees(init_amount, current_amount); - let expected = LinearFeeCalculator::calculate_subscription_fees(init_amount) - - LinearFeeCalculator::calculate_subscription_fees(init_amount); // should be 0 - assert_eq!(refund, expected, "Refund must be zero when all subscription credits are used"); + fn test_calculate_hold_linear_fees_greater_diff() { + let old_amount: u32 = 50; + let new_amount: u32 = 60; // all credits used + let refund = LinearFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let expected = 10 * BASE_FEE; + assert_eq!( + refund, + DiffFees { fees: expected, direction: FeesDirection::Hold }, + "There should be more held balance" + ); } #[test] @@ -178,44 +205,46 @@ mod tests { assert!(fee_101 > fee_100, "101 credits should cost more than 100 credits"); } - /// Test when no subscription credits were used. + /// Thest when a subscription is fully used before being killed or the update does not change + /// the amount. #[test] - fn test_calculate_refund_fees_no_usage() { - let init_amount: u32 = 50; - let current_amount: u32 = 50; // no credits used - // In this case the refund should equal the fee for the entire subscription. - let refund = SteppedTieredFeeCalculator::calculate_refund_fees(init_amount, current_amount); - let expected = SteppedTieredFeeCalculator::calculate_subscription_fees(init_amount) - - SteppedTieredFeeCalculator::calculate_subscription_fees(0); // used_amount is 0 + fn test_calculate_release_stepped_fees_no_diff() { + let old_amount: u32 = 50; + let new_amount: u32 = 50; // no credits diff + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let expected = 0; assert_eq!( - refund, expected, - "Refund must equal entire subscription fee when no credits were used" + refund, + DiffFees { fees: expected, direction: FeesDirection::None }, + "There should be no release when no diff in amount" ); } /// Test for a partial usage scenario. #[test] - fn test_calculate_refund_fees_partial_usage() { - let init_amount: u32 = 50; - let current_amount: u32 = 30; // 20 credits used - let refund = SteppedTieredFeeCalculator::calculate_refund_fees(init_amount, current_amount); - let used_amount = init_amount - current_amount; // 20 - let expected = SteppedTieredFeeCalculator::calculate_subscription_fees(init_amount) - - SteppedTieredFeeCalculator::calculate_subscription_fees(used_amount); + fn test_calculate_release_stepped_fees_lower_diff() { + let old_amount: u32 = 110; + let new_amount: u32 = 100; // 1 value decrease + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let expected = 10 * BASE_FEE.saturating_mul(9_000).saturating_div(10_000); // 10% discount on the extra value assert_eq!( - refund, expected, - "Refund must equal the fee difference between full subscription and the used portion" + refund, + DiffFees { fees: expected, direction: FeesDirection::Release }, + "There should be a release when new amount is lower than old amount" ); } /// Test when the subscription is fully used. #[test] - fn test_calculate_refund_fees_full_usage() { - let init_amount: u32 = 50; - let current_amount: u32 = 0; // all credits used - let refund = SteppedTieredFeeCalculator::calculate_refund_fees(init_amount, current_amount); - let expected = SteppedTieredFeeCalculator::calculate_subscription_fees(init_amount) - - SteppedTieredFeeCalculator::calculate_subscription_fees(init_amount); // should be 0 - assert_eq!(refund, expected, "Refund must be zero when all subscription credits are used"); + fn test_calculate_hold_stepped_fees_greater_diff() { + let old_amount: u32 = 100; + let new_amount: u32 = 101; // 1 value increase + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let expected = 1 * BASE_FEE.saturating_mul(9_000).saturating_div(10_000); // 10% discount on the extra value + assert_eq!( + refund, + DiffFees { fees: expected, direction: FeesDirection::Hold }, + "There should be more held balance" + ); } } diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index 145c4e3..a0e700e 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -287,7 +287,25 @@ fn test_update_subscription() { new_frequency )); - // TODO implement a way to refund or take the difference in fees https://github.com/ideal-lab5/idn-sdk/issues/104 + let new_fees = ::FeesManager::calculate_subscription_fees(new_amount); + let new_deposit = + ::DepositCalculator::calculate_storage_deposit(&subscription); + + let fees_diff = new_fees - original_fees; + let deposit_diff = new_deposit - original_deposit; + + let balance_after_update = balance_after_create - fees_diff - deposit_diff; + + // assert fees and deposit diff is correctly handled + assert_eq!(Balances::free_balance(&ALICE), balance_after_update); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), new_fees); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), + new_deposit + ); + assert_eq!(balance_after_update + new_fees + new_deposit, initial_balance); + + // TODO: test probably in a separate test case fees handling but by adding block advance let subscription = Subscriptions::::get(sub_id).unwrap(); diff --git a/pallets/idn-manager/src/traits.rs b/pallets/idn-manager/src/traits.rs index ed4b1fd..6a79a7d 100644 --- a/pallets/idn-manager/src/traits.rs +++ b/pallets/idn-manager/src/traits.rs @@ -23,12 +23,37 @@ pub enum FeesError { NotEnoughBalance { needed: Fees, balance: Fees }, Other(Context), } + +/// Enum to represent the direction of fees movement. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum FeesDirection { + Hold, + Release, + // Fees aren't going anywhere. This is usually the case when diff is zero. + None, +} + +/// This struct represent movement of fees. +/// +/// * `fees` - how much fees are being moved. +/// * `direction` - if the fees are being collected or released. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub struct DiffFees { + pub fees: Fees, + pub direction: FeesDirection, +} + /// Trait for fees managing pub trait FeesManager, Err, S> { /// Calculate the fees for a subscription based on the amount of random values required. fn calculate_subscription_fees(amount: Amount) -> Fees; - /// Calculate how much fees should be refunded for a subscription that is being cancelled. - fn calculate_refund_fees(init_amount: Amount, current_amount: Amount) -> Fees; + /// Calculate how much fees should be held or release when a subscription changes. + /// + /// * `old_amount` - the amount of random values required before the change. + /// * `new_amount` - the amount of random values required after the change, this will represent + /// the updated amount in an update operation. Or the amount actually consumed in a kill + /// operation. + fn calculate_diff_fees(old_amount: Amount, new_amount: Amount) -> DiffFees; /// Distributes collected fees. Returns the fees that were effectively collected. fn collect_fees(fees: Fees, sub: Sub) -> Result>; } From 371da1e028bc3def9b8ced8353b5c12ae1dfae01 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Mon, 24 Feb 2025 13:37:17 +0100 Subject: [PATCH 02/10] feat: add fees and deposit management --- pallets/idn-manager/src/impls.rs | 65 ++++++++----- pallets/idn-manager/src/lib.rs | 85 +++++++++++++--- pallets/idn-manager/src/tests/fee_examples.rs | 96 ++++++++++--------- pallets/idn-manager/src/tests/pallet.rs | 11 ++- pallets/idn-manager/src/traits.rs | 30 +++--- 5 files changed, 187 insertions(+), 100 deletions(-) diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index 2f4673e..82c72a3 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -18,7 +18,7 @@ use crate::{ self as pallet_idn_manager, - traits::{DiffFees, FeesDirection, FeesError}, + traits::{BalanceDirection, DiffBalance, FeesError}, HoldReason, Subscription, SubscriptionTrait, }; use codec::Encode; @@ -63,7 +63,7 @@ impl< T: Get, B: Get, S: SubscriptionTrait, - BlockNumber: Saturating + Ord, + BlockNumber: Saturating + Ord + Clone, Balances: Mutate, > pallet_idn_manager::FeesManager for FeesManagerImpl @@ -71,38 +71,42 @@ where Balances::Balance: From, Balances::Reason: From, { - fn calculate_subscription_fees(amount: BlockNumber) -> Balances::Balance { + fn calculate_subscription_fees(amount: &BlockNumber) -> Balances::Balance { let base_fee = B::get(); - base_fee.saturating_mul(amount.into()) + base_fee.saturating_mul(amount.clone().into()) } fn calculate_diff_fees( - old_amount: BlockNumber, - new_amount: BlockNumber, - ) -> DiffFees { - let mut direction = FeesDirection::None; + old_amount: &BlockNumber, + new_amount: &BlockNumber, + ) -> DiffBalance { + let mut direction = BalanceDirection::None; let fees = match new_amount.cmp(&old_amount) { Ordering::Greater => { - direction = FeesDirection::Hold; - Self::calculate_subscription_fees(new_amount.saturating_sub(old_amount)) + direction = BalanceDirection::Hold; + Self::calculate_subscription_fees( + &new_amount.clone().saturating_sub(old_amount.clone()), + ) }, Ordering::Less => { - direction = FeesDirection::Release; - Self::calculate_subscription_fees(old_amount.saturating_sub(new_amount)) + direction = BalanceDirection::Release; + Self::calculate_subscription_fees( + &old_amount.clone().saturating_sub(new_amount.clone()), + ) }, Ordering::Equal => Zero::zero(), }; - DiffFees { fees, direction } + DiffBalance { balance: fees, direction } } fn collect_fees( - fees: Balances::Balance, - sub: S, + fees: &Balances::Balance, + sub: &S, ) -> Result> { // Collect the held fees from the subscriber let collected = Balances::transfer_on_hold( &HoldReason::Fees.into(), sub.subscriber(), &T::get(), - fees, + fees.clone(), Precision::BestEffort, Restriction::Free, Fortitude::Polite, @@ -111,29 +115,40 @@ where // Ensure the correct amount was collected. // TODO: error to bubble up and be handled by caller https://github.com/ideal-lab5/idn-sdk/issues/107 - if collected < fees { - return Err(FeesError::NotEnoughBalance { needed: fees, balance: collected }); + if collected < *fees { + return Err(FeesError::NotEnoughBalance { needed: *fees, balance: collected }); } Ok(collected) } } -pub struct DepositCalculatorImpl, Balance> { - pub _phantom: (PhantomData, PhantomData), +pub struct DepositCalculatorImpl, Deposit> { + pub _phantom: (PhantomData, PhantomData), } impl< S: SubscriptionTrait + Encode, - SDMultiplier: Get, - Balance: From + Saturating, - > pallet_idn_manager::DepositCalculator - for DepositCalculatorImpl + SDMultiplier: Get, + Deposit: From + Saturating + Ord, + > pallet_idn_manager::DepositCalculator + for DepositCalculatorImpl { - fn calculate_storage_deposit(sub: &S) -> Balance { + fn calculate_storage_deposit(sub: &S) -> Deposit { let storage_deposit_multiplier = SDMultiplier::get(); // calculate the size of scale encoded `sub` let encoded_size = sub.encode().len() as u32; storage_deposit_multiplier.saturating_mul(encoded_size.into()) } + + fn calculate_diff_deposit(old_sub: &S, new_sub: &S) -> DiffBalance { + let old_deposit = Self::calculate_storage_deposit(old_sub); + let new_deposit = Self::calculate_storage_deposit(new_sub); + let direction = match new_deposit.cmp(&old_deposit) { + Ordering::Greater => BalanceDirection::Hold, + Ordering::Less => BalanceDirection::Release, + Ordering::Equal => BalanceDirection::None, + }; + DiffBalance { balance: new_deposit.saturating_sub(old_deposit), direction } + } } diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index bb5fecc..7bac444 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -66,6 +66,7 @@ use frame_support::{ sp_runtime::traits::AccountIdConversion, traits::{ fungible::{hold::Mutate as HoldMutate, Inspect}, + tokens::Precision, Get, }, BoundedVec, @@ -79,7 +80,10 @@ use sp_arithmetic::traits::Unsigned; use sp_core::H256; use sp_io::hashing::blake2_256; use sp_std::fmt::Debug; -use traits::{DepositCalculator, FeesManager, Subscription as SubscriptionTrait}; +use traits::{ + BalanceDirection, DepositCalculator, DiffBalance, FeesManager, + Subscription as SubscriptionTrait, +}; use xcm::{ v5::{prelude::*, Location}, VersionedLocation, VersionedXcm, @@ -246,8 +250,8 @@ pub mod pallet { SubscriptionAlreadyPaused, /// The origin isn't the subscriber NotSubscriber, - /// Insufficient balance for subscription - InsufficientBalance, + // /// Insufficient balance for subscription + // InsufficientBalance, } /// A reason for the IDN Manager Pallet placing a hold on funds. @@ -352,11 +356,25 @@ pub mod pallet { Subscriptions::::try_mutate(sub_id, |maybe_sub| { let sub = maybe_sub.as_mut().ok_or(Error::::SubscriptionDoesNotExist)?; ensure!(sub.details.subscriber == subscriber, Error::::NotSubscriber); + + let fees_diff = T::FeesManager::calculate_diff_fees(&sub.details.amount, &amount); + let deposit_diff = T::DepositCalculator::calculate_diff_deposit( + &sub, + &Subscription { + state: sub.state.clone(), + credits_left: amount, + details: sub.details.clone(), + }, + ); + sub.details.amount = amount; sub.details.frequency = frequency; sub.details.updated_at = frame_system::Pallet::::block_number(); - // TODO implement a way to refund or take the difference in fees https://github.com/ideal-lab5/idn-sdk/issues/104 - // Self::manage_fees_and_deposit() + + // Hold or refund diff fees + Self::manage_diff_fees(&subscriber, &fees_diff)?; + // Hold or refund diff deposit + Self::manage_diff_deposit(&subscriber, &deposit_diff)?; Self::deposit_event(Event::SubscriptionUpdated { sub_id }); Ok(()) }) @@ -431,9 +449,9 @@ impl Pallet { metadata: Option>, ) -> DispatchResult { // Calculate and hold the subscription fees - let fees = T::FeesManager::calculate_subscription_fees(amount); - T::Currency::hold(&HoldReason::Fees.into(), &subscriber, fees) - .map_err(|_| Error::::InsufficientBalance)?; + let fees = T::FeesManager::calculate_subscription_fees(&amount); + + Self::hold_fees(&subscriber, fees)?; let current_block = frame_system::Pallet::::block_number(); let details = SubscriptionDetails { @@ -448,12 +466,10 @@ impl Pallet { let subscription = Subscription { state: SubscriptionState::Active, credits_left: amount, details }; - T::Currency::hold( - &HoldReason::StorageDeposit.into(), + Self::hold_deposit( &subscriber, T::DepositCalculator::calculate_storage_deposit(&subscription), - ) - .map_err(|_| Error::::InsufficientBalance)?; + )?; let sub_id = subscription.id(); @@ -466,6 +482,51 @@ impl Pallet { Ok(()) } + fn hold_fees(subscriber: &T::AccountId, fees: BalanceOf) -> DispatchResult { + T::Currency::hold(&HoldReason::Fees.into(), subscriber, fees) + } + + fn release_fees(subscriber: &T::AccountId, fees: BalanceOf) -> DispatchResult { + let _ = T::Currency::release(&HoldReason::Fees.into(), subscriber, fees, Precision::Exact)?; + Ok(()) + } + + fn manage_diff_fees( + subscriber: &T::AccountId, + diff: &DiffBalance>, + ) -> DispatchResult { + match diff.direction { + BalanceDirection::Hold => Self::hold_fees(subscriber, diff.balance), + BalanceDirection::Release => Self::release_fees(subscriber, diff.balance), + BalanceDirection::None => Ok(()), + } + } + + fn hold_deposit(subscriber: &T::AccountId, deposit: BalanceOf) -> DispatchResult { + T::Currency::hold(&HoldReason::StorageDeposit.into(), subscriber, deposit) + } + + fn release_deposit(subscriber: &T::AccountId, deposit: BalanceOf) -> DispatchResult { + let _ = T::Currency::release( + &HoldReason::StorageDeposit.into(), + subscriber, + deposit, + Precision::BestEffort, + )?; + Ok(()) + } + + fn manage_diff_deposit( + subscriber: &T::AccountId, + diff: &DiffBalance>, + ) -> DispatchResult { + match diff.direction { + BalanceDirection::Hold => Self::hold_deposit(subscriber, diff.balance), + BalanceDirection::Release => Self::release_deposit(subscriber, diff.balance), + BalanceDirection::None => Ok(()), + } + } + /// Helper function to construct XCM message for randomness distribution // TODO: finish this off as part of https://github.com/ideal-lab5/idn-sdk/issues/77 fn construct_randomness_xcm(target: Location, _rnd: &T::Rnd) -> Result, Error> { diff --git a/pallets/idn-manager/src/tests/fee_examples.rs b/pallets/idn-manager/src/tests/fee_examples.rs index 986aaf8..0b71358 100644 --- a/pallets/idn-manager/src/tests/fee_examples.rs +++ b/pallets/idn-manager/src/tests/fee_examples.rs @@ -18,7 +18,7 @@ //! //! This module contains examples of fee calculators that can be used in the IDN Manager pallet. -use crate::traits::{DiffFees, FeesDirection, FeesManager}; +use crate::traits::{BalanceDirection, DiffBalance, FeesManager}; use sp_runtime::traits::Zero; use sp_std::cmp::Ordering; @@ -28,27 +28,31 @@ pub struct LinearFeeCalculator; const BASE_FEE: u32 = 100; impl FeesManager for LinearFeeCalculator { - fn calculate_subscription_fees(amount: u32) -> u32 { - BASE_FEE.saturating_mul(amount.into()) + fn calculate_subscription_fees(amount: &u32) -> u32 { + BASE_FEE.saturating_mul(amount.clone().into()) } - fn calculate_diff_fees(old_amount: u32, new_amount: u32) -> DiffFees { - let mut direction = FeesDirection::None; + fn calculate_diff_fees(old_amount: &u32, new_amount: &u32) -> DiffBalance { + let mut direction = BalanceDirection::None; let fees = match new_amount.cmp(&old_amount) { Ordering::Greater => { - direction = FeesDirection::Hold; - Self::calculate_subscription_fees(new_amount.saturating_sub(old_amount)) + direction = BalanceDirection::Hold; + Self::calculate_subscription_fees( + &new_amount.clone().saturating_sub(old_amount.clone()), + ) }, Ordering::Less => { - direction = FeesDirection::Release; - Self::calculate_subscription_fees(old_amount.saturating_sub(new_amount)) + direction = BalanceDirection::Release; + Self::calculate_subscription_fees( + &old_amount.clone().saturating_sub(new_amount.clone()), + ) }, Ordering::Equal => Zero::zero(), }; - DiffFees { fees, direction } + DiffBalance { balance: fees, direction } } - fn collect_fees(fees: u32, _: ()) -> Result> { + fn collect_fees(fees: &u32, _: &()) -> Result> { // In this case, we don't need to do anything with the fees, so we just return them - Ok(fees) + Ok(*fees) } } @@ -56,7 +60,7 @@ impl FeesManager for LinearFeeCalculator { pub struct SteppedTieredFeeCalculator; impl FeesManager for SteppedTieredFeeCalculator { - fn calculate_subscription_fees(amount: u32) -> u32 { + fn calculate_subscription_fees(amount: &u32) -> u32 { // Define tier boundaries and their respective discount rates (in basis points) const TIERS: [(u32, u32); 5] = [ (1, 0), // 0-10: 0% discount @@ -67,17 +71,17 @@ impl FeesManager for SteppedTieredFeeCalculator { ]; let mut total_fee = 0u32; - let mut remaining_amount = amount; + let mut remaining_amount = *amount; for (i, &(current_tier_start, current_tier_discount)) in TIERS.iter().enumerate() { // If no remaining credits or the tier starts above the requested amount, exit loop. - if remaining_amount == 0 || amount < current_tier_start { + if remaining_amount == 0 || amount < ¤t_tier_start { break; } let next_tier_start = TIERS.get(i + 1).map(|&(start, _)| start).unwrap_or(u32::MAX); let credits_in_tier = - (amount.min(next_tier_start.saturating_sub(1)) - current_tier_start + 1) + (amount.min(&next_tier_start.saturating_sub(1)) - current_tier_start + 1) .min(remaining_amount); let tier_fee = BASE_FEE @@ -92,26 +96,26 @@ impl FeesManager for SteppedTieredFeeCalculator { total_fee } - fn calculate_diff_fees(old_amount: u32, new_amount: u32) -> DiffFees { + fn calculate_diff_fees(old_amount: &u32, new_amount: &u32) -> DiffBalance { let old_fees = Self::calculate_subscription_fees(old_amount); let new_fees = Self::calculate_subscription_fees(new_amount); - let mut direction = FeesDirection::None; + let mut direction = BalanceDirection::None; let fees = match new_fees.cmp(&old_fees) { Ordering::Greater => { - direction = FeesDirection::Hold; + direction = BalanceDirection::Hold; new_fees - old_fees }, Ordering::Less => { - direction = FeesDirection::Release; + direction = BalanceDirection::Release; old_fees - new_fees }, Ordering::Equal => Zero::zero(), }; - DiffFees { fees, direction } + DiffBalance { balance: fees, direction } } - fn collect_fees(fees: u32, _: ()) -> Result> { + fn collect_fees(fees: &u32, _: &()) -> Result> { // In this case, we don't need to do anything with the fees, so we just return them - Ok(fees) + Ok(*fees) } } @@ -121,9 +125,9 @@ mod tests { #[test] fn test_linear_fee_calculator() { - assert_eq!(LinearFeeCalculator::calculate_subscription_fees(1), 100); - assert_eq!(LinearFeeCalculator::calculate_subscription_fees(10), 1_000); - assert_eq!(LinearFeeCalculator::calculate_subscription_fees(100), 10_000); + assert_eq!(LinearFeeCalculator::calculate_subscription_fees(&1), 100); + assert_eq!(LinearFeeCalculator::calculate_subscription_fees(&10), 1_000); + assert_eq!(LinearFeeCalculator::calculate_subscription_fees(&100), 10_000); } /// Thest when a subscription is fully used before being killed or the update does not change @@ -132,12 +136,12 @@ mod tests { fn test_calculate_release_linear_fees_no_diff() { let old_amount: u32 = 50; let new_amount: u32 = 50; // no credits diff - let refund = LinearFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let refund = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); // there should be no refund as all credit has been used, or no difference in the update let expected = 0; assert_eq!( refund, - DiffFees { fees: expected, direction: FeesDirection::None }, + DiffBalance { balance: expected, direction: BalanceDirection::None }, "There should be no release when no diff in amount" ); } @@ -147,11 +151,11 @@ mod tests { fn test_calculate_release_linear_fees_lower_diff() { let old_amount: u32 = 50; let new_amount: u32 = 30; // 20 credits used - let refund = LinearFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let refund = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); let expected = 20 * BASE_FEE; assert_eq!( refund, - DiffFees { fees: expected, direction: FeesDirection::Release }, + DiffBalance { balance: expected, direction: BalanceDirection::Release }, "There should be a release when new amount is lower than old amount" ); } @@ -161,11 +165,11 @@ mod tests { fn test_calculate_hold_linear_fees_greater_diff() { let old_amount: u32 = 50; let new_amount: u32 = 60; // all credits used - let refund = LinearFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let refund = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); let expected = 10 * BASE_FEE; assert_eq!( refund, - DiffFees { fees: expected, direction: FeesDirection::Hold }, + DiffBalance { balance: expected, direction: BalanceDirection::Hold }, "There should be more held balance" ); } @@ -173,18 +177,18 @@ mod tests { #[test] fn test_stepped_tiered_calculator() { // Test first tier (no discount) - assert_eq!(SteppedTieredFeeCalculator::calculate_subscription_fees(10), 1_000); // 10 * 100 = 1,000 + assert_eq!(SteppedTieredFeeCalculator::calculate_subscription_fees(&10), 1_000); // 10 * 100 = 1,000 // Test crossing into second tier - let fee_11 = SteppedTieredFeeCalculator::calculate_subscription_fees(11); + let fee_11 = SteppedTieredFeeCalculator::calculate_subscription_fees(&11); assert_eq!(fee_11, 1_095); // (10 * 100) + (1 * 95) = 1,000 + 95 = 1,095 // Test middle of second tier - let fee_50 = SteppedTieredFeeCalculator::calculate_subscription_fees(50); + let fee_50 = SteppedTieredFeeCalculator::calculate_subscription_fees(&50); assert_eq!(fee_50, 4_800); // (10 * 100) + (40 * 95) = 1,000 + 3,800 = 4,800 // Test crossing multiple tiers - let fee_150 = SteppedTieredFeeCalculator::calculate_subscription_fees(150); + let fee_150 = SteppedTieredFeeCalculator::calculate_subscription_fees(&150); // First 10 at 100% = 1,000 // Next 90 at 95% = 8,550 // Next 50 at 90% = 4,500 @@ -195,13 +199,13 @@ mod tests { #[test] fn test_no_price_inversion() { // Test that buying more never costs less - let fee_10 = SteppedTieredFeeCalculator::calculate_subscription_fees(10); - let fee_11 = SteppedTieredFeeCalculator::calculate_subscription_fees(11); + let fee_10 = SteppedTieredFeeCalculator::calculate_subscription_fees(&10); + let fee_11 = SteppedTieredFeeCalculator::calculate_subscription_fees(&11); assert!(fee_11 > fee_10, "11 credits should cost more than 10 credits"); // Test around the 100 credit boundary - let fee_100 = SteppedTieredFeeCalculator::calculate_subscription_fees(100); - let fee_101 = SteppedTieredFeeCalculator::calculate_subscription_fees(101); + let fee_100 = SteppedTieredFeeCalculator::calculate_subscription_fees(&100); + let fee_101 = SteppedTieredFeeCalculator::calculate_subscription_fees(&101); assert!(fee_101 > fee_100, "101 credits should cost more than 100 credits"); } @@ -211,11 +215,11 @@ mod tests { fn test_calculate_release_stepped_fees_no_diff() { let old_amount: u32 = 50; let new_amount: u32 = 50; // no credits diff - let refund = SteppedTieredFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); let expected = 0; assert_eq!( refund, - DiffFees { fees: expected, direction: FeesDirection::None }, + DiffBalance { balance: expected, direction: BalanceDirection::None }, "There should be no release when no diff in amount" ); } @@ -225,11 +229,11 @@ mod tests { fn test_calculate_release_stepped_fees_lower_diff() { let old_amount: u32 = 110; let new_amount: u32 = 100; // 1 value decrease - let refund = SteppedTieredFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); let expected = 10 * BASE_FEE.saturating_mul(9_000).saturating_div(10_000); // 10% discount on the extra value assert_eq!( refund, - DiffFees { fees: expected, direction: FeesDirection::Release }, + DiffBalance { balance: expected, direction: BalanceDirection::Release }, "There should be a release when new amount is lower than old amount" ); } @@ -239,11 +243,11 @@ mod tests { fn test_calculate_hold_stepped_fees_greater_diff() { let old_amount: u32 = 100; let new_amount: u32 = 101; // 1 value increase - let refund = SteppedTieredFeeCalculator::calculate_diff_fees(old_amount, new_amount); + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); let expected = 1 * BASE_FEE.saturating_mul(9_000).saturating_div(10_000); // 10% discount on the extra value assert_eq!( refund, - DiffFees { fees: expected, direction: FeesDirection::Hold }, + DiffBalance { balance: expected, direction: BalanceDirection::Hold }, "There should be more held balance" ); } diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index a0e700e..939ee6f 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -32,7 +32,7 @@ use frame_support::{ }; use idn_traits::rand::Dispatcher; use sp_core::H256; -use sp_runtime::AccountId32; +use sp_runtime::{AccountId32, TokenError}; use xcm::v5::{Junction, Location}; const ALICE: AccountId32 = AccountId32::new([1u8; 32]); @@ -75,7 +75,7 @@ fn create_subscription_works() { let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); // assert that the correct fees have been held - let fees = ::FeesManager::calculate_subscription_fees(amount); + let fees = ::FeesManager::calculate_subscription_fees(&amount); let deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees - deposit); assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), fees); @@ -113,7 +113,7 @@ fn create_subscription_fails_if_insufficient_balance() { frequency, None ), - Error::::InsufficientBalance + TokenError::FundsUnavailable ); // Assert the SubscriptionCreated event was not emitted @@ -267,7 +267,7 @@ fn test_update_subscription() { let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); let original_fees = - ::FeesManager::calculate_subscription_fees(original_amount); + ::FeesManager::calculate_subscription_fees(&original_amount); let original_deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); let balance_after_create = initial_balance - original_fees - original_deposit; @@ -287,11 +287,12 @@ fn test_update_subscription() { new_frequency )); - let new_fees = ::FeesManager::calculate_subscription_fees(new_amount); + let new_fees = ::FeesManager::calculate_subscription_fees(&new_amount); let new_deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); let fees_diff = new_fees - original_fees; + // TODO: test a diff > 0 let deposit_diff = new_deposit - original_deposit; let balance_after_update = balance_after_create - fees_diff - deposit_diff; diff --git a/pallets/idn-manager/src/traits.rs b/pallets/idn-manager/src/traits.rs index 6a79a7d..f1deb0c 100644 --- a/pallets/idn-manager/src/traits.rs +++ b/pallets/idn-manager/src/traits.rs @@ -24,38 +24,38 @@ pub enum FeesError { Other(Context), } -/// Enum to represent the direction of fees movement. +/// Enum to represent the direction of balance movement. #[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub enum FeesDirection { +pub enum BalanceDirection { Hold, Release, - // Fees aren't going anywhere. This is usually the case when diff is zero. + // Balance isn't going anywhere. This is usually the case when diff is zero. None, } -/// This struct represent movement of fees. +/// This struct represent movement of balance. /// -/// * `fees` - how much fees are being moved. -/// * `direction` - if the fees are being collected or released. +/// * `balance` - how much balance being moved. +/// * `direction` - if the balance are being collected or released. #[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub struct DiffFees { - pub fees: Fees, - pub direction: FeesDirection, +pub struct DiffBalance { + pub balance: Balance, + pub direction: BalanceDirection, } /// Trait for fees managing pub trait FeesManager, Err, S> { /// Calculate the fees for a subscription based on the amount of random values required. - fn calculate_subscription_fees(amount: Amount) -> Fees; + fn calculate_subscription_fees(amount: &Amount) -> Fees; /// Calculate how much fees should be held or release when a subscription changes. /// /// * `old_amount` - the amount of random values required before the change. /// * `new_amount` - the amount of random values required after the change, this will represent /// the updated amount in an update operation. Or the amount actually consumed in a kill /// operation. - fn calculate_diff_fees(old_amount: Amount, new_amount: Amount) -> DiffFees; + fn calculate_diff_fees(old_amount: &Amount, new_amount: &Amount) -> DiffBalance; /// Distributes collected fees. Returns the fees that were effectively collected. - fn collect_fees(fees: Fees, sub: Sub) -> Result>; + fn collect_fees(fees: &Fees, sub: &Sub) -> Result>; } pub trait Subscription { @@ -66,5 +66,11 @@ pub trait Subscription { /// /// This trait is used to calculate the storage deposit required for a subscription based it. pub trait DepositCalculator { + /// Calculate the storage deposit required for a subscription. fn calculate_storage_deposit(sub: &Sub) -> Deposit; + /// Calculate the difference in storage deposit between two subscriptions. + /// + /// * `old_sub` - the old subscription. + /// * `new_sub` - the new subscription. + fn calculate_diff_deposit(old_sub: &Sub, new_sub: &Sub) -> DiffBalance; } From 3d026cdd043793d16af705c86d1c3274f29748ce Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 25 Feb 2025 12:37:55 +0100 Subject: [PATCH 03/10] feat: add fees management --- .gitignore | 4 +- pallets/idn-manager/src/impls.rs | 10 +- pallets/idn-manager/src/lib.rs | 8 +- pallets/idn-manager/src/tests/pallet.rs | 325 ++++++++++++++++++------ tarpaulin.toml | 2 +- 5 files changed, 268 insertions(+), 81 deletions(-) diff --git a/.gitignore b/.gitignore index c786b7e..4e2f0cb 100644 --- a/.gitignore +++ b/.gitignore @@ -38,4 +38,6 @@ release.json rls*.log runtime/wasm/target/ substrate.code-workspace -target*/ \ No newline at end of file +target*/ +*.profraw +tarpaulin-report.html \ No newline at end of file diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index 82c72a3..da09585 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -130,14 +130,18 @@ pub struct DepositCalculatorImpl, Deposit> { impl< S: SubscriptionTrait + Encode, SDMultiplier: Get, - Deposit: From + Saturating + Ord, + Deposit: Saturating + From + Ord, > pallet_idn_manager::DepositCalculator for DepositCalculatorImpl { fn calculate_storage_deposit(sub: &S) -> Deposit { + // This function could theoretically saturate to the `Deposit` type bounds. Its result type + // has an upper bound, which is Deposit::MAX, while unlikely and very expensive to + // the attacker, if Deposit type (e.g. u32) is bigger than usize machine architecture (e.g. + // 64 bits) there could be subscription object larger than u32::MAX bits, let’s say u32::MAX + // + d and only pay a deposit for u32::MAX and not d. Let's assess it with SRLabs. let storage_deposit_multiplier = SDMultiplier::get(); - // calculate the size of scale encoded `sub` - let encoded_size = sub.encode().len() as u32; + let encoded_size = u64::try_from(sub.encoded_size()).unwrap_or(u64::MAX); storage_deposit_multiplier.saturating_mul(encoded_size.into()) } diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index 7bac444..fab4323 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -105,7 +105,7 @@ pub type SubscriptionOf = pub struct Subscription { details: SubscriptionDetails, // Number of random values left to distribute - credits_left: BlockNumber, + amount_left: BlockNumber, state: SubscriptionState, } @@ -270,7 +270,7 @@ pub mod pallet { fn on_finalize(_n: BlockNumberFor) { // Look for subscriptions that should be finished for (sub_id, _) in - Subscriptions::::iter().filter(|(_, sub)| sub.credits_left == Zero::zero()) + Subscriptions::::iter().filter(|(_, sub)| sub.amount_left == Zero::zero()) { // finish the subscription Self::finish_subscription(sub_id); @@ -362,7 +362,7 @@ pub mod pallet { &sub, &Subscription { state: sub.state.clone(), - credits_left: amount, + amount_left: amount, details: sub.details.clone(), }, ); @@ -464,7 +464,7 @@ impl Pallet { metadata: metadata.unwrap_or_default(), }; let subscription = - Subscription { state: SubscriptionState::Active, credits_left: amount, details }; + Subscription { state: SubscriptionState::Active, amount_left: amount, details }; Self::hold_deposit( &subscriber, diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index 939ee6f..6b38b8b 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -18,7 +18,7 @@ use crate::{ tests::mock::{Balances, ExtBuilder, Test, *}, - traits::{DepositCalculator, FeesManager}, + traits::{BalanceDirection, DepositCalculator, DiffBalance, FeesManager}, Config, Error, Event, HoldReason, SubscriptionState, Subscriptions, }; use frame_support::{ @@ -48,6 +48,99 @@ fn event_emitted(event: Event) -> bool { }) } +fn update_subscription( + subscriber: AccountId32, + original_amount: u64, + original_frequency: u64, + new_amount: u64, + new_frequency: u64, +) { + let target = Location::new(1, [Junction::PalletInstance(1)]); + let metadata = None; + let initial_balance = 99_990_000_000_000_000; + + ::Currency::set_balance(&subscriber, initial_balance); + + assert_ok!(IdnManager::create_subscription( + RuntimeOrigin::signed(subscriber.clone()), + original_amount, + target.clone(), + original_frequency, + metadata.clone() + )); + + // Get the sub_id from the last emitted event + let sub_id = System::events() + .iter() + .rev() + .find_map(|record| { + if let RuntimeEvent::IdnManager(Event::::SubscriptionCreated { sub_id }) = + &record.event + { + Some(*sub_id) + } else { + None + } + }) + .expect("SubscriptionCreated event should be emitted"); + + let subscription = Subscriptions::::get(sub_id).unwrap(); + + assert_eq!(subscription.details.subscriber, subscriber); + + let original_fees = + ::FeesManager::calculate_subscription_fees(&original_amount); + let original_deposit = + ::DepositCalculator::calculate_storage_deposit(&subscription); + let balance_after_create = initial_balance - original_fees - original_deposit; + + // assert correct balance on subscriber after creating subscription + assert_eq!(Balances::free_balance(&subscriber), balance_after_create); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &subscriber), original_fees); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &subscriber), + original_deposit + ); + + assert_ok!(IdnManager::update_subscription( + RuntimeOrigin::signed(subscriber.clone()), + sub_id, + new_amount, + new_frequency + )); + + let new_fees = ::FeesManager::calculate_subscription_fees(&new_amount); + let new_deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); + + let fees_diff: i64 = new_fees as i64 - original_fees as i64; + // TODO: test a diff > 0 + let deposit_diff: i64 = new_deposit as i64 - original_deposit as i64; + + assert!(deposit_diff.is_zero()); + + let balance_after_update: u64 = + (balance_after_create as i64 - fees_diff - deposit_diff).try_into().unwrap(); + + // assert fees and deposit diff is correctly handled + assert_eq!(Balances::free_balance(&subscriber), balance_after_update); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &subscriber), new_fees); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &subscriber), + new_deposit + ); + assert_eq!(balance_after_update + new_fees + new_deposit, initial_balance); + + // TODO: test probably in a separate test case fees handling but by adding block advance + + let subscription = Subscriptions::::get(sub_id).unwrap(); + + // assert subscription details has been updated + assert_eq!(subscription.details.amount, new_amount); + assert_eq!(subscription.details.frequency, new_frequency); + + assert!(event_emitted(Event::::SubscriptionUpdated { sub_id })); +} + #[test] fn create_subscription_works() { ExtBuilder::build().execute_with(|| { @@ -244,77 +337,47 @@ fn kill_subscription_fails_if_sub_does_not_exist() { #[test] fn test_update_subscription() { ExtBuilder::build().execute_with(|| { - let original_amount = 10; - let original_frequency = 2; - // TODO as part of https://github.com/ideal-lab5/idn-sdk/issues/104 - // make these two variables dynamic and test lt and gt - let new_amount = 20; - let new_frequency = 4; - let target = Location::new(1, [Junction::PalletInstance(1)]); - let metadata = None; - let initial_balance = 10_000_000; - - ::Currency::set_balance(&ALICE, initial_balance); - - assert_ok!(IdnManager::create_subscription( - RuntimeOrigin::signed(ALICE.clone()), - original_amount, - target.clone(), - original_frequency, - metadata.clone() - )); - - let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); - - let original_fees = - ::FeesManager::calculate_subscription_fees(&original_amount); - let original_deposit = - ::DepositCalculator::calculate_storage_deposit(&subscription); - let balance_after_create = initial_balance - original_fees - original_deposit; - - // assert correct balance on subscriber after creating subscription - assert_eq!(Balances::free_balance(&ALICE), balance_after_create); - assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), original_fees); - assert_eq!( - Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), - original_deposit - ); - - assert_ok!(IdnManager::update_subscription( - RuntimeOrigin::signed(ALICE), - sub_id, - new_amount, - new_frequency - )); - - let new_fees = ::FeesManager::calculate_subscription_fees(&new_amount); - let new_deposit = - ::DepositCalculator::calculate_storage_deposit(&subscription); - - let fees_diff = new_fees - original_fees; - // TODO: test a diff > 0 - let deposit_diff = new_deposit - original_deposit; - - let balance_after_update = balance_after_create - fees_diff - deposit_diff; - - // assert fees and deposit diff is correctly handled - assert_eq!(Balances::free_balance(&ALICE), balance_after_update); - assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), new_fees); - assert_eq!( - Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), - new_deposit - ); - assert_eq!(balance_after_update + new_fees + new_deposit, initial_balance); - - // TODO: test probably in a separate test case fees handling but by adding block advance - - let subscription = Subscriptions::::get(sub_id).unwrap(); - - // assert subscription details has been updated - assert_eq!(subscription.details.amount, new_amount); - assert_eq!(subscription.details.frequency, new_frequency); + struct SubParams { + amount: u64, + frequency: u64, + } + struct SubUpdate { + old: SubParams, + new: SubParams, + } - assert!(event_emitted(Event::::SubscriptionUpdated { sub_id })); + let updates: Vec = vec![ + SubUpdate { + old: SubParams { amount: 10, frequency: 2 }, + new: SubParams { amount: 20, frequency: 4 }, + }, + SubUpdate { + old: SubParams { amount: 100, frequency: 20 }, + new: SubParams { amount: 20, frequency: 4 }, + }, + SubUpdate { + old: SubParams { amount: 100, frequency: 20 }, + new: SubParams { amount: 100, frequency: 20 }, + }, + SubUpdate { + old: SubParams { amount: 100, frequency: 1 }, + new: SubParams { amount: 9_999_999_999_999, frequency: 1 }, + }, + SubUpdate { + old: SubParams { amount: 9_999_999_999_999, frequency: 1 }, + new: SubParams { amount: 100, frequency: 1 }, + }, + ]; + for i in 0..updates.len() { + let update = &updates[i]; + update_subscription( + AccountId32::new([i.try_into().unwrap(); 32]), + update.old.amount, + update.old.frequency, + update.new.amount, + update.new.frequency, + ); + } }); } @@ -573,7 +636,7 @@ fn test_on_finalize_removes_finished_subscriptions() { let (sub_id, mut subscription) = Subscriptions::::iter().next().unwrap(); // Manually set credits to zero to simulate a finished subscription - subscription.credits_left = Zero::zero(); + subscription.amount_left = Zero::zero(); Subscriptions::::insert(sub_id, subscription); // Before on_finalize, subscription should exist @@ -590,3 +653,121 @@ fn test_on_finalize_removes_finished_subscriptions() { assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); }); } + +#[test] +fn hold_deposit_works() { + ExtBuilder::build().execute_with(|| { + let initial_balance = 10_000_000; + let deposit_amount = 1_000; + + // Setup account with initial balance + ::Currency::set_balance(&ALICE, initial_balance); + + // Hold deposit + assert_ok!(crate::Pallet::::hold_deposit(&ALICE, deposit_amount)); + + // Verify deposit is held + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), + deposit_amount + ); + // Verify free balance is reduced + assert_eq!(Balances::free_balance(&ALICE), initial_balance - deposit_amount); + }); +} + +#[test] +fn release_deposit_works() { + ExtBuilder::build().execute_with(|| { + let initial_balance = 10_000_000; + let deposit_amount = 1_000; + + // Setup account and hold deposit + ::Currency::set_balance(&ALICE, initial_balance); + assert_ok!(crate::Pallet::::hold_deposit(&ALICE, deposit_amount)); + + // Release deposit + assert_ok!(crate::Pallet::::release_deposit(&ALICE, deposit_amount)); + + // Verify deposit is released + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0); + // Verify free balance is restored + assert_eq!(Balances::free_balance(&ALICE), initial_balance); + }); +} + +#[test] +fn manage_diff_deposit_works() { + ExtBuilder::build().execute_with(|| { + let initial_balance = 10_000_000; + let original_deposit = 1_000; + let additional_deposit = 1_500; + let excess_deposit = 500; + + // Setup account with initial balance + ::Currency::set_balance(&ALICE, initial_balance); + + // Test holding deposit + let hold_diff = + DiffBalance { balance: original_deposit, direction: BalanceDirection::Hold }; + assert_ok!(crate::Pallet::::manage_diff_deposit(&ALICE, &hold_diff)); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), + original_deposit + ); + // Test holding additional deposit + let hold_diff = + DiffBalance { balance: additional_deposit, direction: BalanceDirection::Hold }; + assert_ok!(crate::Pallet::::manage_diff_deposit(&ALICE, &hold_diff)); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), + original_deposit + additional_deposit + ); + + // Test releasing excess deposit + let release_diff = + DiffBalance { balance: excess_deposit, direction: BalanceDirection::Release }; + assert_ok!(crate::Pallet::::manage_diff_deposit(&ALICE, &release_diff)); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), + original_deposit + additional_deposit - excess_deposit + ); + + // Test no change in deposit + let no_change_diff = DiffBalance { balance: 0, direction: BalanceDirection::None }; + let held_before = Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE); + assert_ok!(crate::Pallet::::manage_diff_deposit(&ALICE, &no_change_diff)); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), + held_before + ); + + // assert free balance + assert_eq!( + Balances::free_balance(&ALICE), + initial_balance - original_deposit - additional_deposit + excess_deposit + ); + }); +} + +#[test] +fn hold_deposit_fails_with_insufficient_balance() { + ExtBuilder::build().execute_with(|| { + let initial_balance = 500; + let deposit_amount = 1_000; + + // Setup account with insufficient balance + ::Currency::set_balance(&ALICE, initial_balance); + + // Attempt to hold deposit should fail + assert_noop!( + crate::Pallet::::hold_deposit(&ALICE, deposit_amount), + TokenError::FundsUnavailable + ); + + // Verify no deposit is held + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0); + // Verify balance remains unchanged + assert_eq!(Balances::free_balance(&ALICE), initial_balance); + }); +} diff --git a/tarpaulin.toml b/tarpaulin.toml index a3b33e8..83af434 100644 --- a/tarpaulin.toml +++ b/tarpaulin.toml @@ -2,4 +2,4 @@ [test_config] # List of file paths to exclude from testing. -exclude-files = ["solochain/*", "pallets/drand/examples/solochain/*"] \ No newline at end of file +exclude-files = ["solochain/*", "pallets/drand/examples/solochain/*", "*/**/weights.rs"] \ No newline at end of file From 7ad10013d9750710e26c716f6276ed345d1ff140 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Mon, 3 Mar 2025 13:27:06 +0100 Subject: [PATCH 04/10] wip --- pallets/idn-manager/src/impls.rs | 4 +- pallets/idn-manager/src/lib.rs | 55 ++++++++---- pallets/idn-manager/src/tests/fee_examples.rs | 16 ++-- pallets/idn-manager/src/tests/pallet.rs | 89 ++++++++++++++++--- pallets/idn-manager/src/traits.rs | 2 +- 5 files changed, 126 insertions(+), 40 deletions(-) diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index da09585..4753d6b 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -82,7 +82,7 @@ where let mut direction = BalanceDirection::None; let fees = match new_amount.cmp(&old_amount) { Ordering::Greater => { - direction = BalanceDirection::Hold; + direction = BalanceDirection::Collect; Self::calculate_subscription_fees( &new_amount.clone().saturating_sub(old_amount.clone()), ) @@ -149,7 +149,7 @@ impl< let old_deposit = Self::calculate_storage_deposit(old_sub); let new_deposit = Self::calculate_storage_deposit(new_sub); let direction = match new_deposit.cmp(&old_deposit) { - Ordering::Greater => BalanceDirection::Hold, + Ordering::Greater => BalanceDirection::Collect, Ordering::Less => BalanceDirection::Release, Ordering::Equal => BalanceDirection::None, }; diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index fab4323..1883cd3 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -79,6 +79,7 @@ use scale_info::TypeInfo; use sp_arithmetic::traits::Unsigned; use sp_core::H256; use sp_io::hashing::blake2_256; +use sp_runtime::{traits::One, Saturating}; use sp_std::fmt::Debug; use traits::{ BalanceDirection, DepositCalculator, DiffBalance, FeesManager, @@ -269,11 +270,11 @@ pub mod pallet { impl Hooks> for Pallet { fn on_finalize(_n: BlockNumberFor) { // Look for subscriptions that should be finished - for (sub_id, _) in + for (sub_id, sub) in Subscriptions::::iter().filter(|(_, sub)| sub.amount_left == Zero::zero()) { // finish the subscription - Self::finish_subscription(sub_id); + let _ = Self::finish_subscription(&sub, sub_id); } } } @@ -329,15 +330,12 @@ pub mod pallet { sub_id: SubscriptionId, ) -> DispatchResult { let subscriber = ensure_signed(origin)?; - // `try_mutate_exists` deletes maybe_sub if mutated to a `None`` - Subscriptions::::try_mutate_exists(sub_id, |maybe_sub| { - // Takes the value out of `maybe_sub``, leaving a `None`` in its place. - let sub = maybe_sub.take().ok_or(Error::::SubscriptionDoesNotExist)?; - ensure!(sub.details.subscriber == subscriber, Error::::NotSubscriber); - // TODO: refund storage and fees https://github.com/ideal-lab5/idn-sdk/issues/107 - Self::deposit_event(Event::SubscriptionRemoved { sub_id }); - Ok(()) - }) + + let sub = + Subscriptions::::get(sub_id).ok_or(Error::::SubscriptionDoesNotExist)?; + ensure!(sub.details.subscriber == subscriber, Error::::NotSubscriber); + + Self::finish_subscription(&sub, sub_id) } /// Updates a subscription @@ -406,9 +404,22 @@ pub mod pallet { impl Pallet { /// Finishes a subscription by removing it from storage and emitting a finish event. - pub(crate) fn finish_subscription(sub_id: SubscriptionId) { + pub(crate) fn finish_subscription( + sub: &SubscriptionOf, + sub_id: SubscriptionId, + ) -> DispatchResult { + // fees left and deposit to refund + let fees_diff = T::FeesManager::calculate_diff_fees(&sub.amount_left, &Zero::zero()); + let sd = T::DepositCalculator::calculate_storage_deposit(&sub); + + Self::manage_diff_fees(&sub.details.subscriber, &fees_diff)?; + Self::release_deposit(&sub.details.subscriber, sd)?; + + Self::deposit_event(Event::SubscriptionRemoved { sub_id }); + Subscriptions::::remove(sub_id); Self::deposit_event(Event::SubscriptionRemoved { sub_id }); + Ok(()) } fn pallet_account_id() -> T::AccountId { @@ -429,17 +440,23 @@ impl Pallet { let versioned_msg: Box> = Box::new(xcm::VersionedXcm::V5(msg.into())); let origin = frame_system::RawOrigin::Signed(Self::pallet_account_id()); - let _ = T::Xcm::send(origin.into(), versioned_target, versioned_msg); - - // todo: as part of issue #77 use saturating_sub to make sure it does not underflow - - Self::deposit_event(Event::RandomnessDistributed { sub_id }); + if T::Xcm::send(origin.into(), versioned_target, versioned_msg).is_ok() { + Self::consume_amount(&sub_id, sub); + Self::deposit_event(Event::RandomnessDistributed { sub_id }); + } } } Ok(()) } + fn consume_amount(sub_id: &SubscriptionId, mut sub: SubscriptionOf) { + // Decrease amount_left by one using saturating_sub + sub.amount_left = sub.amount_left.saturating_sub(One::one()); + // Update the subscription in storage + Subscriptions::::insert(sub_id, sub) + } + /// Internal function to handle subscription creation fn create_subscription_internal( subscriber: T::AccountId, @@ -496,7 +513,7 @@ impl Pallet { diff: &DiffBalance>, ) -> DispatchResult { match diff.direction { - BalanceDirection::Hold => Self::hold_fees(subscriber, diff.balance), + BalanceDirection::Collect => Self::hold_fees(subscriber, diff.balance), BalanceDirection::Release => Self::release_fees(subscriber, diff.balance), BalanceDirection::None => Ok(()), } @@ -521,7 +538,7 @@ impl Pallet { diff: &DiffBalance>, ) -> DispatchResult { match diff.direction { - BalanceDirection::Hold => Self::hold_deposit(subscriber, diff.balance), + BalanceDirection::Collect => Self::hold_deposit(subscriber, diff.balance), BalanceDirection::Release => Self::release_deposit(subscriber, diff.balance), BalanceDirection::None => Ok(()), } diff --git a/pallets/idn-manager/src/tests/fee_examples.rs b/pallets/idn-manager/src/tests/fee_examples.rs index 0b71358..6850aac 100644 --- a/pallets/idn-manager/src/tests/fee_examples.rs +++ b/pallets/idn-manager/src/tests/fee_examples.rs @@ -35,7 +35,7 @@ impl FeesManager for LinearFeeCalculator { let mut direction = BalanceDirection::None; let fees = match new_amount.cmp(&old_amount) { Ordering::Greater => { - direction = BalanceDirection::Hold; + direction = BalanceDirection::Collect; Self::calculate_subscription_fees( &new_amount.clone().saturating_sub(old_amount.clone()), ) @@ -102,7 +102,7 @@ impl FeesManager for SteppedTieredFeeCalculator { let mut direction = BalanceDirection::None; let fees = match new_fees.cmp(&old_fees) { Ordering::Greater => { - direction = BalanceDirection::Hold; + direction = BalanceDirection::Collect; new_fees - old_fees }, Ordering::Less => { @@ -165,11 +165,11 @@ mod tests { fn test_calculate_hold_linear_fees_greater_diff() { let old_amount: u32 = 50; let new_amount: u32 = 60; // all credits used - let refund = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let hold = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); let expected = 10 * BASE_FEE; assert_eq!( - refund, - DiffBalance { balance: expected, direction: BalanceDirection::Hold }, + hold, + DiffBalance { balance: expected, direction: BalanceDirection::Collect }, "There should be more held balance" ); } @@ -243,11 +243,11 @@ mod tests { fn test_calculate_hold_stepped_fees_greater_diff() { let old_amount: u32 = 100; let new_amount: u32 = 101; // 1 value increase - let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let hold = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); let expected = 1 * BASE_FEE.saturating_mul(9_000).saturating_div(10_000); // 10% discount on the extra value assert_eq!( - refund, - DiffBalance { balance: expected, direction: BalanceDirection::Hold }, + hold, + DiffBalance { balance: expected, direction: BalanceDirection::Collect }, "There should be more held balance" ); } diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index 6b38b8b..1ec551a 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -113,9 +113,12 @@ fn update_subscription( let new_deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); let fees_diff: i64 = new_fees as i64 - original_fees as i64; - // TODO: test a diff > 0 + let deposit_diff: i64 = new_deposit as i64 - original_deposit as i64; + // We are using fixed-width integer types for amount and frequency, so Subscription objects + // can't change in size with this mock. Unit tests are in place insted to ensure the correct + // behaviour in case of other types used. assert!(deposit_diff.is_zero()); let balance_after_update: u64 = @@ -130,7 +133,8 @@ fn update_subscription( ); assert_eq!(balance_after_update + new_fees + new_deposit, initial_balance); - // TODO: test probably in a separate test case fees handling but by adding block advance + // TODO: test probably in a separate test case fees handling but by adding block advance, and + // therefore amount consuption let subscription = Subscriptions::::get(sub_id).unwrap(); @@ -295,8 +299,9 @@ fn test_kill_subscription() { let frequency = 2; let target = Location::new(1, [Junction::PalletInstance(1)]); let metadata = None; + let initial_balance = 10_000_000; - ::Currency::set_balance(&ALICE, 10_000_000); + ::Currency::set_balance(&ALICE, initial_balance); assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), @@ -306,15 +311,26 @@ fn test_kill_subscription() { metadata.clone() )); - let (sub_id, _) = Subscriptions::::iter().next().unwrap(); + let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); + + // assert that the correct fees have been held + let fees = ::FeesManager::calculate_subscription_fees(&amount); + let deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); + assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees - deposit); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), fees); + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), deposit); // TOOD assert: - // - correct fees are refunded - // - correct storage deposit is refunded - // - correct fees were collected - // https://github.com/ideal-lab5/idn-sdk/issues/107 + // - correct fees were collected. Test probably in a separate test case fees handling but by + // adding block advance, + // and therefore amount consuption assert_ok!(IdnManager::kill_subscription(RuntimeOrigin::signed(ALICE), sub_id)); assert!(Subscriptions::::get(sub_id).is_none()); + // assert remaining fees and balance refunded + assert_eq!(Balances::free_balance(&ALICE), initial_balance); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0u64); + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0u64); + assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); }); } @@ -334,6 +350,59 @@ fn kill_subscription_fails_if_sub_does_not_exist() { }); } +#[test] +fn on_finalize_removes_zero_credit_subscriptions() { + ExtBuilder::build().execute_with(|| { + // Setup - Create a subscription + let amount: u64 = 50; + let target = Location::new(1, [Junction::PalletInstance(1)]); + let frequency: u64 = 10; + let initial_balance = 10_000_000; + + ::Currency::set_balance(&ALICE, initial_balance); + + assert_ok!(IdnManager::create_subscription( + RuntimeOrigin::signed(ALICE.clone()), + amount, + target.clone(), + frequency, + None + )); + + // Get the subscription ID + let (sub_id, mut subscription) = Subscriptions::::iter().next().unwrap(); + + let fees = ::FeesManager::calculate_subscription_fees(&amount); + let deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); + assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees - deposit); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), fees); + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), deposit); + + // Manually set credits to zero + subscription.amount_left = Zero::zero(); + Subscriptions::::insert(sub_id, subscription); + + // Verify subscription exists before on_finalize + assert!(Subscriptions::::contains_key(sub_id)); + + // Call on_finalize directly + let current_block = System::block_number(); + crate::Pallet::::on_finalize(current_block); + + // Verify subscription was removed + assert!(!Subscriptions::::contains_key(sub_id)); + + // assert there are no remaining fees and balance refunded + assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees); + // TODO: once fees collection in place uncomment this line + // assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0u64); + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0u64); + + // Verify event was emitted + assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); + }); +} + #[test] fn test_update_subscription() { ExtBuilder::build().execute_with(|| { @@ -709,7 +778,7 @@ fn manage_diff_deposit_works() { // Test holding deposit let hold_diff = - DiffBalance { balance: original_deposit, direction: BalanceDirection::Hold }; + DiffBalance { balance: original_deposit, direction: BalanceDirection::Collect }; assert_ok!(crate::Pallet::::manage_diff_deposit(&ALICE, &hold_diff)); assert_eq!( Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), @@ -717,7 +786,7 @@ fn manage_diff_deposit_works() { ); // Test holding additional deposit let hold_diff = - DiffBalance { balance: additional_deposit, direction: BalanceDirection::Hold }; + DiffBalance { balance: additional_deposit, direction: BalanceDirection::Collect }; assert_ok!(crate::Pallet::::manage_diff_deposit(&ALICE, &hold_diff)); assert_eq!( Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), diff --git a/pallets/idn-manager/src/traits.rs b/pallets/idn-manager/src/traits.rs index f1deb0c..face2bb 100644 --- a/pallets/idn-manager/src/traits.rs +++ b/pallets/idn-manager/src/traits.rs @@ -27,7 +27,7 @@ pub enum FeesError { /// Enum to represent the direction of balance movement. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub enum BalanceDirection { - Hold, + Collect, Release, // Balance isn't going anywhere. This is usually the case when diff is zero. None, From 4116dae1853efbff3048f1ddce49e7197dc69283 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Mon, 3 Mar 2025 13:32:51 +0100 Subject: [PATCH 05/10] feat: rename `amount` to `credits` --- pallets/idn-manager/src/lib.rs | 40 ++++---- pallets/idn-manager/src/tests/pallet.rs | 130 ++++++++++++------------ 2 files changed, 85 insertions(+), 85 deletions(-) diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index 1883cd3..e07f723 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -106,7 +106,7 @@ pub type SubscriptionOf = pub struct Subscription { details: SubscriptionDetails, // Number of random values left to distribute - amount_left: BlockNumber, + credits_left: BlockNumber, state: SubscriptionState, } @@ -117,7 +117,7 @@ pub struct SubscriptionDetails { subscriber: AccountId, created_at: BlockNumber, updated_at: BlockNumber, - amount: BlockNumber, + credits: BlockNumber, frequency: BlockNumber, target: Location, metadata: Metadata, @@ -271,7 +271,7 @@ pub mod pallet { fn on_finalize(_n: BlockNumberFor) { // Look for subscriptions that should be finished for (sub_id, sub) in - Subscriptions::::iter().filter(|(_, sub)| sub.amount_left == Zero::zero()) + Subscriptions::::iter().filter(|(_, sub)| sub.credits_left == Zero::zero()) { // finish the subscription let _ = Self::finish_subscription(&sub, sub_id); @@ -287,7 +287,7 @@ pub mod pallet { pub fn create_subscription( origin: OriginFor, // Number of random values to receive - amount: BlockNumberFor, + credits: BlockNumberFor, // XCM multilocation for random value delivery target: Location, // Distribution interval for random values @@ -296,7 +296,7 @@ pub mod pallet { metadata: Option>, ) -> DispatchResult { let subscriber = ensure_signed(origin)?; - Self::create_subscription_internal(subscriber, amount, target, frequency, metadata) + Self::create_subscription_internal(subscriber, credits, target, frequency, metadata) } /// Temporarily halts randomness distribution @@ -346,7 +346,7 @@ pub mod pallet { origin: OriginFor, sub_id: SubscriptionId, // New number of random values - amount: BlockNumberFor, + credits: BlockNumberFor, // New distribution interval frequency: BlockNumberFor, ) -> DispatchResult { @@ -355,17 +355,17 @@ pub mod pallet { let sub = maybe_sub.as_mut().ok_or(Error::::SubscriptionDoesNotExist)?; ensure!(sub.details.subscriber == subscriber, Error::::NotSubscriber); - let fees_diff = T::FeesManager::calculate_diff_fees(&sub.details.amount, &amount); + let fees_diff = T::FeesManager::calculate_diff_fees(&sub.details.credits, &credits); let deposit_diff = T::DepositCalculator::calculate_diff_deposit( &sub, &Subscription { state: sub.state.clone(), - amount_left: amount, + credits_left: credits, details: sub.details.clone(), }, ); - sub.details.amount = amount; + sub.details.credits = credits; sub.details.frequency = frequency; sub.details.updated_at = frame_system::Pallet::::block_number(); @@ -409,7 +409,7 @@ impl Pallet { sub_id: SubscriptionId, ) -> DispatchResult { // fees left and deposit to refund - let fees_diff = T::FeesManager::calculate_diff_fees(&sub.amount_left, &Zero::zero()); + let fees_diff = T::FeesManager::calculate_diff_fees(&sub.credits_left, &Zero::zero()); let sd = T::DepositCalculator::calculate_storage_deposit(&sub); Self::manage_diff_fees(&sub.details.subscriber, &fees_diff)?; @@ -441,7 +441,7 @@ impl Pallet { Box::new(xcm::VersionedXcm::V5(msg.into())); let origin = frame_system::RawOrigin::Signed(Self::pallet_account_id()); if T::Xcm::send(origin.into(), versioned_target, versioned_msg).is_ok() { - Self::consume_amount(&sub_id, sub); + Self::consume_credits(&sub_id, sub); Self::deposit_event(Event::RandomnessDistributed { sub_id }); } } @@ -450,9 +450,9 @@ impl Pallet { Ok(()) } - fn consume_amount(sub_id: &SubscriptionId, mut sub: SubscriptionOf) { - // Decrease amount_left by one using saturating_sub - sub.amount_left = sub.amount_left.saturating_sub(One::one()); + fn consume_credits(sub_id: &SubscriptionId, mut sub: SubscriptionOf) { + // Decrease credits_left by one using saturating_sub + sub.credits_left = sub.credits_left.saturating_sub(One::one()); // Update the subscription in storage Subscriptions::::insert(sub_id, sub) } @@ -460,13 +460,13 @@ impl Pallet { /// Internal function to handle subscription creation fn create_subscription_internal( subscriber: T::AccountId, - amount: BlockNumberFor, + credits: BlockNumberFor, target: Location, frequency: BlockNumberFor, metadata: Option>, ) -> DispatchResult { // Calculate and hold the subscription fees - let fees = T::FeesManager::calculate_subscription_fees(&amount); + let fees = T::FeesManager::calculate_subscription_fees(&credits); Self::hold_fees(&subscriber, fees)?; @@ -475,13 +475,13 @@ impl Pallet { subscriber: subscriber.clone(), created_at: current_block, updated_at: current_block, - amount, + credits, target: target.clone(), frequency, metadata: metadata.unwrap_or_default(), }; let subscription = - Subscription { state: SubscriptionState::Active, amount_left: amount, details }; + Subscription { state: SubscriptionState::Active, credits_left: credits, details }; Self::hold_deposit( &subscriber, @@ -575,10 +575,10 @@ sp_api::decl_runtime_apis! { Metadata: Codec, AccountId: Codec, { - /// Computes the fee for a given amount of random values to receive + /// Computes the fee for a given credits fn calculate_subscription_fees( // Number of random values to receive - amount: BlockNumber + credits: BlockNumber ) -> Balance; /// Retrieves a specific subscription diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index 1ec551a..63004cc 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -50,9 +50,9 @@ fn event_emitted(event: Event) -> bool { fn update_subscription( subscriber: AccountId32, - original_amount: u64, + original_credits: u64, original_frequency: u64, - new_amount: u64, + new_credits: u64, new_frequency: u64, ) { let target = Location::new(1, [Junction::PalletInstance(1)]); @@ -63,7 +63,7 @@ fn update_subscription( assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(subscriber.clone()), - original_amount, + original_credits, target.clone(), original_frequency, metadata.clone() @@ -89,7 +89,7 @@ fn update_subscription( assert_eq!(subscription.details.subscriber, subscriber); let original_fees = - ::FeesManager::calculate_subscription_fees(&original_amount); + ::FeesManager::calculate_subscription_fees(&original_credits); let original_deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); let balance_after_create = initial_balance - original_fees - original_deposit; @@ -105,18 +105,18 @@ fn update_subscription( assert_ok!(IdnManager::update_subscription( RuntimeOrigin::signed(subscriber.clone()), sub_id, - new_amount, + new_credits, new_frequency )); - let new_fees = ::FeesManager::calculate_subscription_fees(&new_amount); + let new_fees = ::FeesManager::calculate_subscription_fees(&new_credits); let new_deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); let fees_diff: i64 = new_fees as i64 - original_fees as i64; let deposit_diff: i64 = new_deposit as i64 - original_deposit as i64; - // We are using fixed-width integer types for amount and frequency, so Subscription objects + // We are using fixed-width integer types for credits and frequency, so Subscription objects // can't change in size with this mock. Unit tests are in place insted to ensure the correct // behaviour in case of other types used. assert!(deposit_diff.is_zero()); @@ -134,12 +134,12 @@ fn update_subscription( assert_eq!(balance_after_update + new_fees + new_deposit, initial_balance); // TODO: test probably in a separate test case fees handling but by adding block advance, and - // therefore amount consuption + // therefore credits consuption let subscription = Subscriptions::::get(sub_id).unwrap(); // assert subscription details has been updated - assert_eq!(subscription.details.amount, new_amount); + assert_eq!(subscription.details.credits, new_credits); assert_eq!(subscription.details.frequency, new_frequency); assert!(event_emitted(Event::::SubscriptionUpdated { sub_id })); @@ -148,7 +148,7 @@ fn update_subscription( #[test] fn create_subscription_works() { ExtBuilder::build().execute_with(|| { - let amount: u64 = 50; + let credits: u64 = 50; let target = Location::new(1, [Junction::PalletInstance(1)]); let frequency: u64 = 10; let initial_balance = 10_000_000; @@ -161,7 +161,7 @@ fn create_subscription_works() { // assert that the subscription has been created assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, None @@ -172,7 +172,7 @@ fn create_subscription_works() { let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); // assert that the correct fees have been held - let fees = ::FeesManager::calculate_subscription_fees(&amount); + let fees = ::FeesManager::calculate_subscription_fees(&credits); let deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees - deposit); assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), fees); @@ -180,7 +180,7 @@ fn create_subscription_works() { // assert that the subscription details are correct assert_eq!(subscription.details.subscriber, ALICE); - assert_eq!(subscription.details.amount, amount); + assert_eq!(subscription.details.credits, credits); assert_eq!(subscription.details.target, target); assert_eq!(subscription.details.frequency, frequency); assert_eq!( @@ -196,7 +196,7 @@ fn create_subscription_works() { #[test] fn create_subscription_fails_if_insufficient_balance() { ExtBuilder::build().execute_with(|| { - let amount: u64 = 50; + let credits: u64 = 50; let target = Location::new(1, [Junction::PalletInstance(1)]); let frequency: u64 = 10; @@ -205,7 +205,7 @@ fn create_subscription_fails_if_insufficient_balance() { assert_noop!( IdnManager::create_subscription( RuntimeOrigin::signed(ALICE), - amount, + credits, target, frequency, None @@ -224,7 +224,7 @@ fn create_subscription_fails_if_insufficient_balance() { #[test] fn create_subscription_fails_if_sub_already_exists() { ExtBuilder::build().execute_with(|| { - let amount: u64 = 50; + let credits: u64 = 50; let target = Location::new(1, [Junction::PalletInstance(1)]); let frequency: u64 = 10; @@ -232,7 +232,7 @@ fn create_subscription_fails_if_sub_already_exists() { assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, None @@ -244,7 +244,7 @@ fn create_subscription_fails_if_sub_already_exists() { assert_noop!( IdnManager::create_subscription( RuntimeOrigin::signed(ALICE), - amount, + credits, target, frequency, None @@ -266,7 +266,7 @@ fn create_subscription_fails_if_sub_already_exists() { #[ignore] fn distribute_randomness_works() { ExtBuilder::build().execute_with(|| { - let amount: u64 = 50; + let credits: u64 = 50; let target = Location::new(1, [Junction::PalletInstance(1)]); let frequency: u64 = 10; @@ -274,7 +274,7 @@ fn distribute_randomness_works() { assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE), - amount, + credits, target.clone(), frequency, None @@ -295,7 +295,7 @@ fn distribute_randomness_works() { #[test] fn test_kill_subscription() { ExtBuilder::build().execute_with(|| { - let amount = 10; + let credits = 10; let frequency = 2; let target = Location::new(1, [Junction::PalletInstance(1)]); let metadata = None; @@ -305,7 +305,7 @@ fn test_kill_subscription() { assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, metadata.clone() @@ -314,7 +314,7 @@ fn test_kill_subscription() { let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); // assert that the correct fees have been held - let fees = ::FeesManager::calculate_subscription_fees(&amount); + let fees = ::FeesManager::calculate_subscription_fees(&credits); let deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees - deposit); assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), fees); @@ -322,7 +322,7 @@ fn test_kill_subscription() { // TOOD assert: // - correct fees were collected. Test probably in a separate test case fees handling but by // adding block advance, - // and therefore amount consuption + // and therefore credits consuption assert_ok!(IdnManager::kill_subscription(RuntimeOrigin::signed(ALICE), sub_id)); assert!(Subscriptions::::get(sub_id).is_none()); @@ -354,7 +354,7 @@ fn kill_subscription_fails_if_sub_does_not_exist() { fn on_finalize_removes_zero_credit_subscriptions() { ExtBuilder::build().execute_with(|| { // Setup - Create a subscription - let amount: u64 = 50; + let credits: u64 = 50; let target = Location::new(1, [Junction::PalletInstance(1)]); let frequency: u64 = 10; let initial_balance = 10_000_000; @@ -363,7 +363,7 @@ fn on_finalize_removes_zero_credit_subscriptions() { assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, None @@ -372,14 +372,14 @@ fn on_finalize_removes_zero_credit_subscriptions() { // Get the subscription ID let (sub_id, mut subscription) = Subscriptions::::iter().next().unwrap(); - let fees = ::FeesManager::calculate_subscription_fees(&amount); + let fees = ::FeesManager::calculate_subscription_fees(&credits); let deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees - deposit); assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), fees); assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), deposit); // Manually set credits to zero - subscription.amount_left = Zero::zero(); + subscription.credits_left = Zero::zero(); Subscriptions::::insert(sub_id, subscription); // Verify subscription exists before on_finalize @@ -407,7 +407,7 @@ fn on_finalize_removes_zero_credit_subscriptions() { fn test_update_subscription() { ExtBuilder::build().execute_with(|| { struct SubParams { - amount: u64, + credits: u64, frequency: u64, } struct SubUpdate { @@ -417,33 +417,33 @@ fn test_update_subscription() { let updates: Vec = vec![ SubUpdate { - old: SubParams { amount: 10, frequency: 2 }, - new: SubParams { amount: 20, frequency: 4 }, + old: SubParams { credits: 10, frequency: 2 }, + new: SubParams { credits: 20, frequency: 4 }, }, SubUpdate { - old: SubParams { amount: 100, frequency: 20 }, - new: SubParams { amount: 20, frequency: 4 }, + old: SubParams { credits: 100, frequency: 20 }, + new: SubParams { credits: 20, frequency: 4 }, }, SubUpdate { - old: SubParams { amount: 100, frequency: 20 }, - new: SubParams { amount: 100, frequency: 20 }, + old: SubParams { credits: 100, frequency: 20 }, + new: SubParams { credits: 100, frequency: 20 }, }, SubUpdate { - old: SubParams { amount: 100, frequency: 1 }, - new: SubParams { amount: 9_999_999_999_999, frequency: 1 }, + old: SubParams { credits: 100, frequency: 1 }, + new: SubParams { credits: 9_999_999_999_999, frequency: 1 }, }, SubUpdate { - old: SubParams { amount: 9_999_999_999_999, frequency: 1 }, - new: SubParams { amount: 100, frequency: 1 }, + old: SubParams { credits: 9_999_999_999_999, frequency: 1 }, + new: SubParams { credits: 100, frequency: 1 }, }, ]; for i in 0..updates.len() { let update = &updates[i]; update_subscription( AccountId32::new([i.try_into().unwrap(); 32]), - update.old.amount, + update.old.credits, update.old.frequency, - update.new.amount, + update.new.credits, update.new.frequency, ); } @@ -454,14 +454,14 @@ fn test_update_subscription() { fn update_subscription_fails_if_sub_does_not_exists() { ExtBuilder::build().execute_with(|| { let sub_id = H256::from_slice(&[1; 32]); - let new_amount = 20; + let new_credits = 20; let new_frequency = 4; assert_noop!( IdnManager::update_subscription( RuntimeOrigin::signed(ALICE), sub_id, - new_amount, + new_credits, new_frequency ), Error::::SubscriptionDoesNotExist @@ -481,7 +481,7 @@ fn update_subscription_fails_if_sub_does_not_exists() { #[test] fn test_pause_reactivate_subscription() { ExtBuilder::build().execute_with(|| { - let amount = 10; + let credits = 10; let frequency = 2; let target = Location::new(1, [Junction::PalletInstance(1)]); let metadata = None; @@ -490,7 +490,7 @@ fn test_pause_reactivate_subscription() { assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, metadata.clone() @@ -537,7 +537,7 @@ fn pause_subscription_fails_if_sub_does_not_exists() { #[test] fn pause_subscription_fails_if_sub_already_paused() { ExtBuilder::build().execute_with(|| { - let amount = 10; + let credits = 10; let frequency = 2; let target = Location::new(1, [Junction::PalletInstance(1)]); let metadata = None; @@ -546,7 +546,7 @@ fn pause_subscription_fails_if_sub_already_paused() { assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, metadata.clone() @@ -587,7 +587,7 @@ fn reactivate_subscription_fails_if_sub_does_not_exists() { #[test] fn reactivate_subscriptio_fails_if_sub_already_active() { ExtBuilder::build().execute_with(|| { - let amount = 10; + let credits = 10; let frequency = 2; let target = Location::new(1, [Junction::PalletInstance(1)]); let metadata = None; @@ -596,7 +596,7 @@ fn reactivate_subscriptio_fails_if_sub_already_active() { assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, metadata.clone() @@ -617,7 +617,7 @@ fn reactivate_subscriptio_fails_if_sub_already_active() { #[test] fn operations_fail_if_origin_is_not_the_subscriber() { ExtBuilder::build().execute_with(|| { - let amount: u64 = 50; + let credits: u64 = 50; let target = Location::new(1, [Junction::PalletInstance(1)]); let frequency: u64 = 10; let metadata = None; @@ -630,7 +630,7 @@ fn operations_fail_if_origin_is_not_the_subscriber() { // Create subscription for Alice assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, metadata.clone() @@ -661,13 +661,13 @@ fn operations_fail_if_origin_is_not_the_subscriber() { assert!(!event_emitted(Event::::SubscriptionPaused { sub_id })); // Attempt to update the subscription using Bob's origin (should fail) - let new_amount = amount + 10; + let new_credits = credits + 10; let new_frequency = frequency + 1; assert_noop!( IdnManager::update_subscription( RuntimeOrigin::signed(BOB.clone()), sub_id, - new_amount, + new_credits, new_frequency ), Error::::NotSubscriber @@ -687,7 +687,7 @@ fn operations_fail_if_origin_is_not_the_subscriber() { #[test] fn test_on_finalize_removes_finished_subscriptions() { ExtBuilder::build().execute_with(|| { - let amount: u64 = 50; + let credits: u64 = 50; let target = Location::new(1, [Junction::PalletInstance(1)]); let frequency: u64 = 10; let initial_balance = 10_000_000; @@ -696,7 +696,7 @@ fn test_on_finalize_removes_finished_subscriptions() { ::Currency::set_balance(&ALICE, initial_balance); assert_ok!(IdnManager::create_subscription( RuntimeOrigin::signed(ALICE.clone()), - amount, + credits, target.clone(), frequency, None @@ -705,7 +705,7 @@ fn test_on_finalize_removes_finished_subscriptions() { let (sub_id, mut subscription) = Subscriptions::::iter().next().unwrap(); // Manually set credits to zero to simulate a finished subscription - subscription.amount_left = Zero::zero(); + subscription.credits_left = Zero::zero(); Subscriptions::::insert(sub_id, subscription); // Before on_finalize, subscription should exist @@ -727,21 +727,21 @@ fn test_on_finalize_removes_finished_subscriptions() { fn hold_deposit_works() { ExtBuilder::build().execute_with(|| { let initial_balance = 10_000_000; - let deposit_amount = 1_000; + let deposit_credits = 1_000; // Setup account with initial balance ::Currency::set_balance(&ALICE, initial_balance); // Hold deposit - assert_ok!(crate::Pallet::::hold_deposit(&ALICE, deposit_amount)); + assert_ok!(crate::Pallet::::hold_deposit(&ALICE, deposit_credits)); // Verify deposit is held assert_eq!( Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), - deposit_amount + deposit_credits ); // Verify free balance is reduced - assert_eq!(Balances::free_balance(&ALICE), initial_balance - deposit_amount); + assert_eq!(Balances::free_balance(&ALICE), initial_balance - deposit_credits); }); } @@ -749,14 +749,14 @@ fn hold_deposit_works() { fn release_deposit_works() { ExtBuilder::build().execute_with(|| { let initial_balance = 10_000_000; - let deposit_amount = 1_000; + let deposit_credits = 1_000; // Setup account and hold deposit ::Currency::set_balance(&ALICE, initial_balance); - assert_ok!(crate::Pallet::::hold_deposit(&ALICE, deposit_amount)); + assert_ok!(crate::Pallet::::hold_deposit(&ALICE, deposit_credits)); // Release deposit - assert_ok!(crate::Pallet::::release_deposit(&ALICE, deposit_amount)); + assert_ok!(crate::Pallet::::release_deposit(&ALICE, deposit_credits)); // Verify deposit is released assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0); @@ -823,14 +823,14 @@ fn manage_diff_deposit_works() { fn hold_deposit_fails_with_insufficient_balance() { ExtBuilder::build().execute_with(|| { let initial_balance = 500; - let deposit_amount = 1_000; + let deposit_credits = 1_000; // Setup account with insufficient balance ::Currency::set_balance(&ALICE, initial_balance); // Attempt to hold deposit should fail assert_noop!( - crate::Pallet::::hold_deposit(&ALICE, deposit_amount), + crate::Pallet::::hold_deposit(&ALICE, deposit_credits), TokenError::FundsUnavailable ); From d8387f07e60beaa204825d5d57ad371cbc4fac49 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Mon, 3 Mar 2025 13:42:00 +0100 Subject: [PATCH 06/10] feat: rename all `amount` to `credits` --- pallets/idn-manager/src/impls.rs | 16 ++-- pallets/idn-manager/src/tests/fee_examples.rs | 80 +++++++++---------- pallets/idn-manager/src/traits.rs | 16 ++-- 3 files changed, 56 insertions(+), 56 deletions(-) diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index 4753d6b..368393a 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -71,26 +71,26 @@ where Balances::Balance: From, Balances::Reason: From, { - fn calculate_subscription_fees(amount: &BlockNumber) -> Balances::Balance { + fn calculate_subscription_fees(credits: &BlockNumber) -> Balances::Balance { let base_fee = B::get(); - base_fee.saturating_mul(amount.clone().into()) + base_fee.saturating_mul(credits.clone().into()) } fn calculate_diff_fees( - old_amount: &BlockNumber, - new_amount: &BlockNumber, + old_credits: &BlockNumber, + new_credits: &BlockNumber, ) -> DiffBalance { let mut direction = BalanceDirection::None; - let fees = match new_amount.cmp(&old_amount) { + let fees = match new_credits.cmp(&old_credits) { Ordering::Greater => { direction = BalanceDirection::Collect; Self::calculate_subscription_fees( - &new_amount.clone().saturating_sub(old_amount.clone()), + &new_credits.clone().saturating_sub(old_credits.clone()), ) }, Ordering::Less => { direction = BalanceDirection::Release; Self::calculate_subscription_fees( - &old_amount.clone().saturating_sub(new_amount.clone()), + &old_credits.clone().saturating_sub(new_credits.clone()), ) }, Ordering::Equal => Zero::zero(), @@ -113,7 +113,7 @@ where ) .map_err(FeesError::Other)?; - // Ensure the correct amount was collected. + // Ensure the correct credits was collected. // TODO: error to bubble up and be handled by caller https://github.com/ideal-lab5/idn-sdk/issues/107 if collected < *fees { return Err(FeesError::NotEnoughBalance { needed: *fees, balance: collected }); diff --git a/pallets/idn-manager/src/tests/fee_examples.rs b/pallets/idn-manager/src/tests/fee_examples.rs index 6850aac..a7c94b7 100644 --- a/pallets/idn-manager/src/tests/fee_examples.rs +++ b/pallets/idn-manager/src/tests/fee_examples.rs @@ -28,22 +28,22 @@ pub struct LinearFeeCalculator; const BASE_FEE: u32 = 100; impl FeesManager for LinearFeeCalculator { - fn calculate_subscription_fees(amount: &u32) -> u32 { - BASE_FEE.saturating_mul(amount.clone().into()) + fn calculate_subscription_fees(credits: &u32) -> u32 { + BASE_FEE.saturating_mul(credits.clone().into()) } - fn calculate_diff_fees(old_amount: &u32, new_amount: &u32) -> DiffBalance { + fn calculate_diff_fees(old_credits: &u32, new_credits: &u32) -> DiffBalance { let mut direction = BalanceDirection::None; - let fees = match new_amount.cmp(&old_amount) { + let fees = match new_credits.cmp(&old_credits) { Ordering::Greater => { direction = BalanceDirection::Collect; Self::calculate_subscription_fees( - &new_amount.clone().saturating_sub(old_amount.clone()), + &new_credits.clone().saturating_sub(old_credits.clone()), ) }, Ordering::Less => { direction = BalanceDirection::Release; Self::calculate_subscription_fees( - &old_amount.clone().saturating_sub(new_amount.clone()), + &old_credits.clone().saturating_sub(new_credits.clone()), ) }, Ordering::Equal => Zero::zero(), @@ -60,7 +60,7 @@ impl FeesManager for LinearFeeCalculator { pub struct SteppedTieredFeeCalculator; impl FeesManager for SteppedTieredFeeCalculator { - fn calculate_subscription_fees(amount: &u32) -> u32 { + fn calculate_subscription_fees(credits: &u32) -> u32 { // Define tier boundaries and their respective discount rates (in basis points) const TIERS: [(u32, u32); 5] = [ (1, 0), // 0-10: 0% discount @@ -71,18 +71,18 @@ impl FeesManager for SteppedTieredFeeCalculator { ]; let mut total_fee = 0u32; - let mut remaining_amount = *amount; + let mut remaining_credits = *credits; for (i, &(current_tier_start, current_tier_discount)) in TIERS.iter().enumerate() { - // If no remaining credits or the tier starts above the requested amount, exit loop. - if remaining_amount == 0 || amount < ¤t_tier_start { + // If no remaining credits or the tier starts above the requested credits, exit loop. + if remaining_credits == 0 || credits < ¤t_tier_start { break; } let next_tier_start = TIERS.get(i + 1).map(|&(start, _)| start).unwrap_or(u32::MAX); let credits_in_tier = - (amount.min(&next_tier_start.saturating_sub(1)) - current_tier_start + 1) - .min(remaining_amount); + (credits.min(&next_tier_start.saturating_sub(1)) - current_tier_start + 1) + .min(remaining_credits); let tier_fee = BASE_FEE .saturating_mul(credits_in_tier) @@ -90,15 +90,15 @@ impl FeesManager for SteppedTieredFeeCalculator { .saturating_div(10_000); total_fee = total_fee.saturating_add(tier_fee); - remaining_amount = remaining_amount.saturating_sub(credits_in_tier); + remaining_credits = remaining_credits.saturating_sub(credits_in_tier); } total_fee } - fn calculate_diff_fees(old_amount: &u32, new_amount: &u32) -> DiffBalance { - let old_fees = Self::calculate_subscription_fees(old_amount); - let new_fees = Self::calculate_subscription_fees(new_amount); + fn calculate_diff_fees(old_credits: &u32, new_credits: &u32) -> DiffBalance { + let old_fees = Self::calculate_subscription_fees(old_credits); + let new_fees = Self::calculate_subscription_fees(new_credits); let mut direction = BalanceDirection::None; let fees = match new_fees.cmp(&old_fees) { Ordering::Greater => { @@ -131,41 +131,41 @@ mod tests { } /// Thest when a subscription is fully used before being killed or the update does not change - /// the amount. + /// the credits. #[test] fn test_calculate_release_linear_fees_no_diff() { - let old_amount: u32 = 50; - let new_amount: u32 = 50; // no credits diff - let refund = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let old_credits: u32 = 50; + let new_credits: u32 = 50; // no credits diff + let refund = LinearFeeCalculator::calculate_diff_fees(&old_credits, &new_credits); // there should be no refund as all credit has been used, or no difference in the update let expected = 0; assert_eq!( refund, DiffBalance { balance: expected, direction: BalanceDirection::None }, - "There should be no release when no diff in amount" + "There should be no release when no diff in credits" ); } /// Test when a subscription is reduced or killed. #[test] fn test_calculate_release_linear_fees_lower_diff() { - let old_amount: u32 = 50; - let new_amount: u32 = 30; // 20 credits used - let refund = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let old_credits: u32 = 50; + let new_credits: u32 = 30; // 20 credits used + let refund = LinearFeeCalculator::calculate_diff_fees(&old_credits, &new_credits); let expected = 20 * BASE_FEE; assert_eq!( refund, DiffBalance { balance: expected, direction: BalanceDirection::Release }, - "There should be a release when new amount is lower than old amount" + "There should be a release when new credits is lower than old credits" ); } /// Test when the subscription is extended. #[test] fn test_calculate_hold_linear_fees_greater_diff() { - let old_amount: u32 = 50; - let new_amount: u32 = 60; // all credits used - let hold = LinearFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let old_credits: u32 = 50; + let new_credits: u32 = 60; // all credits used + let hold = LinearFeeCalculator::calculate_diff_fees(&old_credits, &new_credits); let expected = 10 * BASE_FEE; assert_eq!( hold, @@ -210,40 +210,40 @@ mod tests { } /// Thest when a subscription is fully used before being killed or the update does not change - /// the amount. + /// the credits. #[test] fn test_calculate_release_stepped_fees_no_diff() { - let old_amount: u32 = 50; - let new_amount: u32 = 50; // no credits diff - let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let old_credits: u32 = 50; + let new_credits: u32 = 50; // no credits diff + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_credits, &new_credits); let expected = 0; assert_eq!( refund, DiffBalance { balance: expected, direction: BalanceDirection::None }, - "There should be no release when no diff in amount" + "There should be no release when no diff in credits" ); } /// Test for a partial usage scenario. #[test] fn test_calculate_release_stepped_fees_lower_diff() { - let old_amount: u32 = 110; - let new_amount: u32 = 100; // 1 value decrease - let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let old_credits: u32 = 110; + let new_credits: u32 = 100; // 1 value decrease + let refund = SteppedTieredFeeCalculator::calculate_diff_fees(&old_credits, &new_credits); let expected = 10 * BASE_FEE.saturating_mul(9_000).saturating_div(10_000); // 10% discount on the extra value assert_eq!( refund, DiffBalance { balance: expected, direction: BalanceDirection::Release }, - "There should be a release when new amount is lower than old amount" + "There should be a release when new credits is lower than old credits" ); } /// Test when the subscription is fully used. #[test] fn test_calculate_hold_stepped_fees_greater_diff() { - let old_amount: u32 = 100; - let new_amount: u32 = 101; // 1 value increase - let hold = SteppedTieredFeeCalculator::calculate_diff_fees(&old_amount, &new_amount); + let old_credits: u32 = 100; + let new_credits: u32 = 101; // 1 value increase + let hold = SteppedTieredFeeCalculator::calculate_diff_fees(&old_credits, &new_credits); let expected = 1 * BASE_FEE.saturating_mul(9_000).saturating_div(10_000); // 10% discount on the extra value assert_eq!( hold, diff --git a/pallets/idn-manager/src/traits.rs b/pallets/idn-manager/src/traits.rs index face2bb..fd61ec3 100644 --- a/pallets/idn-manager/src/traits.rs +++ b/pallets/idn-manager/src/traits.rs @@ -44,16 +44,16 @@ pub struct DiffBalance { } /// Trait for fees managing -pub trait FeesManager, Err, S> { - /// Calculate the fees for a subscription based on the amount of random values required. - fn calculate_subscription_fees(amount: &Amount) -> Fees; +pub trait FeesManager, Err, S> { + /// Calculate the fees for a subscription based on the credits of random values required. + fn calculate_subscription_fees(credits: &Credits) -> Fees; /// Calculate how much fees should be held or release when a subscription changes. /// - /// * `old_amount` - the amount of random values required before the change. - /// * `new_amount` - the amount of random values required after the change, this will represent - /// the updated amount in an update operation. Or the amount actually consumed in a kill - /// operation. - fn calculate_diff_fees(old_amount: &Amount, new_amount: &Amount) -> DiffBalance; + /// * `old_credits` - the credits of random values required before the change. + /// * `new_credits` - the credits of random values required after the change, this will + /// represent the updated credits in an update operation. Or the credits actually consumed in + /// a kill operation. + fn calculate_diff_fees(old_credits: &Credits, new_credits: &Credits) -> DiffBalance; /// Distributes collected fees. Returns the fees that were effectively collected. fn collect_fees(fees: &Fees, sub: &Sub) -> Result>; } From 4b9ff8f963d55956b2db2142462717ab272d17af Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 4 Mar 2025 09:37:43 +0100 Subject: [PATCH 07/10] fix clippy warnings --- pallets/idn-manager/src/impls.rs | 4 +- pallets/idn-manager/src/lib.rs | 39 +++++++++-- pallets/idn-manager/src/tests/mock.rs | 2 +- pallets/idn-manager/src/tests/pallet.rs | 89 +++++++++++++++++++++++-- 4 files changed, 119 insertions(+), 15 deletions(-) diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index 368393a..eb4c9a1 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -80,7 +80,7 @@ where new_credits: &BlockNumber, ) -> DiffBalance { let mut direction = BalanceDirection::None; - let fees = match new_credits.cmp(&old_credits) { + let fees = match new_credits.cmp(old_credits) { Ordering::Greater => { direction = BalanceDirection::Collect; Self::calculate_subscription_fees( @@ -106,7 +106,7 @@ where &HoldReason::Fees.into(), sub.subscriber(), &T::get(), - fees.clone(), + *fees, Precision::BestEffort, Restriction::Free, Fortitude::Polite, diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index e07f723..378afc0 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -57,6 +57,7 @@ pub mod impls; pub mod traits; pub mod weights; +use crate::traits::FeesError; use codec::{Codec, Decode, Encode, MaxEncodedLen}; use frame_support::{ pallet_prelude::{ @@ -237,6 +238,8 @@ pub mod pallet { SubscriptionReactivated { sub_id: SubscriptionId }, /// Randomness was successfully distributed RandomnessDistributed { sub_id: SubscriptionId }, + /// Fees collected + FeesCollected { sub_id: SubscriptionId, fees: BalanceOf }, } #[pallet::error] @@ -357,7 +360,7 @@ pub mod pallet { let fees_diff = T::FeesManager::calculate_diff_fees(&sub.details.credits, &credits); let deposit_diff = T::DepositCalculator::calculate_diff_deposit( - &sub, + sub, &Subscription { state: sub.state.clone(), credits_left: credits, @@ -410,7 +413,7 @@ impl Pallet { ) -> DispatchResult { // fees left and deposit to refund let fees_diff = T::FeesManager::calculate_diff_fees(&sub.credits_left, &Zero::zero()); - let sd = T::DepositCalculator::calculate_storage_deposit(&sub); + let sd = T::DepositCalculator::calculate_storage_deposit(sub); Self::manage_diff_fees(&sub.details.subscriber, &fees_diff)?; Self::release_deposit(&sub.details.subscriber, sd)?; @@ -441,7 +444,10 @@ impl Pallet { Box::new(xcm::VersionedXcm::V5(msg.into())); let origin = frame_system::RawOrigin::Signed(Self::pallet_account_id()); if T::Xcm::send(origin.into(), versioned_target, versioned_msg).is_ok() { - Self::consume_credits(&sub_id, sub); + // We consume a fixed one credit per distribution + let credits_consumed = One::one(); + Self::collect_fees(&sub, credits_consumed)?; + Self::consume_credits(&sub_id, sub.clone(), credits_consumed); Self::deposit_event(Event::RandomnessDistributed { sub_id }); } } @@ -450,13 +456,34 @@ impl Pallet { Ok(()) } - fn consume_credits(sub_id: &SubscriptionId, mut sub: SubscriptionOf) { - // Decrease credits_left by one using saturating_sub - sub.credits_left = sub.credits_left.saturating_sub(One::one()); + fn consume_credits( + sub_id: &SubscriptionId, + mut sub: SubscriptionOf, + credits_consumed: BlockNumberFor, + ) { + // Decrease credits_left by `credits_consumed` using saturating_sub + sub.credits_left = sub.credits_left.saturating_sub(credits_consumed); // Update the subscription in storage Subscriptions::::insert(sub_id, sub) } + fn collect_fees( + sub: &SubscriptionOf, + credits_consumed: BlockNumberFor, + ) -> DispatchResult { + let fees_to_collect = T::FeesManager::calculate_diff_fees( + &sub.credits_left, + &sub.credits_left.saturating_sub(credits_consumed), + ) + .balance; + let fees = T::FeesManager::collect_fees(&fees_to_collect, sub).map_err(|e| match e { + FeesError::NotEnoughBalance { .. } => DispatchError::Other("NotEnoughBalance"), + FeesError::Other(de) => de, + })?; + Self::deposit_event(Event::FeesCollected { sub_id: sub.id(), fees }); + Ok(()) + } + /// Internal function to handle subscription creation fn create_subscription_internal( subscriber: T::AccountId, diff --git a/pallets/idn-manager/src/tests/mock.rs b/pallets/idn-manager/src/tests/mock.rs index a5027e6..dbac715 100644 --- a/pallets/idn-manager/src/tests/mock.rs +++ b/pallets/idn-manager/src/tests/mock.rs @@ -59,7 +59,7 @@ impl pallet_balances::Config for Test { parameter_types! { pub const MaxSubscriptionDuration: u64 = 100; pub const PalletId: frame_support::PalletId = frame_support::PalletId(*b"idn_mngr"); - pub const TreasuryAccount: AccountId32 = AccountId32::new([1u8; 32]); + pub const TreasuryAccount: AccountId32 = AccountId32::new([123u8; 32]); pub const BaseFee: u64 = 10; pub const SDMultiplier: u64 = 10; } diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index 63004cc..c757967 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -471,12 +471,89 @@ fn update_subscription_fails_if_sub_does_not_exists() { assert!(!event_emitted(Event::::SubscriptionUpdated { sub_id })); }); } -// todo: test credits consumption, it consumes credit by credit and verify that -// - fees are moved to treasury -// - credits are consumed -// - storage dep is refunded -// - subscription is removed -// https://github.com/ideal-lab5/idn-sdk/issues/108 + +#[test] +fn test_credits_consumption_and_cleanup() { + ExtBuilder::build().execute_with(|| { + // Setup initial conditions + let credits: u64 = 3; // Small number to make testing easier + let target = Location::new(1, [Junction::PalletInstance(1)]); + let frequency: u64 = 1; + let initial_balance = 10_000_000; + let mut treasury_balance = 0; + let rnd = [0u8; 32]; + + // Set up account + ::Currency::set_balance(&ALICE, initial_balance); + ::Currency::set_balance(&TreasuryAccount::get(), treasury_balance); + + // Create subscription + assert_ok!(IdnManager::create_subscription( + RuntimeOrigin::signed(ALICE.clone()), + credits, + target.clone(), + frequency, + None + )); + + // Get subscription details + let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); + let initial_fees = ::FeesManager::calculate_subscription_fees(&credits); + let initial_deposit = + ::DepositCalculator::calculate_storage_deposit(&subscription); + + // Verify initial state + assert_eq!( + Balances::free_balance(&ALICE), + initial_balance - initial_fees - initial_deposit + ); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), initial_fees); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), + initial_deposit + ); + assert_eq!(subscription.credits_left, credits); + + // Consume credits one by one + for i in 0..credits { + // Dispatch randomness + assert_ok!(IdnManager::dispatch(rnd.into())); + + // // Verify credit consumption + // let sub = Subscriptions::::get(sub_id).unwrap(); + // assert_eq!(sub.credits_left, credits - i - 1, "Credit not consumed correctly"); + + // // Verify fees movement to treasury + // treasury_balance += ::FeesManager::calculate_diff_fees(&(credits - + // i), &(credits - i - 1)).balance; assert_eq!( + // Balances::free_balance(&TreasuryAccount::get()), + // treasury_balance, + // "Fees not moved to treasury correctly" + // ); + } + + // // Verify subscription is removed after last credit + // assert!(!Subscriptions::::contains_key(sub_id)); + + // // Verify final balances + // assert_eq!(Balances::free_balance(&ALICE), initial_balance - initial_fees); + // assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0); + // assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0); + // assert_eq!(Balances::free_balance(&TreasuryAccount::get()), initial_fees); + + // // Verify events + // assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); + // // Should have 'credits' number of RandomnessDistributed events + // let randomness_events = System::events() + // .iter() + // .filter(|record| matches!( + // record.event, + // RuntimeEvent::IdnManager(Event::::RandomnessDistributed { sub_id: _ }) + // )) + // .count(); + // assert_eq!(randomness_events, credits as usize); + }); +} #[test] fn test_pause_reactivate_subscription() { From 4e3081b7312018aa9d3ad1eae4cf3ab2b7748c61 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 4 Mar 2025 13:01:14 +0100 Subject: [PATCH 08/10] fix clippy warnings --- pallets/idn-manager/src/impls.rs | 84 ++++++---- pallets/idn-manager/src/lib.rs | 14 +- pallets/idn-manager/src/tests/mock.rs | 4 +- pallets/idn-manager/src/tests/pallet.rs | 214 +++++++++++++++++------- 4 files changed, 210 insertions(+), 106 deletions(-) diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index eb4c9a1..05192a1 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -48,50 +48,74 @@ impl SubscriptionTrait<()> for () { } /// A FeesManager implementation that holds a dynamic treasury account. -pub struct FeesManagerImpl { - pub _phantom: FeesManagerPhantom, +pub struct FeesManagerImpl { + pub _phantom: FeesManagerPhantom, } -type FeesManagerPhantom = ( - PhantomData, - PhantomData, - PhantomData, - PhantomData, - PhantomData, -); +type FeesManagerPhantom = + (PhantomData, PhantomData, PhantomData, PhantomData); impl< T: Get, B: Get, S: SubscriptionTrait, - BlockNumber: Saturating + Ord + Clone, Balances: Mutate, - > pallet_idn_manager::FeesManager - for FeesManagerImpl + > pallet_idn_manager::FeesManager + for FeesManagerImpl where - Balances::Balance: From, Balances::Reason: From, + Balances::Balance: From, { - fn calculate_subscription_fees(credits: &BlockNumber) -> Balances::Balance { - let base_fee = B::get(); - base_fee.saturating_mul(credits.clone().into()) + fn calculate_subscription_fees(credits: &u64) -> Balances::Balance { + // Define tier boundaries and their respective discount rates (in basis points) + const TIERS: [(u64, u64); 5] = [ + (1, 0), // 0-10: 0% discount + (11, 500), // 11-100: 5% discount + (101, 1000), // 101-1000: 10% discount + (1001, 2000), // 1001-10000: 20% discount + (10001, 3000), // 10001+: 30% discount + ]; + + const BASE_FEE: u64 = 100; + + let mut total_fee = 0u64; + let mut remaining_credits = *credits; + + for (i, &(current_tier_start, current_tier_discount)) in TIERS.iter().enumerate() { + // If no remaining credits or the tier starts above the requested credits, exit loop. + if remaining_credits == 0 || credits < ¤t_tier_start { + break; + } + + let next_tier_start = TIERS.get(i + 1).map(|&(start, _)| start).unwrap_or(u64::MAX); + + let credits_in_tier = + (credits.min(&next_tier_start.saturating_sub(1)) - current_tier_start + 1) + .min(remaining_credits); + + let tier_fee = BASE_FEE + .saturating_mul(credits_in_tier) + .saturating_mul(10_000 - current_tier_discount) + .saturating_div(10_000); + + total_fee = total_fee.saturating_add(tier_fee); + remaining_credits = remaining_credits.saturating_sub(credits_in_tier); + } + + total_fee.into() } - fn calculate_diff_fees( - old_credits: &BlockNumber, - new_credits: &BlockNumber, - ) -> DiffBalance { + + fn calculate_diff_fees(old_credits: &u64, new_credits: &u64) -> DiffBalance { + let old_fees = Self::calculate_subscription_fees(old_credits); + let new_fees = Self::calculate_subscription_fees(new_credits); let mut direction = BalanceDirection::None; - let fees = match new_credits.cmp(old_credits) { + let fees = match new_fees.cmp(&old_fees) { Ordering::Greater => { direction = BalanceDirection::Collect; - Self::calculate_subscription_fees( - &new_credits.clone().saturating_sub(old_credits.clone()), - ) + new_fees - old_fees }, Ordering::Less => { direction = BalanceDirection::Release; - Self::calculate_subscription_fees( - &old_credits.clone().saturating_sub(new_credits.clone()), - ) + old_fees - new_fees }, Ordering::Equal => Zero::zero(), }; @@ -137,9 +161,9 @@ impl< fn calculate_storage_deposit(sub: &S) -> Deposit { // This function could theoretically saturate to the `Deposit` type bounds. Its result type // has an upper bound, which is Deposit::MAX, while unlikely and very expensive to - // the attacker, if Deposit type (e.g. u32) is bigger than usize machine architecture (e.g. - // 64 bits) there could be subscription object larger than u32::MAX bits, let’s say u32::MAX - // + d and only pay a deposit for u32::MAX and not d. Let's assess it with SRLabs. + // the attacker, if Deposit type (e.g. u64) is bigger than usize machine architecture (e.g. + // 64 bits) there could be subscription object larger than u64::MAX bits, let’s say u64::MAX + // + d and only pay a deposit for u64::MAX and not d. Let's assess it with SRLabs. let storage_deposit_multiplier = SDMultiplier::get(); let encoded_size = u64::try_from(sub.encoded_size()).unwrap_or(u64::MAX); storage_deposit_multiplier.saturating_mul(encoded_size.into()) diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index 378afc0..814b030 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -573,18 +573,8 @@ impl Pallet { /// Helper function to construct XCM message for randomness distribution // TODO: finish this off as part of https://github.com/ideal-lab5/idn-sdk/issues/77 - fn construct_randomness_xcm(target: Location, _rnd: &T::Rnd) -> Result, Error> { - Ok(Xcm(vec![ - WithdrawAsset((Here, 0u128).into()), - BuyExecution { fees: (Here, 0u128).into(), weight_limit: Unlimited }, - Transact { - origin_kind: OriginKind::Native, - fallback_max_weight: None, - call: Call::DistributeRnd.encode().into(), // TODO - }, - RefundSurplus, - DepositAsset { assets: All.into(), beneficiary: target }, - ])) + fn construct_randomness_xcm(_target: Location, _rnd: &T::Rnd) -> Result, Error> { + Ok(Xcm(vec![])) } } diff --git a/pallets/idn-manager/src/tests/mock.rs b/pallets/idn-manager/src/tests/mock.rs index dbac715..d2a221c 100644 --- a/pallets/idn-manager/src/tests/mock.rs +++ b/pallets/idn-manager/src/tests/mock.rs @@ -32,7 +32,6 @@ use scale_info::TypeInfo; use sp_runtime::{traits::IdentityLookup, AccountId32}; type Block = frame_system::mocking::MockBlock; -type BlockNumber = frame_system::pallet_prelude::BlockNumberFor; construct_runtime!( pub enum Test @@ -76,8 +75,7 @@ impl Get for SubMetadataLen { impl pallet_idn_manager::Config for Test { type RuntimeEvent = RuntimeEvent; type Currency = Balances; - type FeesManager = - FeesManagerImpl, BlockNumber, Balances>; + type FeesManager = FeesManagerImpl, Balances>; type DepositCalculator = DepositCalculatorImpl; type PalletId = PalletId; type RuntimeHoldReason = RuntimeHoldReason; diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index c757967..f7d7aaf 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -32,14 +32,14 @@ use frame_support::{ }; use idn_traits::rand::Dispatcher; use sp_core::H256; -use sp_runtime::{AccountId32, TokenError}; +use sp_runtime::{AccountId32, DispatchError, TokenError}; use xcm::v5::{Junction, Location}; const ALICE: AccountId32 = AccountId32::new([1u8; 32]); const BOB: AccountId32 = AccountId32::new([2u8; 32]); -fn event_emitted(event: Event) -> bool { - System::events().iter().any(|record| { +fn event_not_emitted(event: Event) -> bool { + !System::events().iter().any(|record| { if let RuntimeEvent::IdnManager(ref e) = &record.event { e == &event } else { @@ -133,16 +133,15 @@ fn update_subscription( ); assert_eq!(balance_after_update + new_fees + new_deposit, initial_balance); - // TODO: test probably in a separate test case fees handling but by adding block advance, and - // therefore credits consuption - let subscription = Subscriptions::::get(sub_id).unwrap(); // assert subscription details has been updated assert_eq!(subscription.details.credits, new_credits); assert_eq!(subscription.details.frequency, new_frequency); - assert!(event_emitted(Event::::SubscriptionUpdated { sub_id })); + System::assert_last_event(RuntimeEvent::IdnManager(Event::::SubscriptionUpdated { + sub_id, + })); } #[test] @@ -189,7 +188,9 @@ fn create_subscription_works() { ); // assert that the correct event has been emitted - assert!(event_emitted(Event::::SubscriptionCreated { sub_id })); + System::assert_last_event(RuntimeEvent::IdnManager(Event::::SubscriptionCreated { + sub_id, + })); }); } @@ -331,7 +332,9 @@ fn test_kill_subscription() { assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0u64); assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0u64); - assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); + System::assert_last_event(RuntimeEvent::IdnManager(Event::::SubscriptionRemoved { + sub_id, + })); }); } @@ -346,7 +349,7 @@ fn kill_subscription_fails_if_sub_does_not_exist() { ); // Assert the SubscriptionRemoved event was not emitted - assert!(!event_emitted(Event::::SubscriptionRemoved { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionRemoved { sub_id })); }); } @@ -394,12 +397,13 @@ fn on_finalize_removes_zero_credit_subscriptions() { // assert there are no remaining fees and balance refunded assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees); - // TODO: once fees collection in place uncomment this line - // assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0u64); + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0u64); // Verify event was emitted - assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); + System::assert_last_event(RuntimeEvent::IdnManager(Event::::SubscriptionRemoved { + sub_id, + })); }); } @@ -468,17 +472,18 @@ fn update_subscription_fails_if_sub_does_not_exists() { ); // Assert the SubscriptionUpdated event was not emitted - assert!(!event_emitted(Event::::SubscriptionUpdated { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionUpdated { sub_id })); }); } #[test] +/// This test makes sure that the correct fees are collected, by consuming credits one by one. fn test_credits_consumption_and_cleanup() { ExtBuilder::build().execute_with(|| { // Setup initial conditions - let credits: u64 = 3; // Small number to make testing easier + let credits: u64 = 1010; let target = Location::new(1, [Junction::PalletInstance(1)]); - let frequency: u64 = 1; + let frequency: u64 = 3; let initial_balance = 10_000_000; let mut treasury_balance = 0; let rnd = [0u8; 32]; @@ -499,59 +504,139 @@ fn test_credits_consumption_and_cleanup() { // Get subscription details let (sub_id, subscription) = Subscriptions::::iter().next().unwrap(); let initial_fees = ::FeesManager::calculate_subscription_fees(&credits); - let initial_deposit = + let storage_deposit = ::DepositCalculator::calculate_storage_deposit(&subscription); // Verify initial state assert_eq!( Balances::free_balance(&ALICE), - initial_balance - initial_fees - initial_deposit + initial_balance - initial_fees - storage_deposit ); assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), initial_fees); assert_eq!( Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), - initial_deposit + storage_deposit ); assert_eq!(subscription.credits_left, credits); // Consume credits one by one for i in 0..credits { + // Advance block and run hooks + System::set_block_number(System::block_number() + 1); + // Dispatch randomness assert_ok!(IdnManager::dispatch(rnd.into())); - // // Verify credit consumption - // let sub = Subscriptions::::get(sub_id).unwrap(); - // assert_eq!(sub.credits_left, credits - i - 1, "Credit not consumed correctly"); - - // // Verify fees movement to treasury - // treasury_balance += ::FeesManager::calculate_diff_fees(&(credits - - // i), &(credits - i - 1)).balance; assert_eq!( - // Balances::free_balance(&TreasuryAccount::get()), - // treasury_balance, - // "Fees not moved to treasury correctly" - // ); + System::assert_last_event(RuntimeEvent::IdnManager( + Event::::RandomnessDistributed { sub_id }, + )); + + // Verify credit consumption + let sub = Subscriptions::::get(sub_id).unwrap(); + assert_eq!(sub.credits_left, credits - i - 1, "Credit not consumed correctly"); + + // Verify fees movement to treasury + let fees = ::FeesManager::calculate_diff_fees( + &(credits - i), + &(credits - i - 1), + ) + .balance; + + treasury_balance += fees; + + assert_eq!( + Balances::free_balance(&TreasuryAccount::get()), + treasury_balance, + "Fees not moved to treasury correctly" + ); + + System::assert_has_event(RuntimeEvent::IdnManager(Event::::FeesCollected { + sub_id, + fees, + })); + + // assert balance has been collected from the hold + assert_eq!( + Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), + initial_fees - treasury_balance + ); + + // assert free balance is still correct + assert_eq!( + Balances::free_balance(&ALICE), + initial_balance - initial_fees - storage_deposit + ); + + // finalize block + IdnManager::on_finalize(System::block_number()); } - // // Verify subscription is removed after last credit - // assert!(!Subscriptions::::contains_key(sub_id)); - - // // Verify final balances - // assert_eq!(Balances::free_balance(&ALICE), initial_balance - initial_fees); - // assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0); - // assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0); - // assert_eq!(Balances::free_balance(&TreasuryAccount::get()), initial_fees); - - // // Verify events - // assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); - // // Should have 'credits' number of RandomnessDistributed events - // let randomness_events = System::events() - // .iter() - // .filter(|record| matches!( - // record.event, - // RuntimeEvent::IdnManager(Event::::RandomnessDistributed { sub_id: _ }) - // )) - // .count(); - // assert_eq!(randomness_events, credits as usize); + // Verify subscription is removed after last credit + assert!(!Subscriptions::::contains_key(sub_id)); + + // Verify final balances + assert_eq!(Balances::free_balance(&ALICE), initial_balance - initial_fees); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0); + assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), 0); + assert_eq!(Balances::free_balance(&TreasuryAccount::get()), initial_fees); + + // Verify events + System::assert_last_event(RuntimeEvent::IdnManager(Event::::SubscriptionRemoved { + sub_id, + })); + }); +} + +#[test] +fn test_credits_consumption_not_enogh_balance() { + ExtBuilder::build().execute_with(|| { + // Setup initial conditions + let credits: u64 = 1010; + let target = Location::new(1, [Junction::PalletInstance(1)]); + let frequency: u64 = 3; + let initial_balance = 10_000_000; + let rnd = [0u8; 32]; + + // Set up account + ::Currency::set_balance(&ALICE, initial_balance); + + // Create subscription + assert_ok!(IdnManager::create_subscription( + RuntimeOrigin::signed(ALICE.clone()), + credits, + target.clone(), + frequency, + None + )); + + // Get subscription details + let (_, sub) = Subscriptions::::iter().next().unwrap(); + + // Consume credits one by one + for i in 0..credits { + // Advance block and run hooks + System::set_block_number(System::block_number() + 1); + + if i == 505 { + // let's fake an incorrect fees collection at some arbitrary point + let _ = ::FeesManager::collect_fees( + &Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), + &sub, + ); + assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), 0); + assert_noop!( + IdnManager::dispatch(rnd.into()), + DispatchError::Other("NotEnoughBalance") + ); + break; + } else { + // Dispatch randomness + assert_ok!(IdnManager::dispatch(rnd.into())); + } + + // finalize block + IdnManager::on_finalize(System::block_number()); + } }); } @@ -579,6 +664,11 @@ fn test_pause_reactivate_subscription() { // Test pause and reactivate subscription assert_ok!(IdnManager::pause_subscription(RuntimeOrigin::signed(ALICE.clone()), sub_id)); + + System::assert_last_event(RuntimeEvent::IdnManager(Event::::SubscriptionPaused { + sub_id, + })); + assert_eq!(Subscriptions::::get(sub_id).unwrap().state, SubscriptionState::Paused); assert_ok!(IdnManager::reactivate_subscription( RuntimeOrigin::signed(ALICE.clone()), @@ -590,9 +680,9 @@ fn test_pause_reactivate_subscription() { // reactivating assert_eq!(Balances::free_balance(&ALICE), free_balance); - assert!(event_emitted(Event::::SubscriptionPaused { sub_id })); - - assert!(event_emitted(Event::::SubscriptionReactivated { sub_id })); + System::assert_last_event(RuntimeEvent::IdnManager( + Event::::SubscriptionReactivated { sub_id }, + )); }); } @@ -607,7 +697,7 @@ fn pause_subscription_fails_if_sub_does_not_exists() { ); // Assert the SubscriptionPaused event was not emitted - assert!(!event_emitted(Event::::SubscriptionPaused { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionPaused { sub_id })); }); } @@ -642,7 +732,7 @@ fn pause_subscription_fails_if_sub_already_paused() { ); // Assert the SubscriptionPaused event was not emitted - assert!(!event_emitted(Event::::SubscriptionPaused { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionPaused { sub_id })); }); } @@ -657,7 +747,7 @@ fn reactivate_subscription_fails_if_sub_does_not_exists() { ); // Assert the SubscriptionReactivated event was not emitted - assert!(!event_emitted(Event::::SubscriptionReactivated { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionReactivated { sub_id })); }); } @@ -687,7 +777,7 @@ fn reactivate_subscriptio_fails_if_sub_already_active() { ); // Assert the SubscriptionReactivated event was not emitted - assert!(!event_emitted(Event::::SubscriptionReactivated { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionReactivated { sub_id })); }); } @@ -726,7 +816,7 @@ fn operations_fail_if_origin_is_not_the_subscriber() { assert!(Subscriptions::::get(sub_id).is_some()); // Assert the SubscriptionRemoved event was not emitted - assert!(!event_emitted(Event::::SubscriptionRemoved { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionRemoved { sub_id })); // Attempt to pause the subscription using Bob's origin (should fail) assert_noop!( @@ -735,7 +825,7 @@ fn operations_fail_if_origin_is_not_the_subscriber() { ); // Assert the SubscriptionPaused event was not emitted - assert!(!event_emitted(Event::::SubscriptionPaused { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionPaused { sub_id })); // Attempt to update the subscription using Bob's origin (should fail) let new_credits = credits + 10; @@ -757,7 +847,7 @@ fn operations_fail_if_origin_is_not_the_subscriber() { ); // Assert the SubscriptionReactivated event was not emitted - assert!(!event_emitted(Event::::SubscriptionReactivated { sub_id })); + assert!(event_not_emitted(Event::::SubscriptionReactivated { sub_id })); }); } @@ -796,7 +886,9 @@ fn test_on_finalize_removes_finished_subscriptions() { assert!(!Subscriptions::::contains_key(sub_id)); // 2. SubscriptionRemoved event should be emitted - assert!(event_emitted(Event::::SubscriptionRemoved { sub_id })); + System::assert_last_event(RuntimeEvent::IdnManager(Event::::SubscriptionRemoved { + sub_id, + })); }); } From 43533b71d48707c55794194b398717ad27a25248 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 4 Mar 2025 13:14:21 +0100 Subject: [PATCH 09/10] remove commented out code --- pallets/idn-manager/src/lib.rs | 2 -- pallets/idn-manager/src/tests/pallet.rs | 4 ---- 2 files changed, 6 deletions(-) diff --git a/pallets/idn-manager/src/lib.rs b/pallets/idn-manager/src/lib.rs index 814b030..f95266a 100644 --- a/pallets/idn-manager/src/lib.rs +++ b/pallets/idn-manager/src/lib.rs @@ -254,8 +254,6 @@ pub mod pallet { SubscriptionAlreadyPaused, /// The origin isn't the subscriber NotSubscriber, - // /// Insufficient balance for subscription - // InsufficientBalance, } /// A reason for the IDN Manager Pallet placing a hold on funds. diff --git a/pallets/idn-manager/src/tests/pallet.rs b/pallets/idn-manager/src/tests/pallet.rs index f7d7aaf..ab9bbb4 100644 --- a/pallets/idn-manager/src/tests/pallet.rs +++ b/pallets/idn-manager/src/tests/pallet.rs @@ -320,10 +320,6 @@ fn test_kill_subscription() { assert_eq!(Balances::free_balance(&ALICE), initial_balance - fees - deposit); assert_eq!(Balances::balance_on_hold(&HoldReason::Fees.into(), &ALICE), fees); assert_eq!(Balances::balance_on_hold(&HoldReason::StorageDeposit.into(), &ALICE), deposit); - // TOOD assert: - // - correct fees were collected. Test probably in a separate test case fees handling but by - // adding block advance, - // and therefore credits consuption assert_ok!(IdnManager::kill_subscription(RuntimeOrigin::signed(ALICE), sub_id)); assert!(Subscriptions::::get(sub_id).is_none()); From e666575ef901f7bbc28cdcfe27d45611c6ab3f56 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Fri, 7 Mar 2025 13:20:57 +0100 Subject: [PATCH 10/10] improve docs --- pallets/idn-manager/src/impls.rs | 200 +++++++++++++++++- pallets/idn-manager/src/tests/fee_examples.rs | 12 +- 2 files changed, 203 insertions(+), 9 deletions(-) diff --git a/pallets/idn-manager/src/impls.rs b/pallets/idn-manager/src/impls.rs index 05192a1..9961ad1 100644 --- a/pallets/idn-manager/src/impls.rs +++ b/pallets/idn-manager/src/impls.rs @@ -65,6 +65,36 @@ where Balances::Reason: From, Balances::Balance: From, { + /// Calculate the subscription fees based on the number of requested credits. + /// + /// This function implements a tiered pricing model with volume discounts: + /// - Tier 1 (1-10 credits): 100% of base fee per credit (no discount) + /// - Tier 2 (11-100 credits): 95% of base fee per credit (5% discount) + /// - Tier 3 (101-1000 credits): 90% of base fee per credit (10% discount) + /// - Tier 4 (1001-10000 credits): 80% of base fee per credit (20% discount) + /// - Tier 5 (10001+ credits): 70% of base fee per credit (30% discount) + /// + /// The fee calculation processes each tier sequentially: + /// 1. For each tier, calculate how many credits fall within that tier + /// 2. Apply the corresponding discount rate to those credits + /// 3. Sum up the fees across all tiers + /// + /// # Parameters + /// * `credits` - The total number of credits requested for the subscription + /// + /// # Returns + /// The total fee required for the requested number of credits, converted to the + /// appropriate balance type + /// + /// # Example + /// ```no_compile + /// // 100 credits would incur a fee of: + /// // - 10 credits at full price: 10 * 100 = 1000 + /// // - 90 credits at 5% discount: 90 * 95 = 8550 + /// // Total: 9550 + /// let fees = calculate_subscription_fees(&100u64); + /// assert_eq!(fees, 9550u64.into()); + /// ``` fn calculate_subscription_fees(credits: &u64) -> Balances::Balance { // Define tier boundaries and their respective discount rates (in basis points) const TIERS: [(u64, u64); 5] = [ @@ -81,8 +111,8 @@ where let mut remaining_credits = *credits; for (i, &(current_tier_start, current_tier_discount)) in TIERS.iter().enumerate() { - // If no remaining credits or the tier starts above the requested credits, exit loop. - if remaining_credits == 0 || credits < ¤t_tier_start { + // If no remaining credits exit loop. + if remaining_credits == 0 { break; } @@ -104,6 +134,50 @@ where total_fee.into() } + /// Calculate the difference in fees when a subscription changes, determining whether + /// additional fees should be collected or excess fees should be released. This is useful for + /// subscription updates and kills for holding or releasing fees. Or when collecting fees from a + /// subscriber. + /// + /// This function compares the fees required before and after a subscription change, + /// then returns: + /// - The fee difference amount + /// - The direction of the balance transfer (collect from user, release to user, or no change) + /// + /// # Parameters + /// * `old_credits` + /// - For an update operation, this represents the original requested credits + /// - For a kill and collect operation, this is the number of credits left in the subscription + /// * `new_credits` + /// - For an update operation, this represents the new requested credits + /// - For a kill operation, this is 0 + /// - For a collect operation, this is the actual number of credits consumed + /// + /// # Returns + /// A `DiffBalance` struct containing: + /// - `balance`: The amount of fees to collect or release + /// - `direction`: Whether to collect additional fees, release excess fees, or do nothing + /// + /// # Examples + /// ```no_compile + /// // When increasing credits, additional fees are collected: + /// // Old: 10 credits (1000 fee), New: 50 credits (5000 fee) + /// let diff = calculate_diff_fees(&10, &50); + /// assert_eq!(diff.balance, 4000); + /// assert_eq!(diff.direction, BalanceDirection::Collect); + /// + /// // When decreasing credits, excess fees are released: + /// // Old: 100 credits (9550 fee), New: 10 credits (1000 fee) + /// let diff = calculate_diff_fees(&100, &10); + /// assert_eq!(diff.balance, 8550); + /// assert_eq!(diff.direction, BalanceDirection::Release); + /// + /// // When credits remain the same, no fee changes occur: + /// // Old: 50 credits (5000 fee), New: 50 credits (5000 fee) + /// let diff = calculate_diff_fees(&50, &50); + /// assert_eq!(diff.balance, 0); + /// assert_eq!(diff.direction, BalanceDirection::None); + /// ``` fn calculate_diff_fees(old_credits: &u64, new_credits: &u64) -> DiffBalance { let old_fees = Self::calculate_subscription_fees(old_credits); let new_fees = Self::calculate_subscription_fees(new_credits); @@ -121,6 +195,47 @@ where }; DiffBalance { balance: fees, direction } } + + /// Attempts to collect subscription fees from a subscriber and transfer them to the treasury + /// account. + /// + /// This function: + /// 1. Transfers the specified fees from the subscriber's held balance to the treasury account + /// 2. Verifies that the full fee amount was successfully collected + /// 3. Returns the actual amount collected or an appropriate error + /// + /// # Parameters + /// * `fees` - The amount of fees to collect + /// * `sub` - The subscription object containing the subscriber account information + /// + /// # Returns + /// - `Ok(collected)` - The amount of fees successfully collected and transferred + /// - `Err(FeesError)` - If the transfer operation fails + /// + /// # Notes + /// - This function uses `transfer_on_hold` which transfers from the subscriber's held balance + /// - The fees are held under the `HoldReason::Fees` reason code + /// - The transfer uses `Precision::BestEffort` which allows partial transfers if full amount + /// isn't available + /// - Despite using best effort, this function will return an error if less than the requested + /// amount is collected + /// + /// # Example + /// ```no_compile + /// let fees = 1000u64.into(); + /// let result = FeesManagerImpl::::collect_fees( + /// &fees, + /// &subscription + /// ); + /// + /// match result { + /// Ok(collected) => println!("Successfully collected {} in fees", collected), + /// Err(FeesError::NotEnoughBalance { needed, balance }) => { + /// println!("Insufficient balance: needed {}, had {}", needed, balance); + /// }, + /// Err(FeesError::Other(err)) => println!("Transfer error: {:?}", err), + /// } + /// ``` fn collect_fees( fees: &Balances::Balance, sub: &S, @@ -158,17 +273,88 @@ impl< > pallet_idn_manager::DepositCalculator for DepositCalculatorImpl { + /// Calculate the storage deposit required for a subscription. + /// + /// This function computes the storage deposit amount based on the encoded size of the + /// subscription object multiplied by a configurable deposit multiplier. + /// + /// # Parameters + /// * `sub` - The subscription object for which to calculate the storage deposit + /// + /// # Returns + /// The amount of deposit required for the subscription's storage + /// + /// # Security Considerations + /// This function handles potential overflow scenarios by: + /// 1. Converting the encoded size from `usize` to `u64` with fallback to `u64::MAX` if the + /// conversion fails + /// 2. Using saturating multiplication to prevent arithmetic overflow + /// + /// # Example + /// ```no_compile + /// let subscription = Subscription { + /// // subscription details... + /// }; + /// + /// // If SDMultiplier is 2 and the subscription encodes to 100 bytes: + /// let deposit = DepositCalculatorImpl::::calculate_storage_deposit(&subscription); + /// assert_eq!(deposit, 200); + /// ``` fn calculate_storage_deposit(sub: &S) -> Deposit { - // This function could theoretically saturate to the `Deposit` type bounds. Its result type - // has an upper bound, which is Deposit::MAX, while unlikely and very expensive to - // the attacker, if Deposit type (e.g. u64) is bigger than usize machine architecture (e.g. - // 64 bits) there could be subscription object larger than u64::MAX bits, let’s say u64::MAX - // + d and only pay a deposit for u64::MAX and not d. Let's assess it with SRLabs. + // [SRLabs] Note: There is a theoretical edge case where if the `Deposit` type (e.g., u64) + // is larger than the machine architecture size (e.g., 32-bit platforms), an attacker + // could create a subscription object larger than u32::MAX bits and only pay a deposit for + // u32::MAX bits, not the full size. This risk is mitigated in practice by platform + // constraints and cost barriers. let storage_deposit_multiplier = SDMultiplier::get(); let encoded_size = u64::try_from(sub.encoded_size()).unwrap_or(u64::MAX); storage_deposit_multiplier.saturating_mul(encoded_size.into()) } + /// Calculate the difference in storage deposit between two subscriptions. + /// + /// This function compares the storage deposit requirements of two subscription states + /// and returns the difference along with the direction of the balance adjustment. + /// + /// # Parameters + /// * `old_sub` - The original subscription before changes + /// * `new_sub` - The updated subscription after changes + /// + /// # Returns + /// A `DiffBalance` struct containing: + /// - `balance`: The absolute difference between the old and new deposit amounts + /// - `direction`: Whether to collect additional deposit, release excess deposit, or make no + /// change + /// + /// # How It Works + /// 1. Calculates the storage deposit for both the old and new subscription states + /// 2. Compares the two deposits to determine if more deposit is needed, some can be released, + /// or no change is required + /// 3. Returns both the amount and direction of the required deposit adjustment + /// + /// # Example + /// ```no_compile + /// // When subscription size increases (e.g., metadata added): + /// let old_sub = /* subscription with 100 bytes encoded size */; + /// let new_sub = /* same subscription with 150 bytes encoded size */; + /// + /// // If SDMultiplier is 2: + /// // Old deposit = 2 * 100 = 200 + /// // New deposit = 2 * 150 = 300 + /// let diff = DepositCalculatorImpl::::calculate_diff_deposit(&old_sub, &new_sub); + /// assert_eq!(diff.balance, 100); // 300 - 200 = 100 more needed + /// assert_eq!(diff.direction, BalanceDirection::Collect); + /// + /// // When subscription size decreases (e.g., metadata removed): + /// let old_sub = /* subscription with 150 bytes encoded size */; + /// let new_sub = /* same subscription with 100 bytes encoded size */; + /// + /// // Old deposit = 2 * 150 = 300 + /// // New deposit = 2 * 100 = 200 + /// let diff = DepositCalculatorImpl::::calculate_diff_deposit(&old_sub, &new_sub); + /// assert_eq!(diff.balance, 100); // 300 - 200 = 100 to release + /// assert_eq!(diff.direction, BalanceDirection::Release); + /// ``` fn calculate_diff_deposit(old_sub: &S, new_sub: &S) -> DiffBalance { let old_deposit = Self::calculate_storage_deposit(old_sub); let new_deposit = Self::calculate_storage_deposit(new_sub); diff --git a/pallets/idn-manager/src/tests/fee_examples.rs b/pallets/idn-manager/src/tests/fee_examples.rs index a7c94b7..d942235 100644 --- a/pallets/idn-manager/src/tests/fee_examples.rs +++ b/pallets/idn-manager/src/tests/fee_examples.rs @@ -74,8 +74,8 @@ impl FeesManager for SteppedTieredFeeCalculator { let mut remaining_credits = *credits; for (i, &(current_tier_start, current_tier_discount)) in TIERS.iter().enumerate() { - // If no remaining credits or the tier starts above the requested credits, exit loop. - if remaining_credits == 0 || credits < ¤t_tier_start { + // If no remaining credits exit loop. + if remaining_credits == 0 { break; } @@ -187,6 +187,14 @@ mod tests { let fee_50 = SteppedTieredFeeCalculator::calculate_subscription_fees(&50); assert_eq!(fee_50, 4_800); // (10 * 100) + (40 * 95) = 1,000 + 3,800 = 4,800 + // Test edge of second tier + let fee_100 = SteppedTieredFeeCalculator::calculate_subscription_fees(&100); + assert_eq!(fee_100, 9550); // (10 * 100) + (90 * 95) = 1,000 + 8,550 = 9,550 + + // Test edge of second tier + let fee_101 = SteppedTieredFeeCalculator::calculate_subscription_fees(&101); + assert_eq!(fee_101, 9_640); // (10 * 100) + (90 * 95) + (1 * 90)= 1,000 + 8,550 + 90 = 9,640 + // Test crossing multiple tiers let fee_150 = SteppedTieredFeeCalculator::calculate_subscription_fees(&150); // First 10 at 100% = 1,000