From 4b11e6a88c14205f6a590314c75e75ad0ef74e7a Mon Sep 17 00:00:00 2001 From: lemunozm Date: Thu, 23 Mar 2023 18:04:15 +0100 Subject: [PATCH 1/4] active loans using BoundedBTreeMap --- pallets/loans-ref/src/lib.rs | 41 +++++++++++++++------------------- pallets/loans-ref/src/tests.rs | 4 ++-- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/pallets/loans-ref/src/lib.rs b/pallets/loans-ref/src/lib.rs index fff7f91a94..9377264442 100644 --- a/pallets/loans-ref/src/lib.rs +++ b/pallets/loans-ref/src/lib.rs @@ -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,7 @@ pub mod pallet { _, Blake2_128Concat, PoolIdOf, - BoundedVec<(ActiveLoan, Moment), T::MaxActiveLoansPerPool>, + BoundedBTreeMap, Moment), T::MaxActiveLoansPerPool>, ValueQuery, >; @@ -717,7 +718,7 @@ pub mod pallet { let count = loans.len().ensure_into()?; let value = loans.into_iter().try_fold( T::Balance::zero(), - |sum, (loan, _)| -> Result { + |sum, (_, (loan, _))| -> Result { Ok(sum.ensure_add(loan.current_present_value(&rates)?)?) }, )?; @@ -737,7 +738,7 @@ pub mod pallet { ActiveLoans::::try_mutate(pool_id, |active_loans| { active_loans - .try_push((loan, last_updated)) + .try_insert(loan.loan_id(), (loan, last_updated)) .map_err(|_| Error::::MaxActiveLoansReached)?; Ok(active_loans.len().ensure_into()?) @@ -755,16 +756,13 @@ pub mod pallet { { PortfolioValuation::::try_mutate(pool_id, |portfolio| { ActiveLoans::::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::::contains_key(pool_id, loan_id) { - Error::::LoanNotActive - } else { - Error::::LoanNotFound - } - })?; + let (loan, last_updated) = active_loans.get_mut(&loan_id).ok_or_else(|| { + if CreatedLoan::::contains_key(pool_id, loan_id) { + Error::::LoanNotActive + } else { + Error::::LoanNotFound + } + })?; *last_updated = (*last_updated).max(portfolio.last_updated()); let old_pv = loan.present_value_at(*last_updated)?; @@ -786,15 +784,12 @@ pub mod pallet { loan_id: T::LoanId, ) -> Result<(ActiveLoan, u32), DispatchError> { ActiveLoans::::try_mutate(pool_id, |active_loans| { - let index = active_loans - .iter() - .position(|(loan, _)| loan.loan_id() == loan_id) - .ok_or(Error::::LoanNotFound)?; - - Ok(( - active_loans.swap_remove(index).0, - active_loans.len().ensure_into()?, - )) + let loan = active_loans + .remove(&loan_id) + .ok_or(Error::::LoanNotFound)? + .0; + + Ok((loan, active_loans.len().ensure_into()?)) }) } diff --git a/pallets/loans-ref/src/tests.rs b/pallets/loans-ref/src/tests.rs index c684a39ab9..b589849e00 100644 --- a/pallets/loans-ref/src/tests.rs +++ b/pallets/loans-ref/src/tests.rs @@ -36,9 +36,9 @@ mod util { pub fn get_loan(loan_id: LoanId) -> ActiveLoan { ActiveLoans::::get(POOL_A) - .into_iter() - .find(|(loan, _)| loan.loan_id() == loan_id) + .get(&loan_id) .unwrap() + .clone() .0 } From 7ba3adfebfae00c2f5791eb076fa273a0620cb1a Mon Sep 17 00:00:00 2001 From: lemunozm Date: Thu, 23 Mar 2023 18:10:19 +0100 Subject: [PATCH 2/4] remove loan_id from ActiveLoan --- pallets/loans-ref/src/lib.rs | 7 ++++--- pallets/loans-ref/src/types.rs | 13 ++----------- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/pallets/loans-ref/src/lib.rs b/pallets/loans-ref/src/lib.rs index 9377264442..4db793bb7e 100644 --- a/pallets/loans-ref/src/lib.rs +++ b/pallets/loans-ref/src/lib.rs @@ -401,10 +401,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| { @@ -728,6 +728,7 @@ pub mod pallet { fn insert_active_loan( pool_id: PoolIdOf, + loan_id: T::LoanId, loan: ActiveLoan, ) -> Result { PortfolioValuation::::try_mutate(pool_id, |portfolio| { @@ -738,7 +739,7 @@ pub mod pallet { ActiveLoans::::try_mutate(pool_id, |active_loans| { active_loans - .try_insert(loan.loan_id(), (loan, last_updated)) + .try_insert(loan_id, (loan, last_updated)) .map_err(|_| Error::::MaxActiveLoansReached)?; Ok(active_loans.len().ensure_into()?) diff --git a/pallets/loans-ref/src/types.rs b/pallets/loans-ref/src/types.rs index 9f1b951250..967465c2b5 100644 --- a/pallets/loans-ref/src/types.rs +++ b/pallets/loans-ref/src/types.rs @@ -394,8 +394,8 @@ impl CreatedLoan { &self.borrower } - pub fn activate(self, loan_id: T::LoanId) -> Result, DispatchError> { - ActiveLoan::new(loan_id, self.info, self.borrower, T::Time::now().as_secs()) + pub fn activate(self) -> Result, DispatchError> { + ActiveLoan::new(self.info, self.borrower, T::Time::now().as_secs()) } pub fn close(self) -> Result<(ClosedLoan, T::AccountId), DispatchError> { @@ -437,9 +437,6 @@ impl ClosedLoan { #[derive(Encode, Decode, Clone, TypeInfo, MaxEncodedLen)] #[scale_info(skip_type_params(T))] pub struct ActiveLoan { - /// Id of this loan - loan_id: T::LoanId, - /// Loan information info: LoanInfoOf, @@ -464,13 +461,11 @@ pub struct ActiveLoan { impl ActiveLoan { pub fn new( - loan_id: T::LoanId, info: LoanInfoOf, borrower: T::AccountId, now: Moment, ) -> Result { Ok(ActiveLoan { - loan_id, info: LoanInfo { interest_rate: T::InterestAccrual::reference_yearly_rate(info.interest_rate)?, ..info @@ -484,10 +479,6 @@ impl ActiveLoan { }) } - pub fn loan_id(&self) -> T::LoanId { - self.loan_id - } - pub fn borrower(&self) -> &T::AccountId { &self.borrower } From 40a38fff95c669e517a3cd530d672fe223d3adbf Mon Sep 17 00:00:00 2001 From: lemunozm Date: Fri, 24 Mar 2023 08:53:04 +0100 Subject: [PATCH 3/4] using both storages --- pallets/loans-ref/src/lib.rs | 126 ++++++++++++++++++++++++--------- pallets/loans-ref/src/tests.rs | 17 +++-- 2 files changed, 102 insertions(+), 41 deletions(-) diff --git a/pallets/loans-ref/src/lib.rs b/pallets/loans-ref/src/lib.rs index 4db793bb7e..f9a6ef0db0 100644 --- a/pallets/loans-ref/src/lib.rs +++ b/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::{EnsureAdd, EnsureAddAssign, EnsureInto, EnsureSub}, InterestAccrual, Permissions, PoolInspect, PoolNAV, PoolReserve, }; use cfg_types::{ @@ -210,7 +210,25 @@ pub mod pallet { _, Blake2_128Concat, PoolIdOf, - BoundedBTreeMap, Moment), T::MaxActiveLoansPerPool>, + BoundedBTreeMap, 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 = StorageMap< + _, + Blake2_128Concat, + PoolIdOf, + BoundedVec<(T::LoanId, ActiveLoan, Moment), T::MaxActiveLoansPerPool>, ValueQuery, >; @@ -714,16 +732,27 @@ pub mod pallet { pool_id: PoolIdOf, ) -> Result<(T::Balance, u32), DispatchError> { let rates = T::InterestAccrual::rates(); - let loans = ActiveLoans::::get(pool_id); - let count = loans.len().ensure_into()?; - let value = loans.into_iter().try_fold( - T::Balance::zero(), - |sum, (_, (loan, _))| -> Result { - Ok(sum.ensure_add(loan.current_present_value(&rates)?)?) - }, - )?; - - Ok((value, count)) + + ActiveLoans::::try_mutate(pool_id, |loans| { + for (loan_id, loan, _) in MutatedActiveLoans::::take(pool_id) { + loans + .try_insert(loan_id, loan) + .map_err(|_| Error::::MaxActiveLoansReached)?; + } + + let value = loans.values().try_fold( + T::Balance::zero(), + |sum, loan| -> Result { + 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( @@ -737,12 +766,11 @@ pub mod pallet { Self::update_portfolio_valuation_with_pv(pool_id, portfolio, Zero::zero(), new_pv)?; - ActiveLoans::::try_mutate(pool_id, |active_loans| { - active_loans - .try_insert(loan_id, (loan, last_updated)) + MutatedActiveLoans::::try_mutate(pool_id, |mutated_loans| { + mutated_loans + .try_push((loan_id, loan, last_updated)) .map_err(|_| Error::::MaxActiveLoansReached)?; - - Ok(active_loans.len().ensure_into()?) + Ok(mutated_loans.len().ensure_into()?) }) }) } @@ -756,16 +784,33 @@ pub mod pallet { F: FnOnce(&mut ActiveLoan) -> Result, { PortfolioValuation::::try_mutate(pool_id, |portfolio| { - ActiveLoans::::try_mutate(pool_id, |active_loans| { - let (loan, last_updated) = active_loans.get_mut(&loan_id).ok_or_else(|| { - if CreatedLoan::::contains_key(pool_id, loan_id) { - Error::::LoanNotActive - } else { - Error::::LoanNotFound - } - })?; - - *last_updated = (*last_updated).max(portfolio.last_updated()); + MutatedActiveLoans::::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::::get(pool_id) + .get(&loan_id) + .ok_or_else(|| { + if CreatedLoan::::contains_key(pool_id, loan_id) { + Error::::LoanNotActive + } else { + Error::::LoanNotFound + } + })? + .clone(); + + mutated_loans + .try_push((loan_id, loan, portfolio.last_updated())) + .map_err(|_| Error::::MaxActiveLoansReached)?; + + mutated_loans + .get_mut(mutated_loans.len().ensure_sub(1)?) + .map(|(_, loan, last_updated)| (loan, last_updated)) + .ok_or(DispatchError::Other("unreachable"))? + } + }; + let old_pv = loan.present_value_at(*last_updated)?; let result = f(loan)?; @@ -775,7 +820,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()?)) }) }) } @@ -784,13 +829,24 @@ pub mod pallet { pool_id: PoolIdOf, loan_id: T::LoanId, ) -> Result<(ActiveLoan, u32), DispatchError> { - ActiveLoans::::try_mutate(pool_id, |active_loans| { - let loan = active_loans - .remove(&loan_id) - .ok_or(Error::::LoanNotFound)? - .0; - - Ok((loan, 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::::try_mutate(pool_id, |mutated_loans| { + ActiveLoans::::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::::LoanNotFound)?, + }; + + Ok((loan, count)) + }) }) } diff --git a/pallets/loans-ref/src/tests.rs b/pallets/loans-ref/src/tests.rs index b589849e00..a91c0c7973 100644 --- a/pallets/loans-ref/src/tests.rs +++ b/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 { - ActiveLoans::::get(POOL_A) - .get(&loan_id) - .unwrap() - .clone() - .0 + match MutatedActiveLoans::::get(POOL_A) + .into_iter() + .find(|(id, _, _)| *id == loan_id) + { + Some((_, loan, _)) => loan, + None => ActiveLoans::::get(POOL_A) + .get(&loan_id) + .unwrap() + .clone(), + } } pub fn portfolio_valuation() -> Balance { From 436445fcdcd766a8d007c383724d61f5047e7269 Mon Sep 17 00:00:00 2001 From: lemunozm Date: Fri, 24 Mar 2023 10:14:57 +0100 Subject: [PATCH 4/4] add BoundedVecExt --- libs/traits/src/ops.rs | 18 ++++++++++++++++++ pallets/loans-ref/src/lib.rs | 10 +++------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/libs/traits/src/ops.rs b/libs/traits/src/ops.rs index 4006cd43de..df0af1330a 100644 --- a/libs/traits/src/ops.rs +++ b/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 { + /// 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> BoundedVecExt for BoundedVec { + 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}, diff --git a/pallets/loans-ref/src/lib.rs b/pallets/loans-ref/src/lib.rs index f9a6ef0db0..501beca706 100644 --- a/pallets/loans-ref/src/lib.rs +++ b/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, EnsureSub}, + ops::{storage::BoundedVecExt, EnsureAdd, EnsureAddAssign, EnsureInto, EnsureSub}, InterestAccrual, Permissions, PoolInspect, PoolNAV, PoolReserve, }; use cfg_types::{ @@ -801,13 +801,9 @@ pub mod pallet { .clone(); mutated_loans - .try_push((loan_id, loan, portfolio.last_updated())) - .map_err(|_| Error::::MaxActiveLoansReached)?; - - mutated_loans - .get_mut(mutated_loans.len().ensure_sub(1)?) + .try_push_fetch((loan_id, loan, portfolio.last_updated())) .map(|(_, loan, last_updated)| (loan, last_updated)) - .ok_or(DispatchError::Other("unreachable"))? + .map_err(|_| Error::::MaxActiveLoansReached)? } };