Apply and mint interest #85

bingen · 2024-03-27T09:58:37Z

Rebase of branch from #76 on top of branch from #81

We now have 3 aggregate trackers on the ActivePool, as per the simple compounding specification:

aggRecordedDebt (D)
recordedDebtSum (G)
weightedRecordedDebtSum (S)

G is the original ActivePool debt tracker from Liquity v1. We keep it for now as a) it may be useful for computing a size-weighted average interest rate on the front end, and b) would be needed if we decide to add an emergency base interest rate to every position.

Pseudocode for aggregate trackers in different operations

As in v1, we always apply any redistribution debt gain before handling interest accounting.
The redistribution gain is already included in G before we alter G from the specific operation.

The prefix “old” means the respective debt with no redistribution gains or interest applied, and “new” implies all debt changes (redist. gains, interest, debt drawn/repaid) are included.

openTrove:

D += agg_accrued_interest + trove_debt
S += weighted_trove_debt 
G += trove_debt

closeTrove:

D += (agg_accrued_interest - old_recorded_trove_debt - accrued_trove_interest) // Don’t remove redist. gain here since it was not already applied to D
S -= old_weighted_trove_debt
G -= (old_recorded_trove_debt + redistribution_gain)  // remove redist.gain here since it was already applied to G.

adjustTrove:

D+= agg_accrued_interest +trove_debt_change + redist_debt_gain
S+= (new_weighted_trove_debt - old_weighted_trove_debt)
G+= (new_recorded_trove_debt - old_recorded_trove_debt)

And for the individual Trove:
new_recorded_trove_debt = old_recorded_trove_debt + accrued_trove_interest + trove_debt_change

applyTroveInterestPermissionless:

D += agg_accrued_interest
S += (new_weighted_trove_debt - old_weighted_trove_debt)
G += (new_recorded_trove_debt - old_recorded_trove_debt)

adjustTroveInterestRate:

D+= agg_accrued_interest
S += (new_weighted_trove_debt - old_weighted_trove_debt)
G += (new_recorded_trove_debt - old_recorded_trove_debt)

SP deposit/withdrawal:
D += agg_accrued_interest

Liquidation - Normal Mode, offset

This is effectively the same as closing the Trove:

D+= agg_accrued_interest - old_recorded_trove_debt - accrued_trove_interest
S -= old_weighted_trove_debt
G -= (old_recorded_trove_debt + redist_debt_gain)

Liquidation - Normal Mode, redistribution:

This is effectively the same as closing the Trove, except we add the entire debt to the DefaultPool:

D+= (agg_accrued_interest - old_recorded_trove_debt - accrued_trove_interest)
S -= old_weighted_trove_debt
G -= (old_recorded_trove_debt + redist_debt_gain)

Also:

DefaultPool.debt += (old_recorded_trove_debt + redistribution_gain + accrued_trove_interest)

TODOs:

** Liquidations in Recovery Mode**

Minting/applying interest has been implemented for RM liquidations to make tests pass, but if we keep RM we should double-check that minting/applying interest is correct for for every logical case of RM.

Redemption:

This is effectively a downward debt adjustment (since we will no longer close Troves on redemption):

D+= agg_accrued_interest - redeemed_debt + redist_debt_gain
S+= (new_weighted_recorded_debt - old_weighted_recorded_debt)
G+= (new_recorded_trove_debt - old_recorded_trove_debt)

This will be implemented in combination with #55 and #48, since these significantly change redemption logic anyway.

Testing TODOs:

more thorough invariant test (i.e. D + S*delta_t == sum_individual_entire_trove_debts)

…rove

Rebasing apply_and_mint_interest on top of trove_ERC721.

bingen

My first batch of comments. Mostly I think I would try to refactor a bit, so that we unify calls to the ActivePool and consolidate all the logic there, but not sure if that’s possible, or if the result would have a lot of if-else that would make things worse.

bingen · 2024-03-28T08:22:58Z

contracts/src/ActivePool.sol

+    // It does *not* include the Trove's individual accrued interest - this gets accounted for in the aggregate accrued interest.
+    // The net Trove debt change could be positive or negative in a repayment (depending on whether its redistribution gain or repayment amount is larger),
+    // so this function accepts both the increase and the decrease to avoid using (and converting to/from) signed ints.
+    function mintAggInterest(uint256 _troveDebtIncrease, uint256 _troveDebtDecrease) public {


I think it would be a good idea to merge this function with the ones for G (increase/decrease recorded debt).
At least the external end point (we can keep internally the functions as they are). For 2 reasons:

Gas efficiency (only 1 call)

Less prone to error, to make sure both trackers are consistent.
I wonder if we could even have S there too.

As I see there are calls with (0,0) as argument, maybe we could have a public function mintAggInterestNoDebtChange that would be called by the more generic mintAggInterest which also calls the increaseBoldDebt and decreaseBoldDebt.
This generic one could also accept the accrued interest as a parameter, to update S if non-zero.

I've refactored the D, G and S updates into a single call in this PR. Let me know what you think. It's still a bit unintuitive. This should get simpler if/when we optimize redistribution gains - then we can just update each tracker once with all the changes (debt change, accrued interest, pending gains).

bingen · 2024-03-28T08:23:34Z

contracts/src/BorrowerOperations.sol

-
-        vars.BoldFee;
-        vars.netDebt = _boldAmount;
+        vars.BoldFee; // TODO


This can be removed, right? Or is it for the initial period pre-payment?

bingen · 2024-03-28T08:24:26Z

contracts/src/BorrowerOperations.sol

        }

+        // --- Effects & interactions ---
+
+        contractsCache.activePool.mintAggInterest(vars.compositeDebt, 0);


Even if we don’t merge the endpoints, I would put the 3 calls to ActivePool together in this function.

bingen

Some more minor comments and doubts I had left for TroveManager.

bingen · 2024-04-03T14:34:19Z