Merge pull request #904 from daniel-savu/round-up-total-borrows

fix(loans): Use compound interest formula

crates/loans/src/interest.rs

@@ -72,7 +72,8 @@ impl<T: Config> Pallet<T> {
         let total_cash = Self::get_total_cash(asset_id);
         let mut total_borrows = Self::total_borrows(asset_id);
         let mut total_reserves = Self::total_reserves(asset_id);
-        let mut borrow_index = Self::borrow_index(asset_id);
+        let borrow_index = Self::borrow_index(asset_id);
+        let mut borrow_index_new = borrow_index;
 
         let util = Self::calc_utilization_ratio(&total_cash, &total_borrows, &total_reserves)?;
         let borrow_rate = market
@@ -87,15 +88,24 @@ impl<T: Config> Pallet<T> {
             let delta_time = now
                 .checked_sub(last_accrued_interest_time)
                 .ok_or(ArithmeticError::Underflow)?;
-            let interest_accumulated = Self::accrued_interest(borrow_rate, &total_borrows, delta_time)?;
-            total_borrows = interest_accumulated.checked_add(&total_borrows)?;
-            total_reserves = interest_accumulated
+            borrow_index_new = Self::accrue_index(borrow_rate, borrow_index, delta_time)?;
+            let total_borrows_old = total_borrows.clone();
+            // Round `total_borrows` down because it needs to be less than or equal to the sum of
+            // `current_borrow_balance` to compute lend token exchange rate correctly. If `total_borrows`
+            // is too big, the exchange rate will also be big and there may not be enough cash in the pallet
+            // account to allow valid redeems.
+            // Due to this rounding error, the lend token exchange rate may increase after a repayment
+            // because the pallet account's cash balance could increase by more than `total_borrows`
+            total_borrows = Self::borrow_balance_from_old_and_new_index(
+                &borrow_index,
+                &borrow_index_new,
+                total_borrows,
+                RoundingMode::Down,
+            )?;
+            let interest_accummulated = total_borrows.checked_sub(&total_borrows_old)?;
+            total_reserves = interest_accummulated
                 .map(|x| market.reserve_factor.mul_floor(x))
                 .checked_add(&total_reserves)?;
-
-            borrow_index = Self::increment_index(borrow_rate, borrow_index, delta_time)
-                .and_then(|r| r.checked_add(&borrow_index))
-                .ok_or(ArithmeticError::Overflow)?;
         }
 
         let exchange_rate = Self::calculate_exchange_rate(&total_supply, &total_cash, &total_borrows, &total_reserves)?;
@@ -107,7 +117,7 @@ impl<T: Config> Pallet<T> {
             util,
             total_borrows.amount(),
             total_reserves.amount(),
-            borrow_index,
+            borrow_index_new,
         ))
     }
 
@@ -161,26 +171,17 @@ impl<T: Config> Pallet<T> {
         })
     }
 
-    fn accrued_interest(
-        borrow_rate: Rate,
-        amount: &Amount<T>,
-        delta_time: Timestamp,
-    ) -> Result<Amount<T>, DispatchError> {
-        let balance = borrow_rate
-            .checked_mul_int(amount.amount())
-            .ok_or(ArithmeticError::Overflow)?
-            .checked_mul(delta_time.into())
-            .ok_or(ArithmeticError::Overflow)?
-            .checked_div(SECONDS_PER_YEAR.into())
-            .ok_or(ArithmeticError::Underflow)?;
-        Ok(Amount::new(balance, amount.currency()))
-    }
-
-    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))?
+    pub(crate) fn accrue_index(borrow_rate: Rate, index: Rate, delta_time: Timestamp) -> Result<Rate, DispatchError> {
+        // Compound interest:
+        // new_index = old_index * (1 + annual_borrow_rate / SECONDS_PER_YEAR) ^ delta_time
+        let rate_fraction = borrow_rate
             .checked_div(&FixedU128::saturating_from_integer(SECONDS_PER_YEAR))
+            .ok_or(ArithmeticError::Underflow)?;
+        let base_rate = Rate::one()
+            .checked_add(&rate_fraction)
+            .ok_or(ArithmeticError::Overflow)?;
+        let compounded_rate = base_rate.saturating_pow(usize::saturated_from(delta_time));
+        Ok(index.checked_mul(&compounded_rate).ok_or(ArithmeticError::Overflow)?)
     }
 
     fn calculate_exchange_rate(

crates/loans/src/lib.rs

@@ -57,7 +57,7 @@ use traits::{
 
 pub use default_weights::WeightInfo;
 pub use orml_traits::currency::{OnDeposit, OnSlash, OnTransfer};
-pub use types::{BorrowSnapshot, EarnedSnapshot, Market, MarketState, RewardMarketState};
+pub use types::{BorrowSnapshot, EarnedSnapshot, Market, MarketState, RewardMarketState, RoundingMode};
 
 #[cfg(feature = "runtime-benchmarks")]
 mod benchmarking;
@@ -1525,11 +1525,8 @@ impl<T: Config> Pallet<T> {
 
         let account_borrows_new = account_borrows.checked_sub(&repay_amount)?;
         let total_borrows = Self::total_borrows(asset_id);
-        // NOTE: `total_borrows()` uses `u128` to calculate accrued interest,
-        // while `current_borrow_balance()` uses a `FixedU128` (the `BorrowSnapshot`) to calculate accrued interest.
-        // As a result, when a user repays all borrows, `total_borrows` may be less than `account_borrows`
-        // due to rounding, which would cause a `checked_sub` to fail with `ArithmeticError::Underflow`.
-        // Use `saturating_sub` instead here:
+        // Use `saturating_sub` here, because it's intended for `total_borrows` to be rounded down,
+        // such that it is less than or equal to the actual borrower debt.
         let total_borrows_new = total_borrows.saturating_sub(&repay_amount)?;
         AccountBorrows::<T>::insert(
             asset_id,
@@ -1551,16 +1548,35 @@ impl<T: Config> Pallet<T> {
         if snapshot.principal.is_zero() || snapshot.borrow_index.is_zero() {
             return Ok(Amount::zero(asset_id));
         }
-        // Calculate new borrow balance using the interest index:
-        // recent_borrow_balance = snapshot.principal * borrow_index / snapshot.borrow_index
         let principal_amount = Amount::<T>::new(snapshot.principal, asset_id);
-        let borrow_index_increase = Self::borrow_index(asset_id)
-            .checked_div(&snapshot.borrow_index)
-            .ok_or(ArithmeticError::Underflow)?;
-        // Round up the borrower's debt, to avoid giving out short-term interest-free loans.
-        let recent_borrow_balance = principal_amount.checked_fixed_point_mul_rounded_up(&borrow_index_increase)?;
+        // Round borrower debt up to avoid interest-free loans
+        Self::borrow_balance_from_old_and_new_index(
+            &snapshot.borrow_index,
+            &Self::borrow_index(asset_id),
+            principal_amount,
+            RoundingMode::Up,
+        )
+    }
 
-        Ok(recent_borrow_balance)
+    pub fn borrow_balance_from_old_and_new_index(
+        old_index: &FixedU128,
+        new_index: &FixedU128,
+        amount: Amount<T>,
+        rounding_mode: RoundingMode,
+    ) -> Result<Amount<T>, DispatchError> {
+        // Calculate new borrow balance using the interest index:
+        // recent_borrow_balance = snapshot.principal * borrow_index / snapshot.borrow_index
+        let borrow_index_increase = new_index.checked_div(&old_index).ok_or(ArithmeticError::Underflow)?;
+        let borrow_balance = match rounding_mode {
+            RoundingMode::Up => amount.checked_fixed_point_mul_rounded_up(&borrow_index_increase)?,
+            RoundingMode::Down => {
+                let balance_u128 = borrow_index_increase
+                    .checked_mul_int(amount.amount())
+                    .ok_or(ArithmeticError::Overflow)?;
+                Amount::<T>::new(balance_u128, amount.currency())
+            }
+        };
+        Ok(borrow_balance)
     }
 
     /// Checks if the liquidation should be allowed to occur

crates/loans/src/tests/edge_cases.rs

@@ -34,23 +34,24 @@ fn repay_borrow_all_no_underflow() {
         assert_ok!(Loans::deposit_all_collateral(RuntimeOrigin::signed(ALICE), Token(KSM)));
 
         // Alice borrow only 1/1e5 KSM which is hard to accrue total borrows interest in 100 seconds
-        assert_ok!(Loans::borrow(RuntimeOrigin::signed(ALICE), Token(KSM), 10_u128.pow(7)));
+        assert_ok!(Loans::borrow(RuntimeOrigin::signed(ALICE), Token(KSM), 10_u128.pow(5)));
 
-        accrue_interest_per_block(Token(KSM), 100, 9);
+        accrue_interest_per_block(Token(KSM), 100, 1000);
 
         assert_eq!(
             Loans::current_borrow_balance(&ALICE, Token(KSM)).unwrap().amount(),
-            10000006
+            100007
         );
-        // FIXME since total_borrows is too small and we accrue internal on it every 100 seconds
-        // accrue_interest fails every time
-        // as you can see the current borrow balance is not equal to total_borrows anymore
-        assert_eq!(Loans::total_borrows(Token(KSM)).amount(), 10000000);
+        // `total_borrows` had its interest rounded down to zero each block because the principal
+        // amount is too small
+        assert_eq!(Loans::total_borrows(Token(KSM)).amount(), 100000);
 
-        // Alice repay all borrow balance. total_borrows = total_borrows.saturating_sub(10000006) = 0.
+        // Alice repay all borrow balance. total_borrows = total_borrows.saturating_sub(100007) = 0.
+        // Repaying should not cause an underflow
         assert_ok!(Loans::repay_borrow_all(RuntimeOrigin::signed(ALICE), Token(KSM)));
+        assert_eq!(Loans::total_borrows(Token(KSM)).amount(), 0);
 
-        assert_eq!(Tokens::balance(Token(KSM), &ALICE), unit(800) - 6);
+        assert_eq!(Tokens::balance(Token(KSM), &ALICE), unit(800) - 7);
 
         assert_eq!(
             Loans::exchange_rate(Token(DOT)).saturating_mul_int(

crates/loans/src/tests/interest_rate.rs

@@ -2,9 +2,9 @@ use crate::{mock::*, tests::Loans, Markets};
 use currency::Amount;
 use frame_support::assert_ok;
 use mocktopus::mocking::Mockable;
-use primitives::{CurrencyId::Token, Rate, Ratio, DOT, KSM, SECONDS_PER_YEAR};
+use primitives::{CurrencyId::Token, Moment, Rate, Ratio, DOT, KSM};
 use sp_runtime::{
-    traits::{CheckedDiv, One, Saturating},
+    traits::{CheckedDiv, One},
     FixedPointNumber,
 };
 use traits::OracleApi;
@@ -84,13 +84,13 @@ fn interest_rate_model_works() {
             assert_eq!(Loans::utilization_ratio(Token(DOT)), util_ratio);
 
             let borrow_rate = (jump_rate - base_rate) * util_ratio.into() / jump_utilization.into() + base_rate;
-            let interest_accumulated: u128 = borrow_rate
-                .saturating_mul_int(total_borrows)
-                .saturating_mul(delta_time.into())
-                .checked_div(SECONDS_PER_YEAR.into())
-                .unwrap();
-            total_borrows = interest_accumulated + total_borrows;
-            assert_eq!(Loans::total_borrows(Token(DOT)).amount(), total_borrows);
+            let borrow_index_old = borrow_index;
+            borrow_index = Loans::accrue_index(borrow_rate, borrow_index, delta_time as Moment).unwrap();
+            let total_borrows_old = total_borrows;
+            total_borrows = (borrow_index / borrow_index_old).saturating_mul_int(total_borrows);
+            let interest_accumulated = total_borrows - total_borrows_old;
+            let actual_total_borrows = Loans::total_borrows(Token(DOT)).amount();
+            assert_eq!(actual_total_borrows, total_borrows);
             total_reserves = Markets::<Test>::get(&Token(DOT))
                 .unwrap()
                 .reserve_factor
@@ -103,26 +103,20 @@ fn interest_rate_model_works() {
                 Loans::exchange_rate(Token(DOT)).into_inner(),
                 (total_cash + total_borrows - total_reserves) * rate_decimal / total_supply
             );
-            let numerator = borrow_index
-                .saturating_mul(borrow_rate)
-                .saturating_mul(Rate::saturating_from_integer(delta_time))
-                .checked_div(&Rate::saturating_from_integer(SECONDS_PER_YEAR))
-                .unwrap();
-            borrow_index = numerator + borrow_index;
             assert_eq!(Loans::borrow_index(Token(DOT)), borrow_index);
         }
-        assert_eq!(total_borrows, 100000063926960646826);
-        assert_eq!(total_reserves, 9589044097001);
-        assert_eq!(borrow_index, Rate::from_inner(1000000639269606444));
-        assert_eq!(Loans::exchange_rate(Token(DOT)), Rate::from_inner(20000005433791654));
+        assert_eq!(total_borrows, 100000063926960953257);
+        assert_eq!(total_reserves, 9589044142967);
+        assert_eq!(borrow_index, Rate::from_inner(1000000639269609557));
+        assert_eq!(Loans::exchange_rate(Token(DOT)), Rate::from_inner(20000005433791681));
 
         // Calculate borrow accrued interest
         let borrow_principal =
             (borrow_index / borrow_snapshot.borrow_index).saturating_mul_int(borrow_snapshot.principal);
-        // TODO: Why subtract `million_unit(200)` here? Accruing interest doesn't fix this.
+        // The supply interest here is probably the fraction of interest that goes to the reserve
         let supply_interest = Loans::exchange_rate(Token(DOT)).saturating_mul_int(total_supply) - million_unit(200);
-        assert_eq!(supply_interest, 54337916540000);
-        assert_eq!(borrow_principal, 100000063926960644400);
+        assert_eq!(supply_interest, 54337916810000);
+        assert_eq!(borrow_principal, 100000063926960955700);
         assert_eq!(total_borrows / 10000, borrow_principal / 10000);
         assert_eq!(
             (total_borrows - million_unit(100) - total_reserves) / 10000,
@@ -143,7 +137,7 @@ fn last_accrued_interest_time_should_be_update_correctly() {
         assert_eq!(Loans::borrow_index(Token(DOT)), Rate::one());
         TimestampPallet::set_timestamp(12000);
         assert_ok!(Loans::mint(RuntimeOrigin::signed(ALICE), Token(DOT), unit(100)));
-        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112633),);
+        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112698),);
     })
 }
 
@@ -156,7 +150,7 @@ fn accrue_interest_works_after_mint() {
         assert_eq!(Loans::borrow_index(Token(DOT)), Rate::one());
         TimestampPallet::set_timestamp(12000);
         assert_ok!(Loans::mint(RuntimeOrigin::signed(ALICE), Token(DOT), unit(100)));
-        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112633),);
+        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000013318112698),);
     })
 }
 
@@ -187,7 +181,7 @@ fn accrue_interest_works_after_redeem() {
             Token(DOT),
             amount_to_redeem
         ));
-        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004756468797),);
+        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004756468801),);
         assert_eq!(
             Loans::exchange_rate(Token(DOT))
                 .saturating_mul_int(Loans::account_deposits(Loans::lend_token_id(Token(DOT)).unwrap(), &BOB).amount()),
@@ -209,7 +203,7 @@ fn accrue_interest_works_after_redeem_all() {
         assert_eq!(Tokens::balance(Token(DOT), &BOB), 980000000000000);
         TimestampPallet::set_timestamp(12000);
         assert_ok!(Loans::redeem_all(RuntimeOrigin::signed(BOB), Token(DOT)));
-        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004669977168),);
+        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000004669977174),);
         assert_eq!(
             Loans::exchange_rate(Token(DOT))
                 .saturating_mul_int(Loans::account_deposits(Loans::lend_token_id(Token(DOT)).unwrap(), &BOB).amount()),
@@ -231,7 +225,7 @@ fn accrue_interest_works_after_repay() {
         assert_eq!(Loans::borrow_index(Token(DOT)), Rate::one());
         TimestampPallet::set_timestamp(12000);
         assert_ok!(Loans::repay_borrow(RuntimeOrigin::signed(ALICE), Token(DOT), unit(10)));
-        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000005707762557),);
+        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000005707762564),);
     })
 }
 
@@ -245,7 +239,7 @@ fn accrue_interest_works_after_repay_all() {
         assert_eq!(Loans::borrow_index(Token(KSM)), Rate::one());
         TimestampPallet::set_timestamp(12000);
         assert_ok!(Loans::repay_borrow_all(RuntimeOrigin::signed(ALICE), Token(KSM)));
-        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643835),);
+        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643864),);
         assert_eq!(Tokens::balance(Token(KSM), &ALICE), 999999999571917);
         let borrow_snapshot = Loans::account_borrows(Token(KSM), ALICE);
         assert_eq!(borrow_snapshot.principal, 0);
@@ -277,8 +271,8 @@ fn accrue_interest_works_after_liquidate_borrow() {
             unit(50),
             Token(DOT)
         ));
-        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000013318112633),);
-        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000006976141552),);
+        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000013318112698),);
+        assert_eq!(Loans::borrow_index(Token(DOT)), Rate::from_inner(1000000006976141566),);
     })
 }
 
@@ -295,7 +289,7 @@ fn accrue_interest_works_after_recompute_collateral_amount() {
             1234,
             Token(KSM)
         )));
-        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643835),);
+        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643864),);
     })
 }
 
@@ -311,7 +305,7 @@ fn accrue_interest_works_after_recompute_underlying_amount() {
         assert_ok!(Loans::recompute_underlying_amount(
             &Loans::free_lend_tokens(Token(KSM), &ALICE).unwrap()
         ));
-        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643835),);
+        assert_eq!(Loans::borrow_index(Token(KSM)), Rate::from_inner(1000000008561643864),);
     })
 }
 

crates/loans/src/types.rs

@@ -14,6 +14,12 @@ pub enum AccountLiquidity<T: Config> {
     Shortfall(Amount<T>),
 }
 
+#[derive(Eq, PartialEq, Clone, RuntimeDebug)]
+pub enum RoundingMode {
+    Up,
+    Down,
+}
+
 impl<T: Config> AccountLiquidity<T> {
     pub fn from_collateral_and_debt(
         collateral_value: Amount<T>,