search

morpho-org / morpho-optimizers

Core contracts of Morpho V1 Optimizers.
https://app.morpho.org
GNU Affero General Public License v3.0
137 stars 22 forks source link

Update liquidity data naming #1559

Closed Rubilmax closed 1 year ago

Rubilmax commented 1 year ago

Pull Request

Issue(s) fixed

This is a naming suggestion, which I find clearer (at least)

pakim249CAL commented 1 year ago

One thing to keep in mind is that we should NOT use inETH naming in V3 to remain chain agnostic. AAVE has also discussed not using ETH as the quote currency at some points, so it's important to have a more generic name ready for if that happens.

MerlinEgalite commented 1 year ago

Should we merge it since we already merged the change with liquidationThresholdValue?

Rubilmax commented 1 year ago

Should we merge it since we already merged the change with liquidationThresholdValue?

The PR for liquidationThresholdValue is fixing a specific issue raised by Spearbit This PR is a proposal to completely rename the liquidity struct fields, to try to make it clearer (from the discussion we had in #1474)

So the decision on whether we should merge it or not should not depend on whether we've previously merged "the change with liquidationThresholdValue"

I still think this PR adds much clarity

MerlinEgalite commented 1 year ago

Should we merge it since we already merged the change with liquidationThresholdValue?

The PR for liquidationThresholdValue is fixing a specific issue raised by Spearbit This PR is a proposal to completely rename the liquidity struct fields, to try to make it clearer (from the discussion we had in #1474)

So the decision on whether we should merge it or not should not depend on whether we've previously merged "the change with liquidationThresholdValue"

I still think this PR adds much clarity

Ok wanted to make sure.

Can you fix the conflicts, please? i guess we'll be able to merge it

Rubilmax commented 1 year ago

I added a small change for coherence

Rubilmax commented 1 year ago

It's changing the bytecode right?

Yes it is changing the bytecode, but it's ok to change the bytecode for the next audit session?

image
MerlinEgalite commented 1 year ago

It's changing the bytecode right?

Yes it is changing the bytecode, but it's ok to change the bytecode for the next audit session?

image

Ah yes sorry about that!

Actually I checked and it does not change the bytecode btw

Rubilmax commented 1 year ago

Actually I checked and it does not change the bytecode btw

Oh, surprising