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

Update liquidity data naming #1559

Merged
merged 5 commits into from
Dec 13, 2022
Merged

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented Dec 9, 2022

Pull Request

Issue(s) fixed

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

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.

Why not using maxDebt instead of liquidationThreshold ?

Copy link
Collaborator

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

I liked the "value" suffix that we used on Compound, because:

  • it allows to have the same denomination for both Compound and Aave
  • it is more natural to me. I believe you can say "my debt value is ..." but not "my debt USD is ...", it should be "my debt in USD is ..."

Copy link
Contributor

@pakim249CAL pakim249CAL left a comment

Choose a reason for hiding this comment

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

I like it. There may be minor arguments against some of this, but as it stands it removes ambiguity about what the variables are.

Copy link
Contributor

@pakim249CAL pakim249CAL left a comment

Choose a reason for hiding this comment

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

Duplicate approve sorry. Github was acting up.

@pakim249CAL
Copy link
Contributor

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.

Base automatically changed from fix/spearbit-30 to upgrade-morpho-1 December 12, 2022 22:24
@MerlinEgalite
Copy link
Contributor

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

@Rubilmax
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

I added a small change for coherence

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.

It's changing the bytecode right?

@Rubilmax
Copy link
Collaborator Author

Rubilmax commented Dec 13, 2022

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
Copy link
Contributor

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
Copy link
Collaborator Author

Rubilmax commented Dec 13, 2022

Actually I checked and it does not change the bytecode btw

Oh, surprising

@Rubilmax Rubilmax merged commit ea8d7bb into upgrade-morpho-1 Dec 13, 2022
@Rubilmax Rubilmax deleted the refactor/liquidity-data branch December 13, 2022 16:37
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.

None yet

5 participants