-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix bounds #73
Fix bounds #73
Conversation
3d0ff32
to
d587807
Compare
d587807
to
08a6921
Compare
|
still to check @morpho-labs/onchain @morpho-labs/protocol |
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.
Agree it makes the code match ∫ startBorrowRate * exp(speed * t) dt btw 0 and elapsed
. To note it creates an inconsistency elsewhere:
- at time
last(t)
we haveendBorrowRate = unboundedEndRateAtTarget * curve(err(last(t))
, - at time
t
we havestartBorrowRate = bound(unboundedEndRateAtTarget) * curve(err(t))
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.
Nice find but the changes come with the following risks:
- there is a risk that the unbounded end rate at target is too large. It is an exponential (in the time) after all, and this can lead to problematic rates and potentially to liquidations
- it does not work with the bounded wExp, because it was assumed that the rate at target would always be between the bounds.
At the same time, the current situation is not risky: it is bounded and it just corresponds to another value of the average rate.
For these reasons I'm against doing the changes in this PR.
Still, it's true that the formula does not work when the bound is reached. So we should at least adapt the comment above to mention when the formula is valid.
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.
there is a risk that the unbounded end rate at target is too large. It is an exponential (in the time) after all, and this can lead to problematic rates and potentially to liquidations
It's still capped to MAX_RATE_AT_TARGET * WEXP_UPPER_VALUE / WAD
though. But I agree. But if we do nothing the code is incorrect so we must find a solution I think. @Rubilmax is working on an alternative solution at the moment.
it does not work with the bounded wExp, because it was assumed that the rate at target would always be between the bounds.
I think we could reduce the max upper value returned by wExp
to avoid overflow in the _curve
function.
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.
It's still capped to MAX_RATE_AT_TARGET * WEXP_UPPER_VALUE / WAD though.
This is multiplying the max rate at target by 115432178323118442717551238386899909037872
, not really a small upper bound 😆
I think we could reduce the max upper value returned by wExp to avoid overflow in the _curve function.
I don't like it, because the _curve
function is supposed to return a rate based on the rate at target (that should be in the bounds). We are now considering edge cases where those rates are not really bounded anymore.
In the end, what is the issue with the current code ? What are we trying to fix exactly ? Just change the comment and we are fine, we don't have to stick to the formula for extreme edge cases, especially if the current code gives reasonable results where the rate is bounded.
The only issue I can see is that the rate can actually go down if we wait for super long. But this is not an issue in practice because the max rate at target is absurdly high
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.
Ok so let's update the comment then
@@ -137,9 +137,9 @@ contract AdaptativeCurveIrm is IIrm { | |||
int256 linearAdaptation = speed * int256(elapsed); | |||
uint256 adaptationMultiplier = MathLib.wExp(linearAdaptation); | |||
// endRateAtTarget is bounded between MIN_RATE_AT_TARGET and MAX_RATE_AT_TARGET. |
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.
This comment should be moved downward
With a |
Closing it but we should adapt the doc perhaps |
We could have made a special case for when the endRate touches the bounds: if end = min: avg = min |
could be a solution as well indeed! |
Based on the following comment: #54 (comment)
endBorrowRate = startBorrowRate * exp(linearAdaptation)
is true mathematically but it's not in the code because of how we bound values in the computation. Here is an example of a failing test showcasing the issue: #77