Added MutatedActiveLoan storage for PoV optimization

libs/traits/src/ops.rs

@@ -4,6 +4,24 @@ pub use ensure::{
 	EnsureSubAssign,
 };
 
+pub mod storage {
+	use sp_runtime::{traits::Get, BoundedVec};
+
+	pub trait BoundedVecExt<T> {
+		/// Same as [`BoundedVec::try_push()`] but returns a reference to the pushed element in the
+		/// vector
+		fn try_push_fetch(&mut self, element: T) -> Result<&mut T, T>;
+	}
+
+	impl<T, S: Get<u32>> BoundedVecExt<T> for BoundedVec<T, S> {
+		fn try_push_fetch(&mut self, element: T) -> Result<&mut T, T> {
+			let len = self.len();
+			self.try_push(element)?;
+			Ok(self.get_mut(len).expect("This can not fail. qed"))
+		}
+	}
+}
+
 mod ensure {
 	use sp_runtime::{
 		traits::{CheckedAdd, CheckedDiv, CheckedMul, CheckedSub, Zero},

pallets/loans-ref/src/lib.rs

@@ -57,7 +57,7 @@ pub use weights::WeightInfo;
 pub mod pallet {
 	use cfg_primitives::Moment;
 	use cfg_traits::{
-		ops::{EnsureAdd, EnsureAddAssign, EnsureInto},
+		ops::{storage::BoundedVecExt, EnsureAdd, EnsureAddAssign, EnsureInto, EnsureSub},
 		InterestAccrual, Permissions, PoolInspect, PoolNAV, PoolReserve,
 	};
 	use cfg_types::{
@@ -79,7 +79,7 @@ pub mod pallet {
 	use sp_arithmetic::FixedPointNumber;
 	use sp_runtime::{
 		traits::{BadOrigin, One, Zero},
-		ArithmeticError, FixedPointOperand,
+		ArithmeticError, BoundedBTreeMap, FixedPointOperand,
 	};
 	use types::{
 		self, ActiveLoan, AssetOf, BorrowLoanError, CloseLoanError, CreateLoanError, LoanInfoOf,
@@ -131,6 +131,7 @@ pub mod pallet {
 			+ MaxEncodedLen
 			+ Copy
 			+ EnsureAdd
+			+ Ord
 			+ One;
 
 		/// Defines the rate type used for math computations
@@ -209,7 +210,25 @@ pub mod pallet {
 		_,
 		Blake2_128Concat,
 		PoolIdOf<T>,
-		BoundedVec<(ActiveLoan<T>, Moment), T::MaxActiveLoansPerPool>,
+		BoundedBTreeMap<T::LoanId, ActiveLoan<T>, T::MaxActiveLoansPerPool>,
+		ValueQuery,
+	>;
+
+	/// Storage for mutated active loans.
+	/// A mutated active loan is an active loan that has changed from the last
+	/// `update_portfolio_valuation()` call.
+	/// This storage is used to avoid PoV Weights in the probably large `ActiveLoans` storage.
+	/// Each `ActiveLoan` change is done over a copy of that loan in this storage which implies
+	/// to statistically write less bytes (there are less mutated active loans than total active
+	/// loans).
+	/// Once `update_portfolio_valuation()` is called, the `ActiveLoans` storage is updated with
+	/// the `MutatedActiveLoans` content and this last storage is cleaned.
+	#[pallet::storage]
+	pub(crate) type MutatedActiveLoans<T: Config> = StorageMap<
+		_,
+		Blake2_128Concat,
+		PoolIdOf<T>,
+		BoundedVec<(T::LoanId, ActiveLoan<T>, Moment), T::MaxActiveLoansPerPool>,
 		ValueQuery,
 	>;
 
@@ -400,10 +419,10 @@ pub mod pallet {
 				Some(created_loan) => {
 					Self::ensure_loan_borrower(&who, created_loan.borrower())?;
 
-					let mut active_loan = created_loan.activate(loan_id)?;
+					let mut active_loan = created_loan.activate()?;
 					active_loan.borrow(amount)?;
 
-					Self::insert_active_loan(pool_id, active_loan)?
+					Self::insert_active_loan(pool_id, loan_id, active_loan)?
 				}
 				None => {
 					Self::update_active_loan(pool_id, loan_id, |loan| {
@@ -713,20 +732,32 @@ pub mod pallet {
 			pool_id: PoolIdOf<T>,
 		) -> Result<(T::Balance, u32), DispatchError> {
 			let rates = T::InterestAccrual::rates();
-			let loans = ActiveLoans::<T>::get(pool_id);
-			let count = loans.len().ensure_into()?;
-			let value = loans.into_iter().try_fold(
-				T::Balance::zero(),
-				|sum, (loan, _)| -> Result<T::Balance, DispatchError> {
-					Ok(sum.ensure_add(loan.current_present_value(&rates)?)?)
-				},
-			)?;
-
-			Ok((value, count))
+
+			ActiveLoans::<T>::try_mutate(pool_id, |loans| {
+				for (loan_id, loan, _) in MutatedActiveLoans::<T>::take(pool_id) {
+					loans
+						.try_insert(loan_id, loan)
+						.map_err(|_| Error::<T>::MaxActiveLoansReached)?;
+				}
+
+				let value = loans.values().try_fold(
+					T::Balance::zero(),
+					|sum, loan| -> Result<T::Balance, DispatchError> {
+						Ok(sum.ensure_add(loan.current_present_value(&rates)?)?)
+					},
+				)?;
+
+				// TODO: How to get the correct count to benchmark this accurated?
+				// Should it be MutatedActiveLoans.len() + ActiveLoans.len()?
+				let count = loans.len().ensure_into()?;
+
+				Ok((value, count))
+			})
 		}
 
 		fn insert_active_loan(
 			pool_id: PoolIdOf<T>,
+			loan_id: T::LoanId,
 			loan: ActiveLoan<T>,
 		) -> Result<u32, DispatchError> {
 			PortfolioValuation::<T>::try_mutate(pool_id, |portfolio| {
@@ -735,12 +766,11 @@ pub mod pallet {
 
 				Self::update_portfolio_valuation_with_pv(pool_id, portfolio, Zero::zero(), new_pv)?;
 
-				ActiveLoans::<T>::try_mutate(pool_id, |active_loans| {
-					active_loans
-						.try_push((loan, last_updated))
+				MutatedActiveLoans::<T>::try_mutate(pool_id, |mutated_loans| {
+					mutated_loans
+						.try_push((loan_id, loan, last_updated))
 						.map_err(|_| Error::<T>::MaxActiveLoansReached)?;
-
-					Ok(active_loans.len().ensure_into()?)
+					Ok(mutated_loans.len().ensure_into()?)
 				})
 			})
 		}
@@ -754,19 +784,29 @@ pub mod pallet {
 			F: FnOnce(&mut ActiveLoan<T>) -> Result<R, DispatchError>,
 		{
 			PortfolioValuation::<T>::try_mutate(pool_id, |portfolio| {
-				ActiveLoans::<T>::try_mutate(pool_id, |active_loans| {
-					let (loan, last_updated) = active_loans
-						.iter_mut()
-						.find(|(loan, _)| loan.loan_id() == loan_id)
-						.ok_or_else(|| {
-							if CreatedLoan::<T>::contains_key(pool_id, loan_id) {
-								Error::<T>::LoanNotActive
-							} else {
-								Error::<T>::LoanNotFound
+				MutatedActiveLoans::<T>::try_mutate(pool_id, |mutated_loans| {
+					let (loan, last_updated) =
+						match mutated_loans.iter_mut().find(|(id, _, _)| *id == loan_id) {
+							Some((_, loan, last_updated)) => (loan, last_updated),
+							None => {
+								let loan = ActiveLoans::<T>::get(pool_id)
+									.get(&loan_id)
+									.ok_or_else(|| {
+										if CreatedLoan::<T>::contains_key(pool_id, loan_id) {
+											Error::<T>::LoanNotActive
+										} else {
+											Error::<T>::LoanNotFound
+										}
+									})?
+									.clone();
+
+								mutated_loans
+									.try_push_fetch((loan_id, loan, portfolio.last_updated()))
+									.map(|(_, loan, last_updated)| (loan, last_updated))
+									.map_err(|_| Error::<T>::MaxActiveLoansReached)?
 							}
-						})?;
+						};
 
-					*last_updated = (*last_updated).max(portfolio.last_updated());
 					let old_pv = loan.present_value_at(*last_updated)?;
 
 					let result = f(loan)?;
@@ -776,7 +816,7 @@ pub mod pallet {
 
 					Self::update_portfolio_valuation_with_pv(pool_id, portfolio, old_pv, new_pv)?;
 
-					Ok((result, active_loans.len().ensure_into()?))
+					Ok((result, mutated_loans.len().ensure_into()?))
 				})
 			})
 		}
@@ -785,16 +825,24 @@ pub mod pallet {
 			pool_id: PoolIdOf<T>,
 			loan_id: T::LoanId,
 		) -> Result<(ActiveLoan<T>, u32), DispatchError> {
-			ActiveLoans::<T>::try_mutate(pool_id, |active_loans| {
-				let index = active_loans
-					.iter()
-					.position(|(loan, _)| loan.loan_id() == loan_id)
-					.ok_or(Error::<T>::LoanNotFound)?;
-
-				Ok((
-					active_loans.swap_remove(index).0,
-					active_loans.len().ensure_into()?,
-				))
+			// A loan can be:
+			// - Only in MutatedActiveLoans, if it was just inserted
+			// - Only in ActiveLoans, if it was not modified since the portfolio valuation
+			// - In both, if it was modified after porfolio valuation
+			MutatedActiveLoans::<T>::try_mutate(pool_id, |mutated_loans| {
+				ActiveLoans::<T>::try_mutate(pool_id, |loans| {
+					let count = loans.len().ensure_add(mutated_loans.len())?.ensure_into()?;
+
+					let loan = loans.remove(&loan_id);
+					let index = mutated_loans.iter().position(|(id, _, _)| *id == loan_id);
+
+					let loan = match index {
+						Some(index) => mutated_loans.swap_remove(index).1,
+						None => loan.ok_or(Error::<T>::LoanNotFound)?,
+					};
+
+					Ok((loan, count))
+				})
 			})
 		}
 

pallets/loans-ref/src/tests.rs

@@ -6,7 +6,7 @@ use sp_runtime::traits::BadOrigin;
 
 use super::{
 	mock::*,
-	pallet::{ActiveLoans, Error, LastLoanId, PortfolioValuation},
+	pallet::{ActiveLoans, Error, LastLoanId, MutatedActiveLoans, PortfolioValuation},
 	types::{
 		ActiveLoan, BorrowLoanError, CloseLoanError, CreateLoanError, LoanInfo, MaxBorrowAmount,
 		WriteOffState, WriteOffStatus, WrittenOffError,
@@ -35,11 +35,16 @@ mod util {
 	}
 
 	pub fn get_loan(loan_id: LoanId) -> ActiveLoan<Runtime> {
-		ActiveLoans::<Runtime>::get(POOL_A)
+		match MutatedActiveLoans::<Runtime>::get(POOL_A)
 			.into_iter()
-			.find(|(loan, _)| loan.loan_id() == loan_id)
-			.unwrap()
-			.0
+			.find(|(id, _, _)| *id == loan_id)
+		{
+			Some((_, loan, _)) => loan,
+			None => ActiveLoans::<Runtime>::get(POOL_A)
+				.get(&loan_id)
+				.unwrap()
+				.clone(),
+		}
 	}
 
 	pub fn portfolio_valuation() -> Balance {

pallets/loans-ref/src/types.rs

@@ -394,8 +394,8 @@ impl<T: Config> CreatedLoan<T> {
 		&self.borrower
 	}
 
-	pub fn activate(self, loan_id: T::LoanId) -> Result<ActiveLoan<T>, DispatchError> {
-		ActiveLoan::new(loan_id, self.info, self.borrower, T::Time::now().as_secs())
+	pub fn activate(self) -> Result<ActiveLoan<T>, DispatchError> {
+		ActiveLoan::new(self.info, self.borrower, T::Time::now().as_secs())
 	}
 
 	pub fn close(self) -> Result<(ClosedLoan<T>, T::AccountId), DispatchError> {
@@ -437,9 +437,6 @@ impl<T: Config> ClosedLoan<T> {
 #[derive(Encode, Decode, Clone, TypeInfo, MaxEncodedLen)]
 #[scale_info(skip_type_params(T))]
 pub struct ActiveLoan<T: Config> {
-	/// Id of this loan
-	loan_id: T::LoanId,
-
 	/// Loan information
 	info: LoanInfoOf<T>,
 
@@ -464,13 +461,11 @@ pub struct ActiveLoan<T: Config> {
 
 impl<T: Config> ActiveLoan<T> {
 	pub fn new(
-		loan_id: T::LoanId,
 		info: LoanInfoOf<T>,
 		borrower: T::AccountId,
 		now: Moment,
 	) -> Result<Self, DispatchError> {
 		Ok(ActiveLoan {
-			loan_id,
 			info: LoanInfo {
 				interest_rate: T::InterestAccrual::reference_yearly_rate(info.interest_rate)?,
 				..info
@@ -484,10 +479,6 @@ impl<T: Config> ActiveLoan<T> {
 		})
 	}
 
-	pub fn loan_id(&self) -> T::LoanId {
-		self.loan_id
-	}
-
 	pub fn borrower(&self) -> &T::AccountId {
 		&self.borrower
 	}