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

Remove automatic supply queue push #351

Merged

Conversation

MathisGD
Copy link
Contributor

@MathisGD MathisGD commented Dec 15, 2023

@MathisGD MathisGD requested a review from a team December 15, 2023 08:53
@MathisGD MathisGD self-assigned this Dec 15, 2023
@MathisGD MathisGD requested review from adhusson, Rubilmax, QGarchery, MerlinEgalite and Jean-Grimal and removed request for a team December 15, 2023 08:53
Jean-Grimal
Jean-Grimal previously approved these changes Dec 15, 2023
test/forge/helpers/IntegrationTest.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.

this discussion needs to be resolved first: https://github.com/morpho-org/metamorpho/pull/351/files#r1427738018

Copy link
Collaborator

@adhusson adhusson left a comment

Choose a reason for hiding this comment

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

This also fixes MM-3 from my report (but duplicates can still be set manually, and so maxDeposit can still be wrong).

Comment on lines +120 to +121
if (newCap > 0) {
if (!isEnabled) {
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
if (newCap > 0) {
if (!isEnabled) {
if (newCap > 0 && !isEnabled) {

@MerlinEgalite MerlinEgalite merged commit dab10a1 into post-cantina Dec 22, 2023
6 checks passed
@MerlinEgalite MerlinEgalite deleted the refactor/remove-automatic-supply-queue-push branch December 22, 2023 15:12
@Jean-Grimal Jean-Grimal mentioned this pull request Dec 28, 2023
@StErMi
Copy link

StErMi commented Dec 30, 2023

The PR seems OK. Consider documenting the de-sync between the withdrawQueue and supplyQueue (before, when a market was added, both the supply and withdraw queue were configured and pushed).

You could have the whole withdrawQueue fully configured, but no market from which you can supply to.

@QGarchery
Copy link
Contributor

You could have the whole withdrawQueue fully configured, but no market from which you can supply to.

Yes this is a wanted behavior: some vault curators will prefer reallocating every time instead of using the supply queue.

Consider documenting the de-sync between the withdrawQueue and supplyQueue (before, when a market was added, both the supply and withdraw queue were configured and pushed).

I think it's ok considering that there is already other functions that could desync the two queues (which seems natural since the code has 2 queues, so it would make sense if they are not always the same)

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.

7 participants