search

morpho-org / morpho-blue-oracles

Morpho Blue Oracles
https://morpho.org
Other
35 stars 28 forks source link

Chainlink for ERC4626 factorized #16

Closed QGarchery closed 1 year ago

QGarchery commented 1 year ago

ONLY THE VAULT TESTS: Screenshot from 2023-10-10 11-22-22

WITHOUT THE VAULT TESTS (and without the WBTC tests, because they get skipped for some reason): Screenshot from 2023-10-10 12-00-44

Rubilmax commented 1 year ago

I'm not 100% sure that it's better.

Btw do you any gas cost study on this change?

Beyond gas cost, it's just less lines of code for the same complexity. And I do mean the same complexity: if separated, there's is still the possibility to set a ERC4626 and no quote or base feed so it's really just the same mechanics. I don't get why we'd not do this multi-purpose oracle wrapper

MerlinEgalite commented 1 year ago

Beyond gas cost, it's just less lines of code for the same complexity. And I do mean the same complexity: if separated, there's is still the possibility to set a ERC4626 and no quote or base feed so it's really just the same mechanics. I don't get why we'd not do this multi-purpose oracle wrapper

It's at least more confusing for reviewers and auditors to me. So I still think it adds a bit of complexity for them.

I'm not against it though.

A gas cost study would have been nice to add least know what is the total cost of triggering the oracle compared to the "yaourt nature" version (without vaults and without multiple oracles).

MerlinEgalite commented 1 year ago

Wait the difference is more than 10k ? I'm not sure how to interpret it 😅

QGarchery commented 1 year ago

Wait the difference is more than 10k ? I'm not sure how to interpret it 😅

I changed the description multiple times (won't do it anymore), so maybe you are referring to an older version. Keep in mind that we should compare the PRs, for the purpose of this choice.

As for why this is so costly to take into account the price of a vault, I'm assuming it is because the accounting warms multiple slots (things like totals shares, total assets, ...)

MerlinEgalite commented 1 year ago

I changed the description multiple times (won't do it anymore), so maybe you are referring to an older version. Keep in mind that we should compare the PRs, for the purpose of this choice.

As for why this is so costly to take into account the price of a vault, I'm assuming it is because the accounting warms multiple slots (things like totals shares, total assets, ...)

Ok now I understand it better!