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 last total assets in updateWithdrawQueue #337

Closed
wants to merge 9 commits into from

Conversation

Jean-Grimal
Copy link
Contributor

@Jean-Grimal
Copy link
Contributor Author

Comments on morpho-org/metamorpho-v1.1#2 to address

src/MetaMorpho.sol Outdated Show resolved Hide resolved
src/MetaMorpho.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

my comments on the former PR need to be addressed pls

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.

misclicked

@MerlinEgalite MerlinEgalite requested review from a team, adhusson, QGarchery and MathisGD and removed request for a team December 11, 2023 11:23
test/forge/FeeTest.sol Outdated Show resolved Hide resolved
adhusson
adhusson previously approved these changes Dec 11, 2023
src/MetaMorpho.sol Outdated Show resolved Hide resolved
src/MetaMorpho.sol Outdated Show resolved Hide resolved
src/MetaMorpho.sol Outdated Show resolved Hide resolved
src/MetaMorpho.sol Show resolved Hide resolved
test/forge/FeeTest.sol Show resolved Hide resolved
test/forge/FeeTest.sol Show resolved Hide resolved
test/forge/FeeTest.sol Show resolved Hide resolved
test/forge/FeeTest.sol Show resolved Hide resolved
test/forge/FeeTest.sol Show resolved Hide resolved
Co-authored-by: Romain Milon <[email protected]>
Signed-off-by: Jean-Grimal <[email protected]>
Co-authored-by: Romain Milon <[email protected]>
Signed-off-by: Jean-Grimal <[email protected]>
src/MetaMorpho.sol Outdated Show resolved Hide resolved
Co-authored-by: Merlin Egalite <[email protected]>
Signed-off-by: Jean-Grimal <[email protected]>
Copy link
Contributor

@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.

The forced removed market removal reducing the fee is not an issue to me: the fee is called a performance fee, and having to remove a market is part of the risk of the vault managers. It's up to vault managers to decide if they want to accrue interest right after removing a market, to start accruing fees again

@@ -360,6 +370,9 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph

withdrawQueue = newWithdrawQueue;

// Accrue interest on all the enabled markets except the removed ones.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait this does not accrue interest ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Accrue interest on all the enabled markets except the removed ones.
// Accrue fee on all the enabled markets except the removed ones.

@Rubilmax
Copy link
Contributor

Rubilmax commented Dec 22, 2023

The forced removed market removal reducing the fee is not an issue to me: the fee is called a performance fee, and having to remove a market is part of the risk of the vault managers. It's up to vault managers to decide if they want to accrue interest right after removing a market, to start accruing fees again

Interesting take. Out of curiosity, how would you recommend to do so? deposit + withdraw?

@QGarchery
Copy link
Contributor

Out of curiosity, how would you recommend to do so? deposit + withdraw?

Yes, or deposit 1 atomic asset on behalf of a dummy address

@Rubilmax
Copy link
Contributor

Replaced with #359

@Rubilmax Rubilmax closed this Dec 22, 2023
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.

5 participants