Add pool fee during interest payments #50
Conversation
| IPool(_pool).manager(), | ||
| poolFee | ||
| ); | ||
| LoanLib.completePayment(liquidityAsset, _pool, poolPayment); |
There was a problem hiding this comment.
this looks like you could optimize and combine the two function calls into one, but as you'll see later when paying off a loan we'll want to add in the remaining principal...
| @@ -210,4 +212,56 @@ library LoanLib { | |||
| IERC20(liquidityAsset).safeTransferFrom(msg.sender, pool, amount); | |||
| emit LoanPaymentMade(pool, liquidityAsset, amount); | |||
There was a problem hiding this comment.
This PR doesn't change this event, but I think it's semantically different now. Trying to reason about it, but assuming people want to monitor payments made by borrowers, it should likely be one event per payment and the amount should likely be the total amount (with these changes, it no longer includes fees). We could add more granular events for fees, but I don't think it's necessary.
| /** | ||
| * @dev The address of the first loss vault | ||
| */ | ||
| function firstLossVault() external view override returns (address) { |
There was a problem hiding this comment.
Do we also need to expose feevault too?
There was a problem hiding this comment.
You're right. I updated the PR and added the feeVault and pool fees now go there instead of directly to the Pool Admin.
contracts/ServiceConfiguration.sol
Outdated
| @@ -21,6 +21,10 @@ contract ServiceConfiguration is AccessControl, IServiceConfiguration { | |||
|
|
|||
| mapping(address => bool) public isLiquidityAsset; | |||
|
|
|||
| uint256 public firstLossFee = 500; | |||
There was a problem hiding this comment.
I think the withdrawal fees were denominated in bips, which was included in the variable name too. I kinda appreciated that explicitness: https://github.com/circlefin/valyria-core/blob/64dd99da802fc52f8b1a89d01775a1dfc7e3c6f5/contracts/interfaces/IPool.sol#L30
| */ | ||
| function previewFees( | ||
| uint256 payment, | ||
| uint256 firstLoss, |
There was a problem hiding this comment.
Should this be firstLossFee or firstLossFeeBips so it's clearer? And then later on it's firstLossFeeAmount etc.
There was a problem hiding this comment.
haha I just dropped the word fee from the config but then realized the pool fee didn't have fee at the end... I actually don't have a good answer for this one. Maybe we can append Config to the params? That'd at least make it clear it is the input versus the output?
There was a problem hiding this comment.
Sounds good to me...as long as it's not ambiguous
Draft PR: Need to improve the code organization and whatnot, but basic functionality is there.