Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loans: ChangeGuard trait and loans modifications #1384

Merged
merged 20 commits into from
Jun 7, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

53 changes: 53 additions & 0 deletions libs/mocks/src/change_guard.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#[frame_support::pallet]
pub mod pallet_mock_change_guard {
use cfg_traits::changes::ChangeGuard;
use frame_support::pallet_prelude::*;
use mock_builder::{execute_call, register_call};

#[pallet::config]
pub trait Config: frame_system::Config {
type PoolId;
type ChangeId;
type Change;
}

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
pub struct Pallet<T>(_);

#[pallet::storage]
pub(super) type CallIds<T: Config> = StorageMap<
_,
Blake2_128Concat,
<Blake2_128 as frame_support::StorageHasher>::Output,
mock_builder::CallId,
>;

impl<T: Config> Pallet<T> {
pub fn mock_note(
f: impl Fn(T::PoolId, T::Change) -> Result<T::ChangeId, DispatchError> + 'static,
) {
register_call!(move |(a, b)| f(a, b));
}

pub fn mock_released(
f: impl Fn(T::PoolId, T::ChangeId) -> Result<T::Change, DispatchError> + 'static,
) {
register_call!(move |(a, b)| f(a, b));
}
}

impl<T: Config> ChangeGuard for Pallet<T> {
type Change = T::Change;
type ChangeId = T::ChangeId;
type PoolId = T::PoolId;

fn note(a: T::PoolId, b: T::Change) -> Result<T::ChangeId, DispatchError> {
execute_call!((a, b))
}

fn released(a: T::PoolId, b: T::ChangeId) -> Result<T::Change, DispatchError> {
execute_call!((a, b))
}
}
}
2 changes: 2 additions & 0 deletions libs/mocks/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
mod change_guard;
mod data;
mod fees;
mod permissions;
mod pools;
mod rewards;

pub use change_guard::pallet_mock_change_guard;
pub use data::pallet as pallet_mock_data;
pub use fees::pallet as pallet_mock_fees;
pub use permissions::pallet as pallet_mock_permissions;
Expand Down
35 changes: 35 additions & 0 deletions libs/traits/src/changes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
use sp_runtime::DispatchError;

/// Trait for get feedback before apply certain changes.
/// It can be used when you need to ask to a third party or external module if
/// applying a change that has some effect into the system is something healthy.
pub trait ChangeGuard {
/// Associated pool where evaluate the change.
type PoolId;

/// Identification of a change.
type ChangeId;

/// Kind of change.
type Change;

/// Notify a `change` related to a `pool_id`.
/// The caller to this method ask for feedback for the implementation of
/// this trait in order be able to semantically proceed successful with that
/// change. The change intention will be noted by this method and identified
/// by the returned ChangeId.
fn note(pool_id: Self::PoolId, change: Self::Change) -> Result<Self::ChangeId, DispatchError>;

/// Ask for a `change_id` if it's ready to proceed.
/// An error will be returned if:
/// - The change not exists.
/// - The change is not ready to be applied yet. The conditions not
/// fulfilled.
/// - The change was already released.
/// - The change has expired.
/// If `Ok()`, the caller can proceed.
fn released(
pool_id: Self::PoolId,
change_id: Self::ChangeId,
) -> Result<Self::Change, DispatchError>;
}
3 changes: 3 additions & 0 deletions libs/traits/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ pub mod rewards;
/// Traits related to data registry & collections.
pub mod data;

/// Traits related to checked changes.
pub mod changes;

/// A trait used for loosely coupling the claim pallet with a reward mechanism.
///
/// ## Overview
Expand Down
110 changes: 107 additions & 3 deletions pallets/loans-ref/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
//! | [`Pallet::create()`] | Borrower |
//! | [`Pallet::borrow()`] | Borrower |
//! | [`Pallet::repay()`] | Borrower |
//! | [`Pallet::write_off()`] | Any |
//! | [`Pallet::write_off()`] | |
//! | [`Pallet::admin_write_off()`] | LoanAdmin |
//! | [`Pallet::close()`] | Borrower |
//!
Expand All @@ -33,6 +33,13 @@
//! | [`Pallet::update_write_off_policy()`] | PoolAdmin |
//! | [`Pallet::update_portfolio_valuation()`] | Any |
//!
//! The following actions are performed based on changes:
//!
//! | Extrinsics | Role |
//! |-------------------------------------|-----------|
//! | [`Pallet::propose_loan_mutation()`] | LoanAdmin |
//! | [`Pallet::apply_change()`] | |
//!
//! The whole pallet is optimized for the more expensive extrinsic that is
//! [`Pallet::update_portfolio_valuation()`] that should go through all active
//! loans.
Expand Down Expand Up @@ -65,6 +72,7 @@ pub use weights::WeightInfo;
pub mod pallet {
use cfg_primitives::Moment;
use cfg_traits::{
changes::ChangeGuard,
data::{DataCollection, DataRegistry},
ops::{EnsureAdd, EnsureAddAssign, EnsureInto},
InterestAccrual, Permissions, PoolInspect, PoolNAV, PoolReserve,
Expand All @@ -75,6 +83,7 @@ pub mod pallet {
};
use frame_support::{
pallet_prelude::*,
storage::transactional,
traits::{
tokens::{
self,
Expand All @@ -89,14 +98,15 @@ pub mod pallet {
use sp_arithmetic::FixedPointNumber;
use sp_runtime::{
traits::{BadOrigin, One, Zero},
ArithmeticError, FixedPointOperand,
ArithmeticError, FixedPointOperand, TransactionOutcome,
};
use sp_std::vec::Vec;
use types::{
self,
policy::{self, WriteOffRule, WriteOffStatus},
portfolio::{self, InitialPortfolioValuation, PortfolioValuationUpdateType},
BorrowLoanError, CloseLoanError, CreateLoanError, RepayLoanError, WrittenOffError,
BorrowLoanError, Change, CloseLoanError, CreateLoanError, LoanMutation, MutationError,
RepayLoanError, WrittenOffError,
};

use super::*;
Expand All @@ -114,6 +124,7 @@ pub mod pallet {
pub type AssetOf<T> = (<T as Config>::CollectionId, <T as Config>::ItemId);
pub type PriceOf<T> = (<T as Config>::Rate, Moment);
pub type PriceResultOf<T> = Result<PriceOf<T>, DispatchError>;
pub type LoanChangeOf<T> = Change<<T as Config>::LoanId, <T as Config>::Rate>;

const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);

Expand All @@ -129,6 +140,9 @@ pub mod pallet {
/// Identify a curreny.
type CurrencyId: Parameter + Copy + MaxEncodedLen;

/// Identify a loan change.
type ChangeId: Parameter + Copy + MaybeSerializeDeserialize + MaxEncodedLen + TypeInfo;

/// Identify a non fungible collection
type CollectionId: Parameter
+ Member
Expand Down Expand Up @@ -206,6 +220,13 @@ pub mod pallet {
NormalizedDebt = Self::Balance,
>;

/// Used to confirm active loan modification properties.
type ChangeGuard: ChangeGuard<
PoolId = PoolIdOf<Self>,
ChangeId = Self::ChangeId,
Change = LoanChangeOf<Self>,
>;

/// Max number of active loans per pool.
#[pallet::constant]
type MaxActiveLoansPerPool: Get<u32>;
Expand Down Expand Up @@ -314,6 +335,12 @@ pub mod pallet {
loan_id: T::LoanId,
status: WriteOffStatus<T::Rate>,
},
/// A loan was modified
hieronx marked this conversation as resolved.
Show resolved Hide resolved
Mutated {
pool_id: PoolIdOf<T>,
loan_id: T::LoanId,
mutation: LoanMutation<T::Rate>,
},
/// A loan was closed
Closed {
pool_id: PoolIdOf<T>,
Expand Down Expand Up @@ -361,6 +388,8 @@ pub mod pallet {
WrittenOffError(WrittenOffError),
/// Emits when the loan can not be closed
CloseLoanError(CloseLoanError),
/// Emits when the loan can not be modified
hieronx marked this conversation as resolved.
Show resolved Hide resolved
MutationError(MutationError),
}

impl<T> From<CreateLoanError> for Error<T> {
Expand Down Expand Up @@ -393,6 +422,12 @@ pub mod pallet {
}
}

impl<T> From<MutationError> for Error<T> {
fn from(error: MutationError) -> Self {
Error::<T>::MutationError(error)
}
}

#[pallet::call]
impl<T: Config> Pallet<T>
where
Expand Down Expand Up @@ -680,6 +715,61 @@ pub mod pallet {

Ok(Some(T::WeightInfo::update_portfolio_valuation(count)).into())
}

/// Propose a change.
/// The change is not performed until you call
/// [`Pallet::apply_change()`].
#[pallet::weight(100_000_000)]
#[pallet::call_index(8)]
pub fn propose_loan_mutation(
origin: OriginFor<T>,
pool_id: PoolIdOf<T>,
loan_id: T::LoanId,
mutation: LoanMutation<T::Rate>,
) -> DispatchResult {
let who = ensure_signed(origin)?;
Self::ensure_role(pool_id, &who, PoolRole::LoanAdmin)?;

let (mut loan, _count) = Self::get_active_loan(pool_id, loan_id)?;
transactional::with_transaction(|| {
let result = loan.mutate_with(mutation.clone());

// We do not want to apply the mutation,
// only check if there is no error in applying it
TransactionOutcome::Rollback(result)
})?;
Comment on lines +734 to +741
Copy link
Contributor Author

@lemunozm lemunozm Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new. It checks that the modification can be applied, at least when it is proposed. It doesn't mean later it can be applied or not, but at least it removes most of the bad usages for it, for example, trying to modify the interest rate in external pricing. If we don't check this, the change will be enqueued, and we will get the error weeks later.

This is not 100% reliable, because once the change can be applied, the loan could have been closed, or its state changed. i.e:

let change_id_1 = Loans::propose_loan_mutation(DCF::DiscountRate(0.2)); // ==> Ok
let change_id_2 = Loans::propose_loan_mutation(ValuationMethod::OutstandingDebt); // ==> Ok

// Time passes and both proposals become available to be applied

Loans::apply_change(change_id_2); // ==> Ok
Loans::apply_change(change_id_1); // ==> MutationError::DiscountedCashFlowExpected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea! We definitely should incorporate the same in pool-system


T::ChangeGuard::note(pool_id, Change::Loan(loan_id, mutation))?;

lemunozm marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
}

/// Apply a proposed change identified by a change id.
/// It will only perform the change if the requirements for it
/// are fulfilled.
#[pallet::weight(100_000_000)]
#[pallet::call_index(9)]
pub fn apply_change(
origin: OriginFor<T>,
pool_id: PoolIdOf<T>,
change_id: T::ChangeId,
) -> DispatchResult {
ensure_signed(origin)?;

let Change::Loan(loan_id, mutation) = T::ChangeGuard::released(pool_id, change_id)?;

let (_, _count) = Self::update_active_loan(pool_id, loan_id, |loan| {
loan.mutate_with(mutation.clone())
})?;

Self::deposit_event(Event::<T>::Mutated {
pool_id,
loan_id,
mutation,
});

Ok(())
}
}

/// Utility methods
Expand Down Expand Up @@ -845,6 +935,20 @@ pub mod pallet {
})
}

fn get_active_loan(
pool_id: PoolIdOf<T>,
loan_id: T::LoanId,
) -> Result<(ActiveLoan<T>, u32), DispatchError> {
let active_loans = ActiveLoans::<T>::get(pool_id);
let count = active_loans.len().ensure_into()?;
let (_, loan) = active_loans
.into_iter()
.find(|(id, _)| *id == loan_id)
.ok_or(Error::<T>::LoanNotActiveOrNotFound)?;

Ok((loan, count))
}

#[cfg(feature = "runtime-benchmarks")]
/// Set the maturity date of the loan to this instant.
pub fn expire(pool_id: PoolIdOf<T>, loan_id: T::LoanId) -> DispatchResult {
Expand Down
24 changes: 20 additions & 4 deletions pallets/loans-ref/src/loan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ use crate::{
},
types::{
policy::{WriteOffStatus, WriteOffTrigger},
BorrowLoanError, BorrowRestrictions, CloseLoanError, CreateLoanError, LoanRestrictions,
RepayLoanError, RepayRestrictions, RepaymentSchedule,
BorrowLoanError, BorrowRestrictions, CloseLoanError, CreateLoanError, LoanMutation,
LoanRestrictions, MutationError, RepayLoanError, RepayRestrictions, RepaymentSchedule,
},
};

Expand Down Expand Up @@ -127,7 +127,7 @@ impl<T: Config> ClosedLoan<T> {
}

/// Data containing an active loan.
#[derive(Encode, Decode, Clone, TypeInfo, MaxEncodedLen)]
#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebugNoBound, TypeInfo, MaxEncodedLen)]
#[scale_info(skip_type_params(T))]
pub struct ActiveLoan<T: Config> {
/// Specify the repayments schedule of the loan
Expand Down Expand Up @@ -368,7 +368,7 @@ impl<T: Config> ActiveLoan<T> {

pub fn write_off(&mut self, new_status: &WriteOffStatus<T::Rate>) -> DispatchResult {
if let ActivePricing::Internal(inner) = &mut self.pricing {
inner.update_penalty(new_status.penalty)?;
inner.set_penalty(new_status.penalty)?;
}

self.write_off_percentage = new_status.percentage;
Expand Down Expand Up @@ -411,6 +411,22 @@ impl<T: Config> ActiveLoan<T> {
Ok((loan, self.borrower))
}

pub fn mutate_with(&mut self, mutation: LoanMutation<T::Rate>) -> DispatchResult {
match mutation {
LoanMutation::Maturity(maturity) => self.schedule.maturity = maturity,
LoanMutation::InterestPayments(payments) => self.schedule.interest_payments = payments,
LoanMutation::PayDownSchedule(schedule) => self.schedule.pay_down_schedule = schedule,
LoanMutation::Internal(mutation) => match &mut self.pricing {
ActivePricing::Internal(inner) => inner.mutate_with(mutation)?,
ActivePricing::External(_) => {
Err(Error::<T>::from(MutationError::InternalPricingExpected))?
}
},
};

hieronx marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
}

#[cfg(feature = "runtime-benchmarks")]
pub fn set_maturity(&mut self, duration: Moment) {
self.schedule.maturity = crate::types::Maturity::Fixed(duration);
Expand Down