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

Harmonize p2p rate computation #1492

Merged
merged 2 commits into from
Dec 8, 2022
Merged

Harmonize p2p rate computation #1492

merged 2 commits into from
Dec 8, 2022

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax linked an issue Nov 30, 2022 that may be closed by this pull request
@Rubilmax Rubilmax changed the base branch from upgrade-0 to upgrade-lens-0 November 30, 2022 15:53
Copy link
Collaborator

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

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

There are other places where the function could be used.

_poolSupplyRate +
_p2pIndexCursor *
_poolBorrowRate) / MAX_BASIS_POINTS;
return PercentageMath.weightedAvg(_poolSupplyRate, _poolBorrowRate, _p2pIndexCursor);
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this function supposed to handle the case where the rates are inverted ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but this PR does not address this issue. I chose to handle them independently. I'm opening a PR on top of this one to handle #1467 (the one in your mind)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, inverted spreads are only possible on aave-v2?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No on both, but for different reasons

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Dec 1, 2022

There are other places where the function could be used.

Please show me (note the function doesn't exist in aave-v2's InterestRatesModel)

@MathisGD
Copy link
Collaborator

MathisGD commented Dec 1, 2022

In the aave-v2/RatesLens.sol sorry, at least lines 219 275 and 335.

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Dec 1, 2022

In the aave-v2/RatesLens.sol sorry, at least lines 219 275 and 335.

Just force-pushed (because it's a change of paradigm) a new version, getting rid of this unnecessary function in InterestRatesModel (just use weightedAvg everywhere instead)

@MathisGD
Copy link
Collaborator

MathisGD commented Dec 1, 2022

Don't we have issues here when the spread is natively inverted ? This time, I was in favor of having an internal function

Copy link
Collaborator

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

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

Still one question to answer

@MerlinEgalite
Copy link
Contributor

MerlinEgalite commented Dec 2, 2022

Don't we have issues here when the spread is natively inverted ? This time, I was in favor of having an internal function

Can we have the same issue as on Aave? Knowing that there's no flashloans nor stable rates on Compound.

Anyway, I think it's safe doing the same we did on Aave.

@MathisGD
Copy link
Collaborator

MathisGD commented Dec 2, 2022

Can we have the same issue as on Aave? Knowing that there's no flashloans nor stable rates on Compound.

The indexes can grow as if there were an inverted spread, when someone send some token on the cToken contract

@MerlinEgalite
Copy link
Contributor

Can we have the same issue as on Aave? Knowing that there's no flashloans nor stable rates on Compound.

The indexes can grow as if there were an inverted spread, when someone send some token on the cToken contract

Yes but it broke the growth factors, not the rates no? But as I say I'm for changing it

@MathisGD
Copy link
Collaborator

MathisGD commented Dec 2, 2022

Yes but it broke the growth factors, not the rates no? But as I say I'm for changing it

tbh I don't remember how the supply rate is computed exacly on Compound

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Dec 6, 2022

https://github.com/compound-finance/compound-protocol/blob/master/contracts/WhitePaperInterestRateModel.sol

On Compound, $supplyRate = utilization * borrowRate * (1 - r)$, which ensures that $supplyRate <= borrowRate$ (because $0 <= utilization <= 1$ and $0 <= r <= 1$), so it's safe to me

Note: $utilization$ as well as $borrowRate$ can be decreased by manipulating getCashPrior (sending underlyings to the CToken contract), but the above argument is enough alone

@QGarchery
Copy link
Collaborator

https://github.com/compound-finance/compound-protocol/blob/master/contracts/WhitePaperInterestRateModel.sol

On Compound, supplyRate=utilization∗borrowRate∗(1−r), which ensures that supplyRate<=borrowRate (because 0<=utilization<=1 and 0<=r<=1), so it's safe to me

Note: utilization as well as borrowRate can be decreased by manipulating getCashPrior (sending underlyings to the CToken contract), but the above argument is enough alone

The white paper abstracts the computation too much in this case. Only the supply index is changed if some underlying tokens are sent to the cToken contract, so the equality does not stand anymore. See the computation of the supply index. Notice that the function exchangeRateCurrent accrues the interests, I found that those comments are the easiest to read.

For an explanation of why the equation stands without transfer of underlyings to the cToken contract : https://github.com/morpho-labs/morpho-yellowpaper/issues/117

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Dec 7, 2022

Seen together:

  1. The WhitePaperInterestRateModel is not the IRM actually used in production. The one used is JumpRateModelV2
  2. The equation I written still stands with the current used IRM, and is based on the code from getSupplyRate, called by CTokens (https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/CToken.sol#L203-L217)

Note: Compound exposes rates per block which are not computed based on the CToken indexes. This is what we call "desync", because the indexes can grow by sending underlyings to CTokens, and may grow faster than what is computed by supplyRatePerBlock
For now though, we do use rates per block computed by Compound, from supplyRatePerBlock & borrowRatePerBlock, which were just proven to always be in ascending order

@MerlinEgalite
Copy link
Contributor

Note: I think we should not rely on papers but on the code for the behavior of the underlying contracts we're plugging into.

@Rubilmax Rubilmax merged commit 9b51df5 into upgrade-lens-0 Dec 8, 2022
@Rubilmax Rubilmax deleted the fix/lens-p2p-rate branch December 8, 2022 12:40
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.

[R&D review] Reuse computation for p2p rate per block
5 participants