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

Fix rounding in max deposit #346

Merged
merged 7 commits into from
Dec 26, 2023
Merged

Fix rounding in max deposit #346

merged 7 commits into from
Dec 26, 2023

Conversation

@Jean-Grimal Jean-Grimal requested review from a team, adhusson, Rubilmax, QGarchery, MerlinEgalite and MathisGD and removed request for a team December 12, 2023 17:29
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.

Ideally we should have a test for it
Hopefully it shouldn't be too difficult

src/MetaMorpho.sol Outdated Show resolved Hide resolved
Jean-Grimal and others added 2 commits December 13, 2023 09:48
Co-authored-by: Romain Milon <rmilon@gmail.com>
Signed-off-by: Jean-Grimal <83286814+Jean-Grimal@users.noreply.github.com>
Rubilmax
Rubilmax previously approved these changes Dec 13, 2023
@MerlinEgalite
Copy link
Contributor

MerlinEgalite
MerlinEgalite previously approved these changes Dec 13, 2023
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.

The change in _maxDeposit reimplements MorphoBalancesLib.expectedSupplyAssets (rounding up), and the change in _supplyMorpho reimplements MetaMorpho. _accruedSupplyBalance (rounding up).

Not sure how to remember this but any future modification of those functions may have to be ported to these reimplementations.

@MathisGD MathisGD merged commit ab8fb00 into post-cantina Dec 26, 2023
6 checks passed
@MathisGD MathisGD deleted the fix/issue-158 branch December 26, 2023 22:15
@Jean-Grimal Jean-Grimal mentioned this pull request Dec 28, 2023

totalSuppliable += supplyCap.zeroFloorSub(supplyAssets);
totalSuppliable += supplyCap.zeroFloorSub(supplyShares.toAssetsUp(totalSupplyAssets, totalSupplyShares));
Copy link

Choose a reason for hiding this comment

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

Like you have done for _supplyMorpho maybe would be good to also add a comment here. I know that this is more a "utility" function and not a "core" one like _supplyMorpho but it will be probably used very much by integrators.

Consider also adding it as a natspec @dev comment.

@StErMi
Copy link

StErMi commented Dec 30, 2023

I was worried about if (assets != 0) revert ErrorsLib.AllCapsReached(); but both _maxDeposit and _supplyMorpho adopt the same behavior, so it should be fine as long as users use maxDeposit if they want to be sure to not revert because they try to supply above the cap.

Note that because of the rounding up that overestimates what has been already supplied to Morpho, it will underestimate what you can supply (compared to the cap). This means that there's the possibility that the cap is never reachable (because of rounding)

This was referenced Jan 2, 2024
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