From 468a361e71be525ef145cedbbf2dd1684f0155a6 Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Mon, 5 Feb 2024 17:38:08 +0100 Subject: [PATCH] feat: defensive negative balance sheet, expose pool id in fees events (#1718) * feat: expose pool id in fee events * refactor: defensive NAV balance sheet * refactor: reduce weight * fix: clippy --- pallets/pool-fees/src/lib.rs | 19 +++++++---- pallets/pool-fees/src/mock.rs | 1 + pallets/pool-fees/src/tests.rs | 9 +++++ pallets/pool-system/src/lib.rs | 25 +++++++++++--- pallets/pool-system/src/tests/mod.rs | 50 ++++++++++++++++++++++++---- runtime/altair/src/lib.rs | 2 +- runtime/centrifuge/src/lib.rs | 2 +- runtime/development/src/lib.rs | 2 +- 8 files changed, 91 insertions(+), 19 deletions(-) diff --git a/pallets/pool-fees/src/lib.rs b/pallets/pool-fees/src/lib.rs index 879c5321bd..f4ba5016a8 100644 --- a/pallets/pool-fees/src/lib.rs +++ b/pallets/pool-fees/src/lib.rs @@ -250,18 +250,21 @@ pub mod pallet { }, /// A pool fee was charged. Charged { + pool_id: T::PoolId, fee_id: T::FeeId, amount: T::Balance, pending: T::Balance, }, /// A pool fee was uncharged. Uncharged { + pool_id: T::PoolId, fee_id: T::FeeId, amount: T::Balance, pending: T::Balance, }, /// A pool fee was paid. Paid { + pool_id: T::PoolId, fee_id: T::FeeId, amount: T::Balance, destination: T::AccountId, @@ -391,7 +394,7 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; - let pending = Self::mutate_active_fee(fee_id, |fee| { + let (pool_id, pending) = Self::mutate_active_fee(fee_id, |fee| { ensure!( fee.destination == who, DispatchError::from(Error::::UnauthorizedCharge) @@ -407,6 +410,7 @@ pub mod pallet { })?; Self::deposit_event(Event::::Charged { + pool_id, fee_id, amount, pending, @@ -427,7 +431,7 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; - let pending = Self::mutate_active_fee(fee_id, |fee| { + let (pool_id, pending) = Self::mutate_active_fee(fee_id, |fee| { ensure!( fee.destination == who, DispatchError::from(Error::::UnauthorizedCharge) @@ -443,6 +447,7 @@ pub mod pallet { })?; Self::deposit_event(Event::::Uncharged { + pool_id, fee_id, amount, pending, @@ -500,11 +505,12 @@ pub mod pallet { .ok_or(Error::::FeeNotFound)?) } - /// Mutate fee id entry in ActiveFees + /// Mutate the given fee id entry in ActiveFees and return the + /// corresponding pool id. fn mutate_active_fee( fee_id: T::FeeId, mut f: impl FnMut(&mut PoolFeeOf) -> Result, - ) -> Result { + ) -> Result<(T::PoolId, T::Balance), DispatchError> { let (pool_id, bucket) = FeeIdsToPoolBucket::::get(fee_id).ok_or(Error::::FeeNotFound)?; @@ -515,9 +521,9 @@ pub mod pallet { .ok_or(Error::::FeeNotFound)?; if let Some(fee) = fees.get_mut(pos) { - f(fee) + Ok((pool_id, f(fee)?)) } else { - Ok(T::Balance::zero()) + Ok((pool_id, T::Balance::zero())) } }) } @@ -560,6 +566,7 @@ pub mod pallet { )?; Self::deposit_event(Event::::Paid { + pool_id, fee_id: fee.id, amount: fee.amounts.disbursement, destination: fee.destination.clone(), diff --git a/pallets/pool-fees/src/mock.rs b/pallets/pool-fees/src/mock.rs index df59d60841..5937417bd0 100644 --- a/pallets/pool-fees/src/mock.rs +++ b/pallets/pool-fees/src/mock.rs @@ -337,6 +337,7 @@ pub fn pay_single_fee_and_assert( if !fee_amount.is_zero() { System::assert_last_event( Event::Paid { + pool_id: POOL, fee_id, amount: fee_amount, destination: DESTINATION, diff --git a/pallets/pool-fees/src/tests.rs b/pallets/pool-fees/src/tests.rs index 2bd3ae6ff0..87dbd3d21b 100644 --- a/pallets/pool-fees/src/tests.rs +++ b/pallets/pool-fees/src/tests.rs @@ -129,6 +129,7 @@ mod extrinsics { assert_pending_fee(fee_id, fee.clone(), 1000, 0, 0); System::assert_last_event( Event::::Charged { + pool_id: POOL, fee_id, amount: 1000, pending: 1000, @@ -144,6 +145,7 @@ mod extrinsics { assert_pending_fee(fee_id, fee.clone(), 1337, 0, 0); System::assert_last_event( Event::::Charged { + pool_id: POOL, fee_id, amount: 337, pending: 1337, @@ -181,6 +183,7 @@ mod extrinsics { System::assert_last_event( Event::::Uncharged { + pool_id: POOL, fee_id, amount: uncharge_amount, pending: charge_amount - uncharge_amount, @@ -1186,6 +1189,7 @@ mod disbursements { ); System::assert_has_event( Event::Paid { + pool_id: POOL, fee_id: 1, amount: fixed_fee_amount, destination: DESTINATION, @@ -1194,6 +1198,7 @@ mod disbursements { ); System::assert_has_event( Event::Paid { + pool_id: POOL, fee_id: charged_fee_ids[0], amount: charged_y1[0], destination: DESTINATION, @@ -1202,6 +1207,7 @@ mod disbursements { ); System::assert_last_event( Event::Paid { + pool_id: POOL, fee_id: charged_fee_ids[1], amount: payable[1], destination: DESTINATION, @@ -1267,6 +1273,7 @@ mod disbursements { System::assert_has_event( Event::Paid { + pool_id: POOL, fee_id: 1, amount: fixed_fee_amount, destination: DESTINATION, @@ -1275,6 +1282,7 @@ mod disbursements { ); System::assert_has_event( Event::Paid { + pool_id: POOL, fee_id: charged_fee_ids[0], amount: charged_y2[0], destination: DESTINATION, @@ -1283,6 +1291,7 @@ mod disbursements { ); System::assert_last_event( Event::Paid { + pool_id: POOL, fee_id: charged_fee_ids[1], amount: 1, destination: DESTINATION, diff --git a/pallets/pool-system/src/lib.rs b/pallets/pool-system/src/lib.rs index d4d311d4ea..d7bbd0cff6 100644 --- a/pallets/pool-system/src/lib.rs +++ b/pallets/pool-system/src/lib.rs @@ -470,6 +470,14 @@ pub mod pallet { change_id: T::Hash, change: T::RuntimeChange, }, + /// The PoolFeesNAV exceeds the sum of the AUM and the total reserve of + /// the pool + NegativeBalanceSheet { + pool_id: T::PoolId, + nav_aum: T::Balance, + nav_fees: T::Balance, + reserve: T::Balance, + }, } #[pallet::error] @@ -548,9 +556,6 @@ pub mod pallet { ChangeNotFound, /// The external change was found for is not ready yet to be released. ChangeNotReady, - /// The PoolFeesNAV exceeds the sum of the AUM and the total reserve of - /// the pool - NegativeBalanceSheet, } #[pallet::call] @@ -649,7 +654,19 @@ pub mod pallet { let nav = Nav::new(nav_aum, nav_fees); let nav_total = nav .total(pool.reserve.total) - .map_err(|_| Error::::NegativeBalanceSheet)?; + // NOTE: From an accounting perspective, erroring out would be correct. However, + // since investments of this epoch are included in the reserve only in the next + // epoch, every new pool with a configured fee is likely to be blocked if we + // threw an error here. Thus, we dispatch an event as a defensive workaround. + .map_err(|_| { + Self::deposit_event(Event::NegativeBalanceSheet { + pool_id, + nav_aum, + nav_fees, + reserve: pool.reserve.total, + }); + }) + .unwrap_or(T::Balance::default()); let submission_period_epoch = pool.epoch.current; pool.start_next_epoch(now)?; diff --git a/pallets/pool-system/src/tests/mod.rs b/pallets/pool-system/src/tests/mod.rs index 74d27a9477..04bb7d7e53 100644 --- a/pallets/pool-system/src/tests/mod.rs +++ b/pallets/pool-system/src/tests/mod.rs @@ -2680,7 +2680,7 @@ mod pool_fees { use pallet_pool_fees::PoolFeeInfoOf; use super::*; - use crate::mock::default_pool_fees; + use crate::{mock::default_pool_fees, Event}; const POOL_OWNER: MockAccountId = 2; const INVESTMENT_AMOUNT: Balance = DEFAULT_POOL_MAX_RESERVE / 10; @@ -3059,7 +3059,7 @@ mod pool_fees { } #[test] - fn execute_epoch_with_overcharged_fees() { + fn negative_balance_sheet() { new_test_ext().execute_with(|| { let charged_amount = 2 * NAV_AMOUNT; let fees: Vec<(PoolFeeBucket, pallet_pool_fees::PoolFeeInfoOf)> = @@ -3079,10 +3079,48 @@ mod pool_fees { )); // NAV = 0 + AUM - PoolFeesNAV = -AUM - assert_noop!( - PoolSystem::close_epoch(RuntimeOrigin::signed(POOL_OWNER), 0), - Error::::NegativeBalanceSheet - ); + assert_ok!(PoolSystem::close_epoch( + RuntimeOrigin::signed(POOL_OWNER), + 0 + )); + assert!(System::events().iter().any(|e| match e.event { + RuntimeEvent::PoolSystem(Event::NegativeBalanceSheet { + pool_id, + nav_aum, + nav_fees, + reserve, + }) => { + assert_eq!(pool_id, DEFAULT_POOL_ID); + assert!(nav_aum + reserve < nav_fees); + assert_eq!(reserve, 0); + assert!(nav_fees > 0); + assert!(nav_aum < nav_fees); + true + } + _ => false, + })); + }); + } + + #[test] + fn execute_epoch_with_overcharged_fees() { + new_test_ext().execute_with(|| { + let charged_amount = 2 * NAV_AMOUNT; + let fees: Vec<(PoolFeeBucket, pallet_pool_fees::PoolFeeInfoOf)> = + default_pool_fees() + .into_iter() + .map(|fee| (PoolFeeBucket::Top, fee)) + .collect(); + + // Create pool with fees + create_fee_pool_setup(fees); + + // Overcharge fee to increase pending amount and thus PoolFeesNAV + assert_ok!(PoolFees::charge_fee( + RuntimeOrigin::signed(DEFAULT_FEE_DESTINATION), + 2, + charged_amount, + )); // Increase NAV by NAV_AMOUNT to reach equilibrium (AUM == PoolFeesNAV) test_nav_up(DEFAULT_POOL_ID, NAV_AMOUNT); diff --git a/runtime/altair/src/lib.rs b/runtime/altair/src/lib.rs index 82bdbf9d5f..71f0b04b0e 100644 --- a/runtime/altair/src/lib.rs +++ b/runtime/altair/src/lib.rs @@ -2182,7 +2182,7 @@ impl_runtime_apis! { let nav_loans = Loans::update_nav(pool_id).ok()?; let nav_fees = PoolFees::update_nav(pool_id).ok()?; let nav = pallet_pool_system::Nav::new(nav_loans, nav_fees); - let total = nav.total(pool.reserve.total).ok()?; + let total = nav.total(pool.reserve.total).unwrap_or(Balance::default()); Some(PoolNav { nav_aum: nav.nav_aum, nav_fees: nav.nav_fees, reserve: pool.reserve.total, total }) } diff --git a/runtime/centrifuge/src/lib.rs b/runtime/centrifuge/src/lib.rs index 401b81e45f..11bda597e4 100644 --- a/runtime/centrifuge/src/lib.rs +++ b/runtime/centrifuge/src/lib.rs @@ -2207,7 +2207,7 @@ impl_runtime_apis! { let nav_loans = Loans::update_nav(pool_id).ok()?; let nav_fees = PoolFees::update_nav(pool_id).ok()?; let nav = pallet_pool_system::Nav::new(nav_loans, nav_fees); - let total = nav.total(pool.reserve.total).ok()?; + let total = nav.total(pool.reserve.total).unwrap_or(Balance::default()); Some(PoolNav { nav_aum: nav.nav_aum, nav_fees: nav.nav_fees, reserve: pool.reserve.total, total }) } diff --git a/runtime/development/src/lib.rs b/runtime/development/src/lib.rs index 7265d5301f..727c535916 100644 --- a/runtime/development/src/lib.rs +++ b/runtime/development/src/lib.rs @@ -2287,7 +2287,7 @@ impl_runtime_apis! { let nav_loans = Loans::update_nav(pool_id).ok()?; let nav_fees = PoolFees::update_nav(pool_id).ok()?; let nav = pallet_pool_system::Nav::new(nav_loans, nav_fees); - let total = nav.total(pool.reserve.total).ok()?; + let total = nav.total(pool.reserve.total).unwrap_or(Balance::default()); Some(PoolNav { nav_aum: nav.nav_aum, nav_fees: nav.nav_fees, reserve: pool.reserve.total, total }) }