diff --git a/pallets/ddc-customers/src/lib.rs b/pallets/ddc-customers/src/lib.rs index 98849d807..cdd8decb0 100644 --- a/pallets/ddc-customers/src/lib.rs +++ b/pallets/ddc-customers/src/lib.rs @@ -9,10 +9,7 @@ mod tests; use codec::{Decode, Encode, HasCompact}; use ddc_primitives::{BucketId, ClusterId}; -use ddc_traits::{ - cluster::ClusterVisitor, - customer::{CustomerCharger, CustomerChargerError}, -}; +use ddc_traits::{cluster::ClusterVisitor, customer::CustomerCharger}; use frame_support::{ parameter_types, traits::{Currency, DefensiveSaturating, ExistenceRequirement}, @@ -21,7 +18,7 @@ use frame_support::{ use scale_info::TypeInfo; use sp_runtime::{ traits::{AccountIdConversion, AtLeast32BitUnsigned, CheckedAdd, CheckedSub, Saturating, Zero}, - RuntimeDebug, SaturatedConversion, + DispatchError, RuntimeDebug, SaturatedConversion, }; use sp_std::prelude::*; @@ -129,8 +126,6 @@ impl< .ok_or(Error::::ArithmeticOverflow)?; if temp <= value { unlocking_balance = temp; - self.active = - self.active.checked_sub(&last.value).ok_or(Error::::ArithmeticUnderflow)?; self.unlocking.pop(); } else { let diff = @@ -138,8 +133,6 @@ impl< unlocking_balance = unlocking_balance.checked_add(&diff).ok_or(Error::::ArithmeticOverflow)?; - self.active = - self.active.checked_sub(&diff).ok_or(Error::::ArithmeticUnderflow)?; last.value = last.value.checked_sub(&diff).ok_or(Error::::ArithmeticUnderflow)?; } @@ -429,7 +422,7 @@ pub mod pallet { .map_err(|_| Error::::NoMoreChunks)?; }; - Self::update_ledger(&owner, &ledger); + >::insert(&owner, &ledger); Self::deposit_event(Event::::InitialDepositUnlock(ledger.owner, value)); } @@ -466,7 +459,7 @@ pub mod pallet { log::debug!("Updating ledger"); // This was the consequence of a partial deposit unlock. just update the ledger and // move on. - Self::update_ledger(&owner, &ledger); + >::insert(&owner, &ledger); }; log::debug!("Current total: {:?}", ledger.total); @@ -506,11 +499,9 @@ pub mod pallet { owner: &T::AccountId, ledger: &AccountsLedger, T>, ) -> DispatchResult { - let account_id = Self::account_id(); - ::Currency::transfer( owner, - &account_id, + &Self::account_id(), ledger.total, ExistenceRequirement::KeepAlive, )?; @@ -519,14 +510,6 @@ pub mod pallet { Ok(()) } - /// Update the ledger for a owner. - fn update_ledger( - owner: &T::AccountId, - ledger: &AccountsLedger, T>, - ) { - >::insert(owner, ledger); - } - /// Remove all associated data of a owner account from the accounts system. /// /// Assumes storage is upgraded before calling. @@ -547,47 +530,48 @@ pub mod pallet { content_owner: T::AccountId, billing_vault: T::AccountId, amount: u128, - ) -> Result { + ) -> Result { let actually_charged: BalanceOf; - let mut ledger = Self::ledger(&content_owner).ok_or(CustomerChargerError::NotOwner)?; - let mut amount_to_deduct = amount.saturated_into::>(); + let mut ledger = Self::ledger(&content_owner).ok_or(Error::::NotOwner)?; + let amount_to_deduct = amount.saturated_into::>(); - ensure!(ledger.total >= ledger.active, CustomerChargerError::ArithmeticUnderflow); if ledger.active >= amount_to_deduct { actually_charged = amount_to_deduct; ledger.active = ledger .active .checked_sub(&amount_to_deduct) - .ok_or(CustomerChargerError::ArithmeticUnderflow)?; + .ok_or(Error::::ArithmeticUnderflow)?; ledger.total = ledger .total .checked_sub(&amount_to_deduct) - .ok_or(CustomerChargerError::ArithmeticUnderflow)?; - Self::update_ledger(&content_owner, &ledger); + .ok_or(Error::::ArithmeticUnderflow)?; + + >::insert(&content_owner, &ledger); } else { let diff = amount_to_deduct .checked_sub(&ledger.active) - .ok_or(CustomerChargerError::ArithmeticUnderflow)?; - actually_charged = diff; + .ok_or(Error::::ArithmeticUnderflow)?; + + actually_charged = ledger.active; ledger.total = ledger .total .checked_sub(&ledger.active) - .ok_or(CustomerChargerError::ArithmeticUnderflow)?; - amount_to_deduct = ledger.active; + .ok_or(Error::::ArithmeticUnderflow)?; ledger.active = BalanceOf::::zero(); - let (ledger, _charged) = ledger - .charge_unlocking(diff) - .map_err(|_| CustomerChargerError::UnlockFailed)?; - Self::update_ledger(&content_owner, &ledger); - }; + + let (ledger, charged) = ledger.charge_unlocking(diff)?; + + actually_charged.checked_add(&charged).ok_or(Error::::ArithmeticUnderflow)?; + + >::insert(&content_owner, &ledger); + } ::Currency::transfer( &Self::account_id(), &billing_vault, - amount_to_deduct, - ExistenceRequirement::KeepAlive, - ) - .map_err(|_| CustomerChargerError::TransferFailed)?; + actually_charged, + ExistenceRequirement::AllowDeath, + )?; Self::deposit_event(Event::::Charged(content_owner, amount_to_deduct)); Ok(actually_charged.saturated_into::()) diff --git a/pallets/ddc-customers/src/mock.rs b/pallets/ddc-customers/src/mock.rs index 0004eb3a5..ae482c7ee 100644 --- a/pallets/ddc-customers/src/mock.rs +++ b/pallets/ddc-customers/src/mock.rs @@ -163,8 +163,10 @@ impl ExtBuilder { sp_tracing::try_init_simple(); let mut storage = frame_system::GenesisConfig::default().build_storage::().unwrap(); - let _ = pallet_balances::GenesisConfig:: { balances: vec![(1, 100), (2, 100)] } - .assimilate_storage(&mut storage); + let _ = pallet_balances::GenesisConfig:: { + balances: vec![(1, 100), (2, 100), (3, 1000)], + } + .assimilate_storage(&mut storage); TestExternalities::new(storage) } diff --git a/pallets/ddc-customers/src/tests.rs b/pallets/ddc-customers/src/tests.rs index 869115e3b..8e2ebd945 100644 --- a/pallets/ddc-customers/src/tests.rs +++ b/pallets/ddc-customers/src/tests.rs @@ -79,26 +79,27 @@ fn deposit_and_deposit_extra_works() { BalancesError::::KeepAlive ); + let amount1 = 10_u128; // Deposited - assert_ok!(DdcCustomers::deposit(RuntimeOrigin::signed(account_1), 10_u128)); + assert_ok!(DdcCustomers::deposit(RuntimeOrigin::signed(account_1), amount1)); // Check storage assert_eq!( DdcCustomers::ledger(&account_1), Some(AccountsLedger { - owner: 1, - total: 10_u128, - active: 10_u128, + owner: account_1, + total: amount1, + active: amount1, unlocking: Default::default(), }) ); // Checking that event was emitted - System::assert_last_event(Event::Deposited(account_1, 10).into()); + System::assert_last_event(Event::Deposited(account_1, amount1).into()); // Deposit should fail when called the second time assert_noop!( - DdcCustomers::deposit(RuntimeOrigin::signed(account_1), 10_u128), + DdcCustomers::deposit(RuntimeOrigin::signed(account_1), amount1), Error::::AlreadyPaired ); @@ -109,21 +110,110 @@ fn deposit_and_deposit_extra_works() { ); // Deposited extra - assert_ok!(DdcCustomers::deposit_extra(RuntimeOrigin::signed(account_1), 20_u128)); + let amount2 = 20_u128; + assert_ok!(DdcCustomers::deposit_extra(RuntimeOrigin::signed(account_1), amount2)); // Check storage assert_eq!( DdcCustomers::ledger(&account_1), Some(AccountsLedger { - owner: 1, - total: 30_u128, - active: 30_u128, + owner: account_1, + total: amount1 + amount2, + active: amount1 + amount2, unlocking: Default::default(), }) ); // Checking that event was emitted - System::assert_last_event(Event::Deposited(account_1, 20).into()); + System::assert_last_event(Event::Deposited(account_1, amount2).into()); + }) +} + +#[test] +fn charge_content_owner_works() { + ExtBuilder.build_and_execute(|| { + System::set_block_number(1); + + let account_3 = 3; + let vault = 4; + let deposit = 100_u128; + + let balance_before_deposit = Balances::free_balance(account_3); + // Deposited + assert_ok!(DdcCustomers::deposit(RuntimeOrigin::signed(account_3), deposit)); + let balance_after_deposit = Balances::free_balance(account_3); + assert_eq!(balance_before_deposit - deposit, balance_after_deposit); + + let pallet_balance = Balances::free_balance(DdcCustomers::account_id()); + assert_eq!(deposit, pallet_balance); + + // Check storage + assert_eq!( + DdcCustomers::ledger(&account_3), + Some(AccountsLedger { + owner: account_3, + total: deposit, + active: deposit, + unlocking: Default::default(), + }) + ); + + // Checking that event was emitted + System::assert_last_event(Event::Deposited(account_3, deposit).into()); + + // successful transfer + let charge1 = 10; + let charged = DdcCustomers::charge_content_owner(account_3, vault, charge1).unwrap(); + assert_eq!(charge1, charged); + + let vault_balance = Balances::free_balance(vault); + assert_eq!(charged, vault_balance); + + let account_balance = Balances::free_balance(account_3); + assert_eq!(balance_after_deposit, account_balance); + + let pallet_balance_after_charge = Balances::free_balance(DdcCustomers::account_id()); + assert_eq!(pallet_balance - charged, pallet_balance_after_charge); + + // Check storage + assert_eq!( + DdcCustomers::ledger(&account_3), + Some(AccountsLedger { + owner: account_3, + total: deposit - charge1, + active: deposit - charge1, + unlocking: Default::default(), + }) + ); + + // failed transfer + let charge2 = 100u128; + let charge_result = DdcCustomers::charge_content_owner(account_3, vault, charge2).unwrap(); + assert_eq!( + DdcCustomers::ledger(&account_3), + Some(AccountsLedger { + owner: account_3, + total: 0, + active: 0, + unlocking: Default::default(), + }) + ); + + assert_eq!(0, Balances::free_balance(DdcCustomers::account_id())); + assert_eq!(charge_result, deposit - charge1); + + assert_ok!(DdcCustomers::deposit_extra(RuntimeOrigin::signed(account_3), deposit)); + assert_eq!( + DdcCustomers::ledger(&account_3), + Some(AccountsLedger { + owner: account_3, + total: deposit, + active: deposit, + unlocking: Default::default(), + }) + ); + + assert_eq!(deposit, Balances::free_balance(DdcCustomers::account_id())); }) } @@ -188,7 +278,7 @@ fn unlock_and_withdraw_deposit_works() { }) ); - // Unlock remaining chuncks & withdraw + // Unlock remaining chunks & withdraw assert_ok!(DdcCustomers::unlock_deposit(RuntimeOrigin::signed(account_1), 3_u128)); System::set_block_number(52); assert_ok!(DdcCustomers::withdraw_unlocked_deposit(RuntimeOrigin::signed(account_1))); diff --git a/pallets/ddc-payouts/src/lib.rs b/pallets/ddc-payouts/src/lib.rs index cb4c0b841..036c521c8 100644 --- a/pallets/ddc-payouts/src/lib.rs +++ b/pallets/ddc-payouts/src/lib.rs @@ -140,6 +140,13 @@ pub mod pallet { customer_id: T::AccountId, amount: u128, }, + Indebted { + cluster_id: ClusterId, + era: DdcEra, + batch_index: BatchIndex, + customer_id: T::AccountId, + amount: u128, + }, ChargingFinished { cluster_id: ClusterId, era: DdcEra, @@ -386,15 +393,23 @@ pub mod pallet { DebtorCustomers::::try_get(cluster_id, customer_id.clone()) .unwrap_or_else(|_| Zero::zero()); - customer_debt = (|| -> Option { - customer_debt - .checked_add(total_customer_charge)? - .checked_sub(amount_actually_charged) - })() - .ok_or(Error::::ArithmeticOverflow)?; + let debt = total_customer_charge + .checked_sub(amount_actually_charged) + .ok_or(Error::::ArithmeticOverflow)?; + + customer_debt = + customer_debt.checked_add(debt).ok_or(Error::::ArithmeticOverflow)?; DebtorCustomers::::insert(cluster_id, customer_id.clone(), customer_debt); + Self::deposit_event(Event::::Indebted { + cluster_id, + era, + batch_index, + customer_id: customer_id.clone(), + amount: debt, + }); + Self::deposit_event(Event::::ChargeFailed { cluster_id, era, @@ -639,7 +654,7 @@ pub mod pallet { &updated_billing_report.vault, &node_provider_id, reward, - ExistenceRequirement::KeepAlive, + ExistenceRequirement::AllowDeath, )?; updated_billing_report @@ -744,7 +759,7 @@ pub mod pallet { vault, treasury_vault, amount_to_deduct, - ExistenceRequirement::KeepAlive, + ExistenceRequirement::AllowDeath, ) } @@ -758,7 +773,7 @@ pub mod pallet { vault, reserve_vault, amount_to_deduct, - ExistenceRequirement::KeepAlive, + ExistenceRequirement::AllowDeath, ) } @@ -776,7 +791,7 @@ pub mod pallet { vault, &validator_account_id, amount_to_deduct, - ExistenceRequirement::KeepAlive, + ExistenceRequirement::AllowDeath, )?; } diff --git a/pallets/ddc-payouts/src/mock.rs b/pallets/ddc-payouts/src/mock.rs index eaee8ffc6..c181e7431 100644 --- a/pallets/ddc-payouts/src/mock.rs +++ b/pallets/ddc-payouts/src/mock.rs @@ -6,7 +6,7 @@ use crate::{self as pallet_ddc_payouts, *}; use ddc_primitives::{ClusterFeesParams, ClusterPricingParams, NodePubKey, NodeType}; use ddc_traits::{ cluster::{ClusterVisitor, ClusterVisitorError}, - customer::{CustomerCharger, CustomerChargerError}, + customer::CustomerCharger, pallet::PalletVisitor, }; use frame_election_provider_support::SortedListProvider; @@ -23,6 +23,7 @@ use sp_io::TestExternalities; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, + DispatchError, }; use sp_std::prelude::*; @@ -112,8 +113,8 @@ impl CustomerCharger for TestCustomerCharger { content_owner: T::AccountId, billing_vault: T::AccountId, amount: u128, - ) -> Result { - ensure!(amount > 1_000_000, CustomerChargerError::TransferFailed); // any error will do + ) -> Result { + ensure!(amount > 1_000_000, DispatchError::BadOrigin); // any error will do let mut amount_to_charge = amount; if amount_to_charge < 50_000_000 { @@ -127,8 +128,7 @@ impl CustomerCharger for TestCustomerCharger { &billing_vault, charge, ExistenceRequirement::KeepAlive, - ) - .map_err(|_| CustomerChargerError::TransferFailed)?; + )?; Ok(amount_to_charge) } } diff --git a/pallets/ddc-payouts/src/tests.rs b/pallets/ddc-payouts/src/tests.rs index b4df284ae..40480bf88 100644 --- a/pallets/ddc-payouts/src/tests.rs +++ b/pallets/ddc-payouts/src/tests.rs @@ -369,6 +369,17 @@ fn send_charging_customers_batch_works() { } .into(), ); + + System::assert_has_event( + Event::Indebted { + cluster_id, + era, + customer_id: user2_debtor, + batch_index, + amount: debt, + } + .into(), + ); System::assert_last_event( Event::Charged { cluster_id, @@ -380,7 +391,7 @@ fn send_charging_customers_batch_works() { .into(), ); - assert_eq!(System::events().len(), 5 + 3); // 3 for Currency::transfer + assert_eq!(System::events().len(), 5 + 3 + 1); // 3 for Currency::transfer // batch 2 let mut before_total_customer_charge = report.total_customer_charge.clone(); @@ -428,6 +439,7 @@ fn send_charging_customers_batch_works() { assert_eq!(user1_debt, None); let balance_before = Balances::free_balance(DdcPayouts::sub_account_id(cluster_id, era)); + // batch 3 batch_index += 2; before_total_customer_charge = report.total_customer_charge.clone(); @@ -467,6 +479,17 @@ fn send_charging_customers_batch_works() { debt = user3_charge - PARTIAL_CHARGE; assert_eq!(user3_debt, debt); + System::assert_has_event( + Event::Indebted { + cluster_id, + era, + customer_id: user3_debtor, + batch_index, + amount: user3_debt, + } + .into(), + ); + System::assert_last_event( Event::ChargeFailed { cluster_id, diff --git a/traits/src/customer.rs b/traits/src/customer.rs index 2441fd46b..2fd624af1 100644 --- a/traits/src/customer.rs +++ b/traits/src/customer.rs @@ -1,13 +1,13 @@ use codec::{Decode, Encode}; use scale_info::TypeInfo; -use sp_runtime::RuntimeDebug; +use sp_runtime::{DispatchError, RuntimeDebug}; pub trait CustomerCharger { fn charge_content_owner( content_owner: T::AccountId, billing_vault: T::AccountId, amount: u128, - ) -> Result; + ) -> Result; } #[derive(Clone, Encode, Decode, RuntimeDebug, TypeInfo, PartialEq)]