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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
+26 −8
Closed

Fix bounds #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abf841b
fix: try fixing bounds
MerlinEgalite Nov 12, 2023
08a6921
refactor: remove bounded average
MerlinEgalite Nov 13, 2023
1eec0de
test: remove failing test
MerlinEgalite Nov 14, 2023
88b72e2
Merge branch 'refactor/curve' of github.com:morpho-labs/morpho-blue-i…
MerlinEgalite Nov 14, 2023
1316754
test: remove correct test
MerlinEgalite Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/SpeedJumpIrm.sol
View file
Open in desktop
Copy link
Contributor

@QGarchery QGarchery Nov 14, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@QGarchery QGarchery Nov 14, 2023 •
edited
Loading

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

Copy link
Contributor Author

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

Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

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

uint256 endRateAtTarget =
startRateAtTarget.wMulDown(adaptationMultiplier).bound(MIN_RATE_AT_TARGET, MAX_RATE_AT_TARGET);
uint256 endBorrowRate = _curve(endRateAtTarget, err);
uint256 unboundedEndRateAtTarget = startRateAtTarget.wMulDown(adaptationMultiplier);
uint256 endRateAtTarget = unboundedEndRateAtTarget.bound(MIN_RATE_AT_TARGET, MAX_RATE_AT_TARGET);
uint256 endBorrowRate = _curve(unboundedEndRateAtTarget, err);

// Then we compute the average rate over the period.
// Note that startBorrowRate is defined in the computations below.
Expand Down
28 changes: 23 additions & 5 deletions test/SpeedJumpIrmTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,15 +190,33 @@ contract AdaptativeCurveIrmTest is Test {
);
}

function testUnbounded(uint256 startRateAtTarget, uint256 elapsed) public {
startRateAtTarget = bound(startRateAtTarget, irm.MIN_RATE_AT_TARGET(), irm.MAX_RATE_AT_TARGET());
elapsed = bound(elapsed, 0, 2 days);

int256 err = 1 ether;
int256 speed = ADJUSTMENT_SPEED.wMulDown(err);
uint256 adaptationMultiplier = MathLib.wExp(speed * int256(elapsed));

uint256 unboundedEndRateAtTarget = startRateAtTarget.wMulDown(adaptationMultiplier);
//uint256 endRateAtTarget = unboundedEndRateAtTarget.bound(irm.MIN_RATE_AT_TARGET(), irm.MAX_RATE_AT_TARGET());
uint256 endBorrowRate = _curve(unboundedEndRateAtTarget, err);
uint256 startBorrowRate = _curve(startRateAtTarget, err);

assertApproxEqAbs(startBorrowRate.wMulDown(adaptationMultiplier), endBorrowRate, 1e3);
}

function _expectedRateAtTarget(Id id, Market memory market) internal view returns (uint256) {
return _expectedUnboundedRateAtTarget(id, market).bound(irm.MIN_RATE_AT_TARGET(), irm.MAX_RATE_AT_TARGET());
}

function _expectedUnboundedRateAtTarget(Id id, Market memory market) internal view returns (uint256) {
uint256 rateAtTarget = irm.rateAtTarget(id);
int256 speed = ADJUSTMENT_SPEED.wMulDown(_err(market));
uint256 elapsed = (rateAtTarget > 0) ? block.timestamp - market.lastUpdate : 0;
int256 linearAdaptation = speed * int256(elapsed);
uint256 adaptationMultiplier = MathLib.wExp(linearAdaptation);
return (rateAtTarget > 0)
? rateAtTarget.wMulDown(adaptationMultiplier).bound(irm.MIN_RATE_AT_TARGET(), irm.MAX_RATE_AT_TARGET())
: INITIAL_RATE_AT_TARGET;
return (rateAtTarget > 0) ? rateAtTarget.wMulDown(adaptationMultiplier) : INITIAL_RATE_AT_TARGET;
}

function _expectedAvgRate(Id id, Market memory market) internal view returns (uint256) {
Expand All @@ -207,8 +225,8 @@ contract AdaptativeCurveIrmTest is Test {
int256 speed = ADJUSTMENT_SPEED.wMulDown(err);
uint256 elapsed = (rateAtTarget > 0) ? block.timestamp - market.lastUpdate : 0;
int256 linearAdaptation = speed * int256(elapsed);
uint256 endRateAtTarget = _expectedRateAtTarget(id, market);
uint256 newBorrowRate = _curve(endRateAtTarget, err);
uint256 unboundedEndRateAtTarget = _expectedUnboundedRateAtTarget(id, market);
uint256 newBorrowRate = _curve(unboundedEndRateAtTarget, err);

uint256 avgBorrowRate;
if (linearAdaptation == 0 || rateAtTarget == 0) {
Expand Down