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: Add multiple triggers for write-off #1314

Merged
merged 21 commits into from
Apr 18, 2023
Merged
Show file tree
Hide file tree
Changes from 20 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
1 change: 1 addition & 0 deletions Cargo.lock

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

3 changes: 3 additions & 0 deletions pallets/loans-ref/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ cfg-primitives = { path = "../../libs/primitives", default-features = false }
cfg-traits = { path = "../../libs/traits", default-features = false }
cfg-types = { path = "../../libs/types", default-features = false }

strum = { version = "0.24", default-features = false, features = ["derive"] }

# Optionals for benchamarking
frame-benchmarking = { git = "https://github.com/paritytech/substrate", default-features = false, optional = true, branch = "polkadot-v0.9.37" }

Expand Down Expand Up @@ -57,6 +59,7 @@ std = [
"cfg-types/std",
"frame-benchmarking/std",
"sp-io/std",
"strum/std",
]
runtime-benchmarks = [
"frame-benchmarking",
Expand Down
58 changes: 25 additions & 33 deletions pallets/loans-ref/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ use cfg_types::{
permissions::{PermissionScope, PoolRole, Role},
};
use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite};
use frame_support::traits::{
tokens::nonfungibles::{Create, Mutate},
UnixTime,
use frame_support::{
storage::bounded_vec::BoundedVec,
traits::{
tokens::nonfungibles::{Create, Mutate},
UnixTime,
},
};
use frame_system::RawOrigin;
use sp_arithmetic::FixedPointNumber;
Expand All @@ -16,8 +19,9 @@ use sp_std::{time::Duration, vec};

use super::{
pallet::*,
types::{LoanInfo, MaxBorrowAmount, WriteOffState},
types::{LoanInfo, MaxBorrowAmount},
valuation::{DiscountedCashFlow, ValuationMethod},
write_off::{WriteOffRule, WriteOffTrigger},
};

const OFFSET: Duration = Duration::from_secs(120);
Expand Down Expand Up @@ -77,7 +81,7 @@ where
T::NonFungible::create_collection(&COLLECION_ID.into(), &borrower, &borrower).unwrap();
T::Permissions::add(
PermissionScope::Pool(pool_id),
borrower.clone(),
borrower,
Role::PoolRole(PoolRole::Borrower),
)
.unwrap();
Expand Down Expand Up @@ -134,31 +138,27 @@ where
.unwrap();
}

// Worst case policy where you need to iterate for the whole policy.
fn create_policy() -> BoundedVec<WriteOffRule<T::Rate>, T::MaxWriteOffPolicySize> {
vec![
WriteOffRule::new(
[WriteOffTrigger::PrincipalOverdueDays(0)],
T::Rate::zero(),
T::Rate::zero(),
);
T::MaxWriteOffPolicySize::get() as usize
]
.try_into()
.unwrap()
}

fn set_policy(pool_id: PoolIdOf<T>) {
let pool_admin = account::<T::AccountId>("pool_admin", 0, 0);

// Worst case policy where you need to iterate for the whole policy.
let policy = [
vec![
WriteOffState {
overdue_days: u32::MAX,
percentage: T::Rate::zero(),
penalty: T::Rate::zero(),
};
T::MaxWriteOffPolicySize::get() as usize - 1
],
vec![WriteOffState {
overdue_days: 0, // Last element is overdue
percentage: T::Rate::zero(),
penalty: T::Rate::zero(),
}],
]
.concat();

Pallet::<T>::update_write_off_policy(
RawOrigin::Signed(pool_admin).into(),
pool_id,
policy.try_into().unwrap(),
Self::create_policy(),
)
.unwrap();
}
Expand Down Expand Up @@ -263,15 +263,7 @@ benchmarks! {
update_write_off_policy {
let pool_admin = account("pool_admin", 0, 0);
let pool_id = Helper::<T>::prepare_benchmark();

let state = WriteOffState {
overdue_days: 0,
percentage: T::Rate::zero(),
penalty: T::Rate::zero(),
};
let policy = vec![state; T::MaxWriteOffPolicySize::get() as usize]
.try_into()
.unwrap();
let policy = Helper::<T>::create_policy();

}: _(RawOrigin::Signed(pool_admin), pool_id, policy)

Expand Down
79 changes: 55 additions & 24 deletions pallets/loans-ref/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,14 @@
//! The whole pallet is optimized for the more expensive extrinsic that is
//! [`Pallet::update_portfolio_valuation()`] that should go through all active loans.

pub mod migrations;
pub mod migrations {
pub mod nuke;
pub mod v1;
}

Comment on lines +39 to +43
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not keep them around forever or hide them behind a feature flag to not bloat the wasm. But rather minor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once the migration is not used, the unused code should be removed from the wasm. Until I know, Substrate passes wasm-opt that removes unused symbols/code and things like those.

pub mod types;
pub mod valuation;
pub mod write_off;

#[cfg(test)]
mod mock;
Expand Down Expand Up @@ -82,10 +87,12 @@ pub mod pallet {
traits::{BadOrigin, One, Zero},
ArithmeticError, FixedPointOperand,
};
use sp_std::vec::Vec;
use types::{
self, ActiveLoan, AssetOf, BorrowLoanError, CloseLoanError, CreateLoanError, LoanInfoOf,
PortfolioValuationUpdateType, WriteOffState, WriteOffStatus, WrittenOffError,
PortfolioValuationUpdateType, WrittenOffError,
};
use write_off::{WriteOffRule, WriteOffStatus};

use super::*;

Expand All @@ -94,7 +101,7 @@ pub mod pallet {
<T as Config>::CurrencyId,
>>::PoolId;

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

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
Expand Down Expand Up @@ -237,7 +244,7 @@ pub mod pallet {
_,
Blake2_128Concat,
PoolIdOf<T>,
BoundedVec<WriteOffState<T::Rate>, T::MaxWriteOffPolicySize>,
BoundedVec<WriteOffRule<T::Rate>, T::MaxWriteOffPolicySize>,
ValueQuery,
>;

Expand Down Expand Up @@ -293,7 +300,7 @@ pub mod pallet {
},
WriteOffPolicyUpdated {
pool_id: PoolIdOf<T>,
policy: BoundedVec<WriteOffState<T::Rate>, T::MaxWriteOffPolicySize>,
policy: BoundedVec<WriteOffRule<T::Rate>, T::MaxWriteOffPolicySize>,
},
}

Expand All @@ -305,9 +312,9 @@ pub mod pallet {
LoanNotFound,
/// Emits when a loan exist but it's not active
LoanNotActive,
/// Emits when a write-off state is not found in a policy for a specific loan.
/// Emits when a write-off rule is not found in a policy for a specific loan.
/// It happens when there is no policy or the loan is not overdue.
NoValidWriteOffState,
NoValidWriteOffRule,
/// Emits when the NFT owner is not found
NFTOwnerNotFound,
/// Emits when NFT owner doesn't match the expected owner
Expand Down Expand Up @@ -473,7 +480,7 @@ pub mod pallet {
/// - Write off by admin with percentage 0.5 and penalty 0.2
/// - Time passes and the policy can be applied.
/// - Write of with a policy that says: percentage 0.3, penaly 0.4
/// - The loan is written off with the maximum between the policy and the current state:
/// - The loan is written off with the maximum between the policy and the current rule:
/// percentage 0.5, penaly 0.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
/// percentage 0.5, penaly 0.4
/// percentage 0.5, penalty 0.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Realizing that only means you did a deep review, thanks!!

///
/// No special permisions are required to this call.
Expand All @@ -488,8 +495,9 @@ pub mod pallet {
ensure_signed(origin)?;

let (status, _count) = Self::update_active_loan(pool_id, loan_id, |loan| {
let state = Self::find_write_off_state(pool_id, loan.maturity_date())?;
let limit = state.status().max(loan.write_off_status());
let rule = Self::find_write_off_rule(pool_id, loan)?
.ok_or(Error::<T>::NoValidWriteOffRule)?;
let limit = rule.status.compose_max(loan.write_off_status());

loan.write_off(&limit, &limit)?;

Expand Down Expand Up @@ -532,10 +540,11 @@ pub mod pallet {
};

let _count = Self::update_active_loan(pool_id, loan_id, |loan| {
let state = Self::find_write_off_state(pool_id, loan.maturity_date());
let limit = state.map(|s| s.status()).unwrap_or_else(|_| status.clone());
let rule = Self::find_write_off_rule(pool_id, loan)?;
let limit = rule.map(|r| r.status).unwrap_or_else(|| status.clone());

loan.write_off(&limit, &status)
loan.write_off(&limit, &status)?;
Ok(limit)
lemunozm marked this conversation as resolved.
Show resolved Hide resolved
})?;

Self::deposit_event(Event::<T>::WrittenOff {
Expand Down Expand Up @@ -584,7 +593,7 @@ pub mod pallet {
Ok(())
}

/// Updates the write off policy.
/// Updates the write off policy with write off rules.
///
/// The write off policy is used to automatically set a write off minimum value to the
/// loan.
Expand All @@ -593,7 +602,7 @@ pub mod pallet {
pub fn update_write_off_policy(
origin: OriginFor<T>,
pool_id: PoolIdOf<T>,
policy: BoundedVec<WriteOffState<T::Rate>, T::MaxWriteOffPolicySize>,
policy: BoundedVec<WriteOffRule<T::Rate>, T::MaxWriteOffPolicySize>,
) -> DispatchResult {
let who = ensure_signed(origin)?;
Self::ensure_role(pool_id, &who, PoolRole::PoolAdmin)?;
Expand Down Expand Up @@ -679,16 +688,38 @@ pub mod pallet {
})
}

fn find_write_off_state(
/// From all overdue write off rules, it returns the one has highest percentage
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
/// From all overdue write off rules, it returns the one has highest percentage
/// From all overdue write off rules, it returns the one with the highest percentage

/// (or highest penalty, if same percentage) that can be applied.
///
/// Suppose a policy with the following rules:
/// - overdue_days: 5, percentage 10%
/// - overdue_days: 10, percentage 30%
/// - overdue_days: 15, percentage 20%
///
/// If the loan is not overdue, it will not return any rule.
/// If the loan overdue by 4 days, it will not return any rule.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
/// If the loan overdue by 4 days, it will not return any rule.
/// If the loan is overdue by 4 days, it will not return any rule.

/// If the loan is overdue by 9 days, it will return the first rule.
/// If the loan is overdue by 60 days, it will return the second rule
/// (because it has a higher percetage).
fn find_write_off_rule(
pool_id: PoolIdOf<T>,
maturity_date: Moment,
) -> Result<WriteOffState<T::Rate>, DispatchError> {
WriteOffState::find_best(
WriteOffPolicy::<T>::get(pool_id).into_iter(),
maturity_date,
T::Time::now().as_secs(),
)
.ok_or_else(|| Error::<T>::NoValidWriteOffState.into())
loan: &ActiveLoan<T>,
) -> Result<Option<WriteOffRule<T::Rate>>, DispatchError> {
Ok(WriteOffPolicy::<T>::get(pool_id)
.into_iter()
.filter_map(|rule| {
rule.triggers
.iter()
.map(|trigger| loan.check_write_off_trigger(&trigger.0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe it would be better to destructure into the 0th tuple element directly to make it more explicit?

Suggested change
.map(|trigger| loan.check_write_off_trigger(&trigger.0))
.map(|(trigger, _)| loan.check_write_off_trigger(&trigger))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has only one element, the map part should be something like .map(|UniqueWriteOffTrigger(trigger)|), which maybe is more confusing (line breaks and adds a new indentation). It if was a tuple, I'd agree with you.

.find(|e| match e {
Ok(value) => *value,
Err(_) => true,
})
.map(|result| result.map(|_| rule))
})
.collect::<Result<Vec<_>, _>>()? // This exits if error before getting the maximum
.into_iter()
.max_by(|r1, r2| r1.status.cmp(&r2.status)))
}

fn update_portfolio_valuation_with_pv(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use sp_std::vec::Vec;

use crate::*;

mod v0 {
mod old {
use super::*;

/// This storage comes from the previous pallet loans.
Expand All @@ -24,9 +24,9 @@ mod v0 {
}

/// This migration nukes all storages from the pallet individually.
pub struct NukeMigration<T>(sp_std::marker::PhantomData<T>);
pub struct Migration<T>(sp_std::marker::PhantomData<T>);

impl<T: Config> OnRuntimeUpgrade for NukeMigration<T> {
impl<T: Config> OnRuntimeUpgrade for Migration<T> {
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
ensure!(
Expand All @@ -35,15 +35,15 @@ impl<T: Config> OnRuntimeUpgrade for NukeMigration<T> {
);

ensure!(
v0::NextLoanId::<T>::iter_values().count() == 1,
old::NextLoanId::<T>::iter_values().count() == 1,
"Pallet loans contains doesn't contain old data"
);

Ok(Vec::new())
}

fn on_runtime_upgrade() -> Weight {
let old_values = v0::NextLoanId::<T>::iter_values().count();
let old_values = old::NextLoanId::<T>::iter_values().count();
if old_values > 0 {
let result = storage::unhashed::clear_prefix(&loan_prefix(), None, None);

Expand All @@ -69,7 +69,7 @@ impl<T: Config> OnRuntimeUpgrade for NukeMigration<T> {
);

ensure!(
v0::NextLoanId::<T>::iter_values().count() == 0,
old::NextLoanId::<T>::iter_values().count() == 0,
"Pallet loans still contains old data"
);

Expand Down