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

@@ -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::<T>::UnauthorizedCharge)
@@ -407,6 +410,7 @@ pub mod pallet {
 			})?;
 
 			Self::deposit_event(Event::<T>::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::<T>::UnauthorizedCharge)
@@ -443,6 +447,7 @@ pub mod pallet {
 			})?;
 
 			Self::deposit_event(Event::<T>::Uncharged {
+				pool_id,
 				fee_id,
 				amount,
 				pending,
@@ -500,11 +505,12 @@ pub mod pallet {
 				.ok_or(Error::<T>::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<T>) -> Result<T::Balance, DispatchError>,
-		) -> Result<T::Balance, DispatchError> {
+		) -> Result<(T::PoolId, T::Balance), DispatchError> {
 			let (pool_id, bucket) =
 				FeeIdsToPoolBucket::<T>::get(fee_id).ok_or(Error::<T>::FeeNotFound)?;
 
@@ -515,9 +521,9 @@ pub mod pallet {
 					.ok_or(Error::<T>::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::<T>::Paid {
+						pool_id,
 						fee_id: fee.id,
 						amount: fee.amounts.disbursement,
 						destination: fee.destination.clone(),

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,

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::<Runtime>::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::<Runtime>::Charged {
+							pool_id: POOL,
 							fee_id,
 							amount: 337,
 							pending: 1337,
@@ -181,6 +183,7 @@ mod extrinsics {
 
 					System::assert_last_event(
 						Event::<Runtime>::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,

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::<T>::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)?;

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<Runtime>)> =
@@ -3079,10 +3079,48 @@ mod pool_fees {
 			));
 
 			// NAV = 0 + AUM - PoolFeesNAV = -AUM
-			assert_noop!(
-				PoolSystem::close_epoch(RuntimeOrigin::signed(POOL_OWNER), 0),
-				Error::<Runtime>::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<Runtime>)> =
+				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);

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 })
 		}

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 })
 		}

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 })
 		}