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

refactor(interests): expose accrued interests and fee #138

Closed
wants to merge 6 commits into from

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented Jul 17, 2023

The goal of this PR is to let integrators be able to query the amount of hypothetically accrued interests (and corresponding fee shares) upon interaction. In solidity, it would look like:

Changes are significant and we lose minimalism, so I am ok to leave the protocol without this "hypothetical interests" getter. Integrators have the possibility to query independently query the variables necessary for this calculation, but it would cost them more than with the corresponding getter built into the protocol, because of the cost of CALLs.

The difference of gas cost between a built-in getter and a reproduced calculation would approximately be:

  • 2 CALLs (totalBorrow + lastUpdate) if only interested in the borrow side (don't need to calculate the fee)
  • 5 CALLs (totalSupply + totalBorrow + lastUpdate + fee + totalSupplyShares) if interested in the supply side

To address this issue, we could expose a getter for all information related to a state. It could save 4 CALLs in the worst case, which approximately represents 20k gas.

EDIT: actually, just expose extsload with arrays, just like UniV4, and this topic can be closed.

@Rubilmax Rubilmax marked this pull request as ready for review July 17, 2023 14:20
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

EDIT: actually, just expose extsload with arrays, just like UniV4, and this topic can be closed.

Maybe we can do that and write some helpers in the doc help developers?

MerlinEgalite
MerlinEgalite previously approved these changes Jul 18, 2023
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

I'm also ok with this solution

src/Blue.sol Outdated Show resolved Hide resolved
@QGarchery
Copy link
Contributor

QGarchery commented Jul 18, 2023

actually, just expose extsload with arrays, just like UniV4, and this topic can be closed.

I'm for adding a getter like this. It seems like the one of uniswap is not appropriate because it only allows to retrieve contiguous storage (and totalSupply, totalBorrow, .. are not contiguous). I think it can be easily adapted to take an array of slots as input though

@Rubilmax Rubilmax marked this pull request as draft July 19, 2023 08:22
@Rubilmax Rubilmax marked this pull request as ready for review July 19, 2023 08:48
@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Jul 19, 2023

I'm for adding a getter like this. It seems like the one of uniswap is not appropriate because it only allows to retrieve contiguous storage (and totalSupply, totalBorrow, .. are not contiguous). I think it can be easily adapted to take an array of slots as input though

Done in #147!

I gave my POV there, but TLDR: I may prefer this solution even though it complexifies Blue, because I believe it has a way higher business value for integrators (including MetaMorpho vaults).

Jean-Grimal
Jean-Grimal previously approved these changes Jul 19, 2023
@Rubilmax Rubilmax requested a review from MerlinEgalite July 19, 2023 09:27
@Rubilmax Rubilmax changed the base branch from main to fix/hardhat-ci-collateral July 20, 2023 14:19
MerlinEgalite
MerlinEgalite previously approved these changes Jul 20, 2023
Base automatically changed from fix/hardhat-ci-collateral to main July 21, 2023 13:43
@Rubilmax Rubilmax dismissed stale reviews from MerlinEgalite and Jean-Grimal July 21, 2023 13:43

The base branch was changed.

@Rubilmax Rubilmax marked this pull request as draft July 25, 2023 08:25
@MathisGD
Copy link
Contributor

which approximately represents 20k gas.

No it's more like 4 x 100 gas

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Aug 7, 2023

#171 seems to better do the job

@Rubilmax Rubilmax closed this Aug 7, 2023
@MerlinEgalite MerlinEgalite reopened this Aug 15, 2023
@MerlinEgalite
Copy link
Contributor

Well merge conflicts are not really nice. I'll redo the PR

@Rubilmax Rubilmax deleted the refactor/accrue-interests branch August 28, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make accrueInterests public ?
5 participants