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

Make accrueInterests public ? #129

Closed
MerlinEgalite opened this issue Jul 14, 2023 · 18 comments · Fixed by #171, #183 or #565
Closed

Make accrueInterests public ? #129

MerlinEgalite opened this issue Jul 14, 2023 · 18 comments · Fixed by #171, #183 or #565
Assignees
Labels

Comments

@MerlinEgalite
Copy link
Contributor

Based on #3

@MathisGD
Copy link
Contributor

It was not settled right ?

@MerlinEgalite
Copy link
Contributor Author

Ok added the question label. I thought you were in favor.

@MathisGD MathisGD changed the title Make accrueInterests public Make accrueInterests public ? Jul 15, 2023
@MathisGD
Copy link
Contributor

I said that I don't see any significant drawbacks, but we still need to make sure that it's useful in some way.

@MerlinEgalite
Copy link
Contributor Author

Ok mb then

@QGarchery
Copy link
Contributor

Because it's already basically available by doing small transactions but in a clunky way, I'm in favor of adding it. The use case for it would be to compound interests more precisely, which may be interesting in some cases (but maybe less relevant with #135)

@Rubilmax Rubilmax self-assigned this Jul 17, 2023
@Rubilmax
Copy link
Collaborator

Referencing my comment for completeness: #3 (comment). I'll suggest a PR for this ASAP.

@Rubilmax
Copy link
Collaborator

Making accrueInterests public is actually not enough to expose a getter because the EVM reverts as soon as a staticcall changes the state. So we need to isolate the logic to a dedicated getter instead.

@pakim249CAL

This comment was marked as outdated.

@Rubilmax
Copy link
Collaborator

It's a bit of a hack, but we can also simply expose the accrueInterests as public without a view in the main contract, and integrators can simply use a view modifier in their own interfaces when calling it to be able to static call.

It seems this doesn't work:
image

And it seems it is equivalent to having integrators staticcall(accrueInterests)

@pakim249CAL
Copy link
Contributor

pakim249CAL commented Jul 17, 2023

Ah yeah I'll hide my older comment. Seems like I'm wrong. I'll do a little more research on it.

@MerlinEgalite
Copy link
Contributor Author

Yeah @adhusson remembered me that a staticcal reverts and there's an OPCODE modifying the state. So it's unfortunately not possible. Maybe the solution you told me about @adhusson would work here btw (even though a bit overkill here I think)!

@adhusson
Copy link
Contributor

I’m scarce on context but here is what comes to mind:

  • The scheme I have in mind is about adding arbitrary « semantically view » functions to a contract (I want it to reduce codesize). TLDR is make a contract delegatecall arbitrary code even hostile code, then revert and return the returned value.
  • These functions cannot be syntactically view, and if you force them to be view by breaking the type system then you will get compiled a staticcall which is bad as explained by you guys above.
  • However you can make them view in the abi destined to clients, they will do an eth_call and everything will work.
  • I like the idea by @Rubilmax in Balance getter discussion #3 to split accrual computation from accrual update. On the surface it seems like a fine solution that is also simple?

@MerlinEgalite
Copy link
Contributor Author

Juste to recenter the debate on this issue because I think it was not clear enough. The question is more the following: do we want to let people accrue interest by triggering a function without needing to supply/borrow/... ?

It seems that #135 fixes the pb for market with few interactions though

@pakim249CAL
Copy link
Contributor

I think an accrue interest function could be very valuable to integrators based on our own experiences with integrating with other lending protocols. Even with my PR, the interest accrued cannot be predetermined exactly without some sort of lens. I think integrators should be able to accrue interest (or have some sort of getter) to ease exact computations for them if they need it.

@Rubilmax
Copy link
Collaborator

Rubilmax commented Jul 20, 2023

Supporting @pakim249CAL's POV, but I'd be ok to extract this to a library which can be embedded into every integrator's bytecode or made external and deployed.
In order to be fully convinced I'm ok with this alternative solution (instead of exposing a getter for accrued interests), I'd have to check the bytecode size of such a library. Let's say I'd be ok if it weighs less than 0.6kb.

EDIT: I read too fast. I now understand that @pakim249CAL suggests to have accrueInterests public so integrators can accrue interests before querying totalSupply/totalBorrow and don't have to calculate interests upfront. I agree.

@MerlinEgalite
Copy link
Contributor Author

I also agree that it would be useful but I'm less convinced about the need of a getter now.

@Rubilmax
Copy link
Collaborator

Rubilmax commented Jul 21, 2023

I also agree that it would be useful but I'm less convinced about the need of a getter now.

So you essentially support #147 right? And we'd suggest integrators that don't want to trigger the public accrueInterests to use a library we develop, mimicking the interests accrual and fee calculation?

EDIT: I'd like to quantify the gas cost of accrueInterests and associated calculation in order to make a choice.

@Rubilmax
Copy link
Collaborator

This PR introduced it: #183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment