-
Notifications
You must be signed in to change notification settings - Fork 36
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 post-cantina
#349
Update post-cantina
#349
Conversation
refactor(interface): add metamorpho factory interface
refactor(ifc): move natspecs to interfaces
function timelock() external view returns (uint256); | ||
|
||
/// @dev Stores the order of markets on which liquidity is supplied upon deposit. | ||
/// @dev Can contain any market. A market is skipped as soon as its supply cap is reached. | ||
function supplyQueue(uint256) external view returns (Id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not have @notice
, @param
and @return
I would have anyway suggested documenting it properly, but I think that in this case is even more important given that the name of the function seems more "active" (by executing you supply to the queue) while instead it's a getter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The supplyQueue
, as far as I remember, can contain duplicates (I don't think you have changed such behavior).
In the withdrawQueue
you specify that it does not contain duplicates. You should also specify with a @dev
comment that supplyQueue
can contain duplicates.
|
||
/// @dev Stores the order of markets from which liquidity is withdrawn upon withdrawal. | ||
/// @dev Always contain all non-zero cap markets as well as all markets on which the vault supplies liquidity, | ||
/// without duplicate. | ||
function withdrawQueue(uint256) external view returns (Id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not have @notice
, @param
and @return
I would have anyway suggested documenting it properly, but I think that in this case is even more important given that the name of the function seems more "active" (by executing you supply to the queue) while instead it's a getter.
function revokePendingCap(Id id) external; | ||
|
||
/// @notice Submits a forced market removal from the vault, potentially losing all funds supplied to the market. | ||
/// @dev Warning: Submitting a forced removal will overwrite the timestamp at which the market will be removable. | ||
function submitMarketRemoval(Id id) external; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be worth specifying that this function does set the supply cap to 0
immediately, but does not remove such market from the withdraw queue. You need to later on call updateWithdrawQueue
to remove it from the queue.
function revokePendingGuardian() external; | ||
|
||
/// @notice Skims the vault `token` balance to `skimRecipient`. | ||
function skim(address) external; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's missing the parameter name in the signature
@@ -30,57 +30,148 @@ interface IOwnable { | |||
/// @dev This interface is used for factorizing IMetaMorphoStaticTyping and IMetaMorpho. | |||
/// @dev Consider using the IMetaMorpho interface instead of this one. | |||
interface IMetaMorphoBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MerlinEgalite please verity that each function is properly and fully natspec documented with at least @notice
and all the needed @param
and @return
tags. Many of the functions are missing these tags.
/// @param name The name of the vault. | ||
/// @param symbol The symbol of the vault. | ||
/// @param salt The salt to use for the MetaMorpho vault's CREATE2 address. | ||
function createMetaMorpho( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is missing the @return
tag
No description provided.