-
Notifications
You must be signed in to change notification settings - Fork 2
Start on insurance fund implementation #75
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Bastian Müller <bastian@turbolent.com>
|
Tracked by Issue: #84 |
| debitIncome = totalDebitBalance * debitRate | ||
| insuranceAmount = totalCreditBalance * insuranceRate | ||
| creditRate = (debitIncome - insuranceAmount) / totalCreditBalance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More clearly:
creditRate = debitRate * (totalDebitBalance / totalCreditBalance) - insuranceRate
Co-authored-by: Bastian Müller <bastian@turbolent.com>
…_exists; transaction: collect_insurance
Added mock: set_insurance_swapper_mock.cdc
…saction. Moved flowTokenIdentifier to test_helpers.
| let yearsElapsed = UFix128(timeElapsed) / FlowCreditMarket.secondsInYear | ||
| let insuranceRate = UFix128(self.insuranceRate) | ||
| // Insurance amount is a percentage of total credit balance per year | ||
| let insuranceAmount = self.totalCreditBalance * insuranceRate * yearsElapsed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kay-Zee, I noticed a comment on similar code in another PR about updating this calculation. I just want to flag that we're using totalCreditBalance here.
Could you clarify please if the calculation change should apply here as well, or if this implementation is correct?
If there's a doc with the expected calculations, could you share it so I can verify everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the calculation change should be applied here as well. Since previously that implementation would be a flat rate charged on all borrows instead of it being the desired proportional fee collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current PR, we won't fix this because we found a problem with the calculation of perSecondInterestRate that should be fixed in this task.
Later, the current calculation issue will be fixed in this PR.
Сurrently it is partially fixed - replaced self.totalCreditBalance by self.totalDebitBalance - still wrong, but tests updated to create "borrows" in order to calculate it insurance.
Is it ok to merge current PR? or described problem above should be fixed first?
@Kay-Zee @liobrasil @Gornutz
jordanschalm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes to FlowCreditMarket
…according to comment
…ranceRate * yearsElapsed
Kay-Zee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly, nit's generally, looks good.
| assert(swapper.outType() == Type<@MOET.Vault>(), message: "Swapper output type must be MOET") | ||
|
|
||
| } else { | ||
| // cannot remove swapper if insurance rate > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other choice here would be to set the insurance rate to 0 on removal.
Honestly, not sure exactly which way is best, probably what you all have now where you have to explicitly set the insurance rate and then remove the swapper. With that said, i feel like this won't be a common operation so probably either option is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose current approach to keep the two configurations separate. This way governance actions are more predictable and each change is clear.
| } | ||
|
|
||
| execute { | ||
| self.pool.setInsuranceSwapper(tokenType: self.tokenType, swapper: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide not to "auto" set insurance rate to zero, then i think the "base" tx here should set the insurance rate to 0 before removing the swapper, to showcase the correct way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve addressed this by splitting the behavior into two transactions and adding explicit documentation to each to clarify their intended usage in bba6631:
-
I added a base governance transaction at
./cadence/transactions/flow-credit-market/pool-governance/remove_insurance_swapper.cdc
which explicitly sets the insurance rate to0.0before removing the swapper. This showcases the correct and safe way to disable insurance when the protocol does not auto-zero the rate. -
I left the transaction without setting the insurance rate in
./cadence/tests/transactions/flow-credit-market/pool-governance/remove_insurance_swapper.cdc
for testing purposes only. This version is used to validate failure scenarios and invariants when an insurance rate is still set and greater than zero.
This keeps the flow explicit and safe, while still allowing tests to cover the edge case where the swapper is removed without first disabling the insurance rate.
Gornutz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the small suggestions from @Kay-Zee outside of that, nothing else stands out to me.
Co-authored-by: Gornutz <90406700+Gornutz@users.noreply.github.com> Co-authored-by: Kan Zhang <kan.zhang@flowfoundation.com>
…/FlowCreditMarket into kan/insurance-rate-stability-rate
| let insuranceAmountUFix64 = FlowCreditMarketMath.toUFix64RoundDown(insuranceAmount) | ||
|
|
||
| // If calculated amount is zero or negative, skip collection but update timestamp | ||
| if insuranceAmountUFix64 <= 0.0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UFix64 can't be negative
| } | ||
|
|
||
| /// Collects insurance by withdrawing from reserves and swapping to MOET. | ||
| /// The insurance amount is calculated based on the insurance rate applied to the total credit balance over the time elapsed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insurance rate applies to the total debit balance (not credit)
Closes: #84
Description
Implementation of insurance fund
For contributor use:
masterbranchFiles changedin the Github PR explorer