search

morpho-org / morpho-blue

🟦 Morpho Blue Protocol
https://morpho.org
Other
134 stars 49 forks source link

Balance getter discussion #3

Closed MathisGD closed 1 year ago

MathisGD commented 1 year ago

References:

julien-devatom commented 1 year ago

If you are using this getter internally, so.

I'm against creating external getters only for external usage.

QGarchery commented 1 year ago

I'm against creating external getters only for external usage.

Last time we talked you liked that idea, no ? The idea would be to avoid having to rewrite the logic in javascript

Rubilmax commented 1 year ago

I believe we're leaning towards minimalism in Blue, so I'd instead suggest to only expose the contract's accounting variables (shares, totalShares, totalAssets) and expose a getter to know how many interests have to be accrued to storage at next accrual. These getters should be enough to calculate anything related to a Blue market so we shouldn't have to add any other getter.

In fact, even the accruedInterests getter isn't required: one could mimic the logic. But this intermediary function would be used by the contract itself, so I'm just suggesting extracting it and making it public.

I am also ok to expose the accrueInterests function, but I don't think it's necessary. We could also make accrueInterests public and return the accruedInterests, so it can be staticcall and enables everything at once:

MerlinEgalite commented 1 year ago

Is there a problem of exposing the accrueInterests function to public? It's ok in my mind but we must be sure

julien-devatom commented 1 year ago

Is there a problem of exposing the accrueInterests function to public? It's ok in my mind but we must be sure

accrueInterests is not a view, so this vision is like compound v2: You cannot query the up-to-date balance without making a call. I'm more in favor to split the interest accruing logic & the storage as @Rubilmax suggested

Rubilmax commented 1 year ago

accrueInterests is not a view, so this vision is like compound v2: You cannot query the up-to-date balance without making a call.

I want to emphasize this, because it would be minimalist (and may thus please @MathisGD):

We could also make accrueInterests public and return the accruedInterests, so it can be staticcall and enables everything at once:

  • query interests to accrue (STATICCALL)
  • accrue interests (CALL)
MathisGD commented 1 year ago

In fact, even the accruedInterests getter isn't required: one could mimic the logic. But this intermediary function would be used by the contract itself, so I'm just suggesting extracting it and making it public.

At a first sight, I don't see any issue with putting it public (effect really similar to supplying and withdrawing 1 wei of collat).

We could also make accrueInterests public and return the accruedInterests, so it can be staticcall and enables everything at once: query interests to accrue (STATICCALL) accrue interests (CALL)

Interesting idea !

Rubilmax commented 1 year ago

So this discussion is now only about whether to expose getters, and which one of them. I'll highlight some comment I made above:

I believe we're leaning towards minimalism in Blue, so I'd instead suggest to only expose the contract's accounting variables (shares, totalShares, totalAssets) [...]. These getters should be enough to calculate anything related to a Blue market so we shouldn't have to add any other getter.

So a getter for the amount of collateral, a getter for the supply shares and a getter for the borrow shares is enough to me. It is also necessary to isolate accounting to logic to appropriate libraries, so integrators only have to copy-paste/import isolated logic.

Rubilmax commented 1 year ago

Should we close this?

Jean-Grimal commented 1 year ago

Should we close this?

What is the final decision about getters ?

Rubilmax commented 1 year ago

What is the final decision about getters ?

The current code expose storage getters and should be enough, provided #129 is resolved or anything similar. I'd argue this issue is thus redundant now

Balancer getters seem to be overkill as an integrator is in most usecases better off querying the shares, total supply (resp. borrow), total shares and calculate it themselves ; which also enables them to calculate the balance based on hypothetical market changes

Jean-Grimal commented 1 year ago

The current code expose storage getters and should be enough, provided https://github.com/morpho-labs/blue/issues/129 is resolved or anything similar. I'd argue this issue is thus redundant now

Ok so we won't expose getters for underlying balances and assume that integrators will compute it themselves via the shares storage getters and the shareMath lib ?

MerlinEgalite commented 1 year ago

Yes I think we can provide libraries or code snippets for that

Rubilmax commented 1 year ago

And we already do, via SharesMath! But we're missing a way to calculate interests to be accrued. #129 is about this, and attached PRs are: