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

Refactor interfaces #240

Merged
merged 7 commits into from
Oct 25, 2023
Merged

Refactor interfaces #240

merged 7 commits into from
Oct 25, 2023

Conversation

QGarchery
Copy link
Contributor

@QGarchery QGarchery commented Oct 19, 2023

Refactor interfaces with the following goals in mind:

  • provide a single interface (IMetaMorphoFull) that enables you to do all the calls to MM
  • provide an interface (IMetaMorpho) that the MetaMorpho contract inherits so that we benefit from the compiler checks on it
  • factorize the two interfaces so that we don't write the function signatures twice
  • fixes [protocol review] Provide an interface to get all structs #216

@QGarchery QGarchery requested review from MathisGD and a team October 19, 2023 17:21
@QGarchery QGarchery self-assigned this Oct 19, 2023
@QGarchery QGarchery requested review from Rubilmax, MerlinEgalite and Jean-Grimal and removed request for a team October 19, 2023 17:21
src/interfaces/IMetaMorpho.sol Show resolved Hide resolved
src/interfaces/IMetaMorpho.sol Outdated Show resolved Hide resolved
function pendingFee() external view returns (uint192 value, uint64 submittedAt);
}

interface IMetaMorphoFull is IMetaMorphoSpecific, IERC4626, IERC20Permit, IOwnable, IMultiCall {
Copy link
Contributor

Choose a reason for hiding this comment

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

But then IMetaMorpho does not inherit IERC4626 and thus the documentation won't appear in etherscan because the following comments won't work: /// @inheritdoc IERC4626

Copy link
Contributor Author

@QGarchery QGarchery Oct 20, 2023

Choose a reason for hiding this comment

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

I think it works because MetaMorpho inherits from ERC4626 which itself inherits from IERC4626 (btw this is one other advantage of this refactor: function signatures are imported only once, which is clearer). In this test PR, you can see that if you remove is ICounterGetNumber you get an error in the inheritdoc comment, which means that the inheritdoc comments are aware of inherited interfaces of inherited contracts

@MerlinEgalite
Copy link
Contributor

I'm not sure convince yet this is a better way to split the interfaces per the comment raised above

@QGarchery
Copy link
Contributor Author

I'm not sure convince yet this is a better way to split the interfaces per the comment raised above

It could be refactored a bit (comments, naming, ...) but I think that having a single interface for every usage is very nice, I don't see any real downside for now. Maybe we should even use the main name IMetaMorpho for the biggest interface with all the functions (which is called IMetaMorphoFull for now). Indeed that's the one that is expected to be useful for integrators, while the one inherited by the contract MetaMorpho is mainly there for checking the function signatures against the implementation

@Rubilmax
Copy link
Contributor

I'm not sure convince yet this is a better way to split the interfaces per the comment raised above

It could be refactored a bit (comments, naming, ...) but I think that having a single interface for every usage is very nice, I don't see any real downside for now. Maybe we should even use the main name IMetaMorpho for the biggest interface with all the functions (which is called IMetaMorphoFull for now). Indeed that's the one that is expected to be useful for integrators, while the one inherited by the contract MetaMorpho is mainly there for checking the function signatures against the implementation

That is a great idea! IMetaMorpho -> IMetaMorphoFlat or IMetaMorphoStaticTyping and IMetaMorphoFull -> IMetaMorpho

we should do the same for Blue IMO

@QGarchery
Copy link
Contributor Author

QGarchery commented Oct 20, 2023

Changed all the names, please review and tell me what you think:

  • IMetaMorpho is now the full interface, expected to be the most used
  • IMetaMorphoStaticTyping is the interface inherited by the MM contract (and used only there). It is checked by the compiler against the contract
  • IMetaMorphoBare the smallest interface that is common to the 2 previous interfaces (and used only there)

Should we put those in separate files ?

src/interfaces/IMetaMorpho.sol Outdated Show resolved Hide resolved
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 better now. I like @Rubilmax naming suggestion with base.

Perhaps we should add comments in interfaces to guide integrators? It may be hard to know which one to use at first glance

@MerlinEgalite
Copy link
Contributor

@QGarchery will you apply the change?

@MerlinEgalite MerlinEgalite changed the base branch from main to sec-review October 23, 2023 08:18
@MerlinEgalite MerlinEgalite changed the base branch from sec-review to cantina-review October 24, 2023 09:15
src/interfaces/IMetaMorpho.sol Outdated Show resolved Hide resolved
src/interfaces/IMetaMorpho.sol Outdated Show resolved Hide resolved
@MerlinEgalite
Copy link
Contributor

The CI is broken can you update this PR please @QGarchery ?

@QGarchery
Copy link
Contributor Author

QGarchery commented Oct 25, 2023

The CI is broken can you update this PR please @QGarchery ?

I merged the target branch but with no luck. Not sure what the problem is, why can't it find morpho-blue-irm ?

@MerlinEgalite
Copy link
Contributor

Ok I get it this is because of the IRM dependency. Just created this issue #266 and assigned @Rubilmax to it

@QGarchery QGarchery merged commit fec39c2 into cantina-review Oct 25, 2023
6 checks passed
@QGarchery QGarchery deleted the refactor/interfaces branch October 25, 2023 14:56
@MerlinEgalite
Copy link
Contributor

Please do not merge pending PRs on cantina-review

@MerlinEgalite MerlinEgalite restored the refactor/interfaces branch October 25, 2023 15:41
@MerlinEgalite
Copy link
Contributor

Reverting the PR here #271

@QGarchery QGarchery mentioned this pull request Oct 25, 2023
@QGarchery
Copy link
Contributor Author

Re-open the PR in #272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[protocol review] Provide an interface to get all structs
4 participants