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

VAL-50 (Part 1) Tighten loan <> pool interactions to fix accounting gaps #98

Closed
wants to merge 4 commits into from
Closed

Conversation

ams9198
Copy link
Contributor

@ams9198 ams9198 commented Nov 22, 2022

The main issue here was that open-term loan principals could be returned to the pool through reclaimFunds, which wasn't updating the pool's outstandingLoanPrincipal value that's used in its exchange rate calculations.

This led me down a rabbit whole of trying to fix up the notifyLoanPrincipalReturned method (which authenticating calls through seeing if the loan was active) and in general the coupling between loans and pools. I started by writing a few new e2e tests under scenarios/pool/accounting.

This is what I landed on, but definitely open to feedback:

In the Pool, there are now 2 data structures for tracking loans:

  • fundedLoans (a mapping, that's "append" only) - as new loans are funded, they get added here. Checks against this datastructure authenticate loans calling into the notifyLoanPrincipalReturned method.
  • activeLoans (an enumerable set) - , same as before, used for iterating over active (i.e. drawndown) loans. But now this structure is only for ACTIVE loans, not funded loans.

The flow of funds in open-term loans is preserved, but now we notify the pool when principal is returned through reclaimFunds, and also update the pool through notifyLoanStateUpdated which adds and removes loans to the _activeLoans set.

This felt more explicit and like a looser coupling, but adds some code....so open to feedback.


uint256 firstLossBalance = IERC20(asset).balanceOf(
address(firstLossVault)
);

// TODO - handle open-term loans where principal may
// not be fully oustanding.
uint256 outstandingLoanDebt = ILoan(loan).principal() +
uint256 outstandingLoanDebt = ILoan(loan).outstandingPrincipal() +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still incomplete -- what would be the cleanest is if the loan could expose a variable that's like: debt still owed to the pool, inclusive of fees. @bricestacey for your thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

I originally thought that if a borrower were to go into default, the minutia of fees would be unimportant. Should the logic be basically the same as if paying off the entirety of the loan at that time ignoring fees?

If so, to rephrase this --

For open-term loans, the outstanding loan debt would be the principal + prorated interest.
For fixed term loans, the outstanding loan debt would be the principal + all outstanding interest as if held to maturity.

All fees are deducted from payments except the origination fee.

First-loss fee can probably be ignored here because FL is paying it anyway
Service fee can likely be ignored as FL funds likely shouldn't go to the pool admin, right?
Same for origination fee

What really matters is the outstanding principal and interest, not fees. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be easier to chat IRL in standup tomorrow, but in the simple case of a fixed term loan of 1000 principal, with 1 outstanding payment of 100, with 10 going to FL, and 10 going as service fees, I'd assume that the first-loss transferred to the pool in a default would be 1000 + 80.

My assumption was that first-loss was intended to cover pool losses (1080 in this case), so we'd want to carve out exactly what the pool would've received from outstanding interest payments + returned principals. Including the FL fee would kinda "double-pay" it, in a sense

Of course, this is just ultimately up to what the protocol decides "first loss" means and how it's used, not sure there's a "right" way...

@ams9198
Copy link
Contributor Author

ams9198 commented Nov 28, 2022

Marking as in progress since the latest changes to master require re-working this a bit.

Copy link
Contributor

@bricestacey bricestacey left a comment

Choose a reason for hiding this comment

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

I think overall this PR is good to go, but I think needs more clarity around how first-loss funds are used.

@bricestacey
Copy link
Contributor

Whoops, missed the last comment.

@ams9198
Copy link
Contributor Author

ams9198 commented Nov 29, 2022

Going to close and re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants