Lending protocol #737

daniel-savu · 2022-10-27T23:17:30Z

Adds lending functionality to the parachain, based on a fork of Parallel Finance that was adapted to have orml-token conforming lend-tokens. This enables usage of these lend-tokens as interbtc vault collateral.

Notes since the last review:

Adds comments to the convert function in the currency crate (to clarify how lend-token conversions are made)
Adds comments about why the MIN and MAX exchange rate boundaries are required
- The Rate type used by these fields is a FixedU128 which already implements Default, so no need to implement it
Use interlay assets in all loans tests
Removes all unused code and dependencies
Updates repo to latest master from the orml repo

Closes #740
Closes #741
Closes #742
Closes #752

Co-authored-by: Sander Bosma <sanderbosma@gmail.com>

…orml-tokens

and also distributes rewards based on pToken holdings instead of voucher balance

Lending Protocol

crates/loans/src/lib.rs

daniel-savu · 2022-10-28T17:12:38Z

crates/traits/Cargo.toml

+[package]
+authors = ["Parallel Team"]
+edition = "2021"
+name = "pallet-traits"


I've been thinking about moving the contents of this pallet into other pallets and remove pallet-traits, but since both the currency and loans pallets depend on it, I'm not sure it's possible.

It might be nice to merge this pallet yea, what are the dependencies exactly?

The main traits pallet_traits exports are LoansApi and OracleApi.

The currency pallet uses both traits to define the default CurrencyConversion trait implementation, so it depends on pallet_traits. The oracle pallet depends on currency. If I moved OracleApi to the oracle pallet then I'd get a circular dependency between currency and oracle.

The other solution would be to move the entire contents of pallet_traits to the currency pallet but this would add some responsibility creep to currency.

gregdhill

Seems like mostly non-invasive code changes to what was already reviewed so I'm mostly happy with this (see outstanding comments). My only gripe is the naming LendToken tracking the "Loans" pallet - I feel like LoanToken would make more sense but I'm not gonna block on this.

gregdhill · 2022-11-04T09:40:53Z

crates/loans/src/benchmarking.rs

-const CKSM: CurrencyId = PToken(3);
-const CKBTC: CurrencyId = PToken(4);
+const LEND_KSM: CurrencyId = LendToken(3);
+const LendKBTC: CurrencyId = LendToken(4);


Suggested change

const LendKBTC: CurrencyId = LendToken(4);

const Lend_KBTC: CurrencyId = LendToken(4);

gregdhill · 2022-11-04T09:48:56Z

crates/loans/src/lib.rs

+        if total_collateral_value > total_borrow_value {
+            Ok((total_collateral_value - total_borrow_value, FixedU128::zero()))
+        } else {
+            Ok((FixedU128::zero(), total_borrow_value - total_collateral_value))
        }


Probably would use checked or saturated math here

Done and I'll create an issue, there are other places without failsafe math too

gregdhill · 2022-11-04T09:49:16Z

crates/loans/src/lib.rs

+        if total_collateral_value > total_borrow_value {
+            Ok((total_collateral_value - total_borrow_value, FixedU128::zero()))
+        } else {
+            Ok((FixedU128::zero(), total_borrow_value - total_collateral_value))
        }


Also checked / saturated math here

gregdhill · 2022-11-04T09:53:57Z

crates/loans/src/lib.rs

-        let amount = Amount::new(orml_tokens::Pallet::<T>::free_balance(ptoken_id, account_id), ptoken_id);
+    /// Free lending tokens (lend_tokens) of an account, given the underlying
+    pub fn free_lend_tokens(asset_id: AssetIdOf<T>, account_id: &T::AccountId) -> Result<Amount<T>, DispatchError> {
+        let lend_token_id = Self::lend_token_id(asset_id)?;


Is it worth returning an error if asset_id is already a LendToken?

An error is returned if the asset_id is already a LendToken: Error::<T>::MarketDoesNotExist (as in, there is no market for lending out the given asset, so a LendToken for the asset_id doesn't exist). This happens in Self::lend_token_id(...)

gregdhill · 2022-11-04T09:56:52Z

crates/traits/Cargo.toml

+[package]
+authors = ["Parallel Team"]
+edition = "2021"
+name = "pallet-traits"


It might be nice to merge this pallet yea, what are the dependencies exactly?

daniel-savu · 2022-11-04T10:01:23Z

the naming LendToken tracking the "Loans" pallet - I feel like LoanToken would make more sense

@gregdhill My reasoning is that these tokens represent a lending position as opposed to a loan position

daniel-savu · 2022-11-04T12:59:49Z

see outstanding comments

@gregdhill If you mean outstanding comments in other PRs, all of those should be addressed by this PR

daniel-savu · 2022-11-04T13:10:41Z

crates/loans/src/tests/lend_tokens.rs

@@ -0,0 +1,237 @@
+use crate::{


This file is not new, it's just a rename of the previous ptokens.rs test file

sander2 · 2022-11-04T13:10:28Z

crates/loans/src/lib.rs

@@ -245,6 +241,8 @@ pub mod pallet {
        CodecError,
        /// Collateral is reserved and cannot be liquidated
        CollateralReserved,
+        /// Arithmetic underflow
+        ArithmeticUnderflow,


let's not redefine this, instead let's return ArithmeticError::Underflow as we do in other pallets

…icError

gregdhill · 2022-11-07T10:08:16Z

crates/currency/src/lib.rs

+            // get the equivalent lend_token amount using the internal exchange rate
+            Loans::recompute_collateral_amount(&underlying_amount)
+        } else {
+            // Exampe (DOT to INTR):


Suggested change

// Exampe (DOT to INTR):

// Example (DOT to INTR):

gregdhill · 2022-11-07T10:09:19Z

crates/currency/src/lib.rs

@@ -42,7 +83,9 @@ pub mod pallet {
    /// ## Configuration
    /// The pallet's configuration trait.
    #[pallet::config]
-    pub trait Config: frame_system::Config + orml_tokens::Config<Balance = BalanceOf<Self>> {
+    pub trait Config:
+        frame_system::Config + orml_tokens::Config<Balance = BalanceOf<Self>, CurrencyId = PrimitivesCurrencyId>


Let's not use the concrete type:

Suggested change

frame_system::Config + orml_tokens::Config<Balance = BalanceOf<Self>, CurrencyId = PrimitivesCurrencyId>

frame_system::Config + orml_tokens::Config<Balance = BalanceOf<Self>, CurrencyId = CurrencyId<Self>>

This somehow triggers a compiler bug: https://stackoverflow.com/questions/66631251/why-do-i-get-an-overflow-evaluating-the-requirement-error-for-a-simple-trait-i

error[E0275]: overflow evaluating the requirement `<T as orml_tokens::Config>::CurrencyId == _` --> crates/currency/src/amount.rs:18:46 | 18 | #[cfg_attr(feature = "testing-utils", derive(Copy))] | ^^^^ | = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)

I think it's because CurrencyId is defined like this, so there might be a reference loop?

pub type CurrencyId<T> = <T as orml_tokens::Config>::CurrencyId;

The reason I specified the concrete type in the first place was this error:

error[E0599]: no method named `is_lend_token` found for associated type `<T as orml_tokens::Config>::CurrencyId` in the current scope --> crates/currency/src/lib.rs:46:30 | 46 | if amount.currency().is_lend_token() && to.is_lend_token() { | ^^^^^^^^^^^^^ method not found in `<T as orml_tokens::Config>::CurrencyId`

gregdhill · 2022-11-07T10:13:52Z

crates/loans/rpc/src/lib.rs

+impl From<Error> for i32 {
+    fn from(e: Error) -> i32 {
+        match e {
+            Error::RuntimeError => 1,
+            Error::AccountLiquidityError => 2,
+            Error::MarketStatusError => 3,
+        }
+    }
+}


Why do we need this trait implementation?

It was there in the original implementation, they have these custom RPC errors. As far as I can tell, serializing for json-rpc requires this trait, and since we use JsonRpsee default errors, there's no need for us to implement the trait.

Example in their code, example in our code.

gregdhill · 2022-11-07T10:16:17Z

crates/loans/src/farming.rs

+    fn reward_scale() -> u128 {
+        10_u128.pow(12)
+    }


We should refactor this pallet to use FixedPoint math instead (to be consistent with the rest of our code), can you make a ticket?

crates/loans/src/farming.rs

gregdhill · 2022-11-07T10:24:59Z

crates/loans/src/lib.rs

+
+    #[pallet::event]
+    #[pallet::generate_deposit(pub (crate) fn deposit_event)]
+    pub enum Event<T: Config> {


I think we should switch to named events before this is live ideally to limit downstream (squid) changes

Created issue here: #757

gregdhill · 2022-11-07T10:28:56Z

parachain/runtime/testnet-kintsugi/src/lib.rs

+    type OnSlash = ();
+    type OnDeposit = ();
+    type OnTransfer = ();


Do these not need to be set?

yes they do... which is exactly why I need to add more integration tests. Seems like I had only added these to the mock runtime

gregdhill · 2022-11-07T10:33:30Z

crates/traits/src/lib.rs

+
+impl ConvertToBigUint for u128 {
+    fn get_big_uint(&self) -> BigUint {
+        self.to_biguint().unwrap()


Is this safe to do? When can to_biguint fail?

This snippet is unchanged from the original code. It can fail if the u128 doesn't fit inside a u64 during the conversion:

Step 1: https://docs.rs/num-bigint/0.4.0/src/num_bigint/biguint/convert.rs.html#571

Step 2: https://docs.rs/num-bigint/0.4.0/src/num_bigint/biguint/convert.rs.html#548

Step 3: https://docs.rs/num-traits/0.2.14/src/num_traits/cast.rs.html#498

When let's make it return Result, as we do for other type conversions?

gregdhill · 2022-11-07T10:42:37Z

primitives/src/lib.rs

+/// Loans pallet types
+
+pub type Price = FixedU128;
+pub type Timestamp = u64;


Suggested change

pub type Timestamp = u64;

pub type Timestamp = Moment;

gregdhill · 2022-11-07T10:46:55Z

crates/loans/src/interest.rs

+    fn accrued_interest(borrow_rate: Rate, amount: BalanceOf<T>, delta_time: Timestamp) -> Option<BalanceOf<T>> {
+        borrow_rate
+            .checked_mul_int(amount)?
+            .checked_mul(delta_time.into())?
+            .checked_div(SECONDS_PER_YEAR.into())
+    }
+
+    fn increment_index(borrow_rate: Rate, index: Rate, delta_time: Timestamp) -> Option<Rate> {
+        borrow_rate
+            .checked_mul(&index)?
+            .checked_mul(&FixedU128::saturating_from_integer(delta_time))?
+            .checked_div(&FixedU128::saturating_from_integer(SECONDS_PER_YEAR))
+    }


Why are we dividing by SECONDS_PER_YEAR? I can see these two functions are only called via RPC so I can only assume this is used to determine APY? Although I think there would be an implicit assumption that Timestamp is stored in seconds.

On a related note I can see that Substrate uses the "Julian" year, perhaps that is a standard we should consider adopting also elsewhere: https://github.com/paritytech/substrate/blob/ded44948e2d5a398abcb4e342b0513cb690961bb/frame/staking/src/inflation.rs#L42

This is how interest accrual was implemented by Parallel (the functions are used everywhere, not just in RPCs), but ultimately it doesn't matter that much. The interest rate model that decides interest rates probably also assumes a fixed-length year. We should double-check but it might actually simplify things.

Created a nice-to-have issue: #758

…e comments

daniel-savu and others added 30 commits September 19, 2022 18:02

feat(lend): add lending protocol crates

0224470

feat(lend): get unit tests to run

30100fe

feat(lend): use orml-tokens instead of pallet-assets

9122745

feat(lend): configure lending pallet

d1be241

chore: bump toolchain to match lending pallet

66b4572

chore: fix toml files

d45d68d

Co-authored-by: Sander Bosma <sanderbosma@gmail.com>

fix(lend): PriceFeeder trait import

8ad2319

feat(lend): add loans pallet to standalone runtime

64af00e

chore(lend): remove liquidate-free collateral

89a6032

fix(lend): btc oracle integration, notes on integrating pTokens with …

590a405

…orml-tokens

refactor(lend): cleanup

cf8a1b0

feat(lend): update benchmarks and copyrights

7c41e20

fix(lend): attempt to fix benchmarks

a70aa8e

fix(lend): fmt

81d925a

fix(lend): cleanup

50fd3a7

feat(loans): import orml pallet with hooks feature

bca36ff

feat(loans): add currency pallet

32c204e

feat(loans): split do_collateral_asset

409bb8b

and also distributes rewards based on pToken holdings instead of voucher balance

Merge pull request #1 from interlay/dan/lend

1d509e9

Lending Protocol

feat(loans): fully tokenize ptokens

139a314

feat(loans): Replace T::Asset actions with Currency pallet

9ec2ed0

feat(loans): clean up ptoken module

7fd05bb

feat(loans): replace local orml-tokens fork with interlay repo

25bec4e

Merge branch 'master' into dan/orml-ptoken

481957b

fix(loans): fix all local orml imports

1b6d0b2

fix(loans): benchmark

a4331b2

chore(loans): cleanup and added comments

815c53f

fix(loans): don't propagate on_slash errors; feature-gate test utils

4b94319

fix(loans): use a weghts-v1 compatible orml dependency

e3215ef

feat(loans): ptokens as currency id

e18c703

daniel-savu commented Oct 28, 2022

View reviewed changes

crates/loans/src/lib.rs Show resolved Hide resolved

daniel-savu added 3 commits October 28, 2022 14:16

fix(loans): reset storage version

f40383c

test(loans): zero amount extrinsics

c089ac3

test(loans): improve comments for tests.rs

df3a2d9

daniel-savu commented Oct 28, 2022

View reviewed changes

daniel-savu requested a review from sander2 October 31, 2022 10:01

daniel-savu added 2 commits October 31, 2022 16:59

chore(loans): remove comment typo

d8a6824

fix(loans): failing test and warnings

ca6eccf

gregdhill reviewed Nov 4, 2022

View reviewed changes

feat(loans): calculate liquidity with checked math

acc03a7

daniel-savu commented Nov 4, 2022

View reviewed changes

sander2 requested changes Nov 4, 2022

View reviewed changes

fix(loans): add missing copyright and authorship acks; reuse Arithmet…

6f88113

…icError

sander2 previously approved these changes Nov 4, 2022

View reviewed changes

gregdhill requested changes Nov 7, 2022

View reviewed changes

gregdhill assigned daniel-savu Nov 7, 2022

gregdhill changed the title ~~InterLend~~ Lending protocol Nov 7, 2022

daniel-savu mentioned this pull request Nov 8, 2022

Use FixedPoint arithmetic in the loans pallet #755

Closed

daniel-savu dismissed sander2’s stale review via 07ba329 November 8, 2022 12:52

fix(loans): use currency hooks on testnet; refactor constants; improv…

13e038e

…e comments

daniel-savu force-pushed the dan/interbtc-repo-sync branch from 07ba329 to 13e038e Compare November 8, 2022 13:16

daniel-savu requested a review from gregdhill November 8, 2022 13:33

gregdhill approved these changes Nov 10, 2022

View reviewed changes

sander2 approved these changes Nov 11, 2022

View reviewed changes

gregdhill merged commit 4d6d1b3 into master Nov 11, 2022

gregdhill deleted the dan/interbtc-repo-sync branch November 11, 2022 10:43

sander2 added this to the Release 1.20 milestone Dec 7, 2022

nud3l mentioned this pull request Aug 14, 2023

qToken Vaults #1174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lending protocol #737

Lending protocol #737

daniel-savu commented Oct 27, 2022 •

edited

Loading

daniel-savu Oct 28, 2022

gregdhill Nov 4, 2022

daniel-savu Nov 4, 2022

gregdhill left a comment

gregdhill Nov 4, 2022

gregdhill Nov 4, 2022

daniel-savu Nov 4, 2022

gregdhill Nov 4, 2022

gregdhill Nov 4, 2022

daniel-savu Nov 4, 2022 •

edited

Loading

gregdhill Nov 4, 2022

daniel-savu commented Nov 4, 2022 •

edited

Loading

daniel-savu commented Nov 4, 2022

daniel-savu Nov 4, 2022

sander2 Nov 4, 2022

gregdhill Nov 7, 2022

gregdhill Nov 7, 2022

daniel-savu Nov 8, 2022

daniel-savu Nov 8, 2022

daniel-savu Nov 8, 2022

gregdhill Nov 7, 2022

daniel-savu Nov 8, 2022

gregdhill Nov 7, 2022

gregdhill Nov 7, 2022

daniel-savu Nov 8, 2022 •

edited

Loading

gregdhill Nov 7, 2022

daniel-savu Nov 7, 2022

gregdhill Nov 7, 2022

daniel-savu Nov 8, 2022

sander2 Nov 11, 2022

gregdhill Nov 7, 2022

gregdhill Nov 7, 2022

daniel-savu Nov 8, 2022 •

edited

Loading

daniel-savu Nov 8, 2022

	const LendKBTC: CurrencyId = LendToken(4);
	const Lend_KBTC: CurrencyId = LendToken(4);

	frame_system::Config + orml_tokens::Config<Balance = BalanceOf<Self>, CurrencyId = PrimitivesCurrencyId>
	frame_system::Config + orml_tokens::Config<Balance = BalanceOf<Self>, CurrencyId = CurrencyId<Self>>

Lending protocol #737

Lending protocol #737

Conversation

daniel-savu commented Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregdhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-savu Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-savu commented Nov 4, 2022 • edited Loading

daniel-savu commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-savu Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-savu Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-savu commented Oct 27, 2022 •

edited

Loading

daniel-savu Nov 4, 2022 •

edited

Loading

daniel-savu commented Nov 4, 2022 •

edited

Loading

daniel-savu Nov 8, 2022 •

edited

Loading

daniel-savu Nov 8, 2022 •

edited

Loading