-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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 ERC4626-maxWithdraw to reflect other functions overrides #5130
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 757a25f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
function maxWithdraw(address owner) public view virtual returns (uint256) { | ||
return _convertToAssets(balanceOf(owner), Math.Rounding.Floor); | ||
return previewRedeem(maxRedeem(owner)); |
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.
This is the change I propose.
In the case of ERC4626Fees, the fees inclusion in previewRedeem
woudl reflect here.
Additionally, if maxRedeem
is ever overriden, it would reflect here (but the inverse is not true)
Co-authored-by: Ernesto García <[email protected]>
On second thought (thanks @cairoeth), I don't think the changelog entry is enough. It seems like there are multiple cases of ERC4626 vaults that override To me, it's not a huge deal because I can't see how it may affect the vault critically. If we continue with this change, we should discuss whether a changelog entry under the "Breaking Changes" section is good enough, or whether the change will cause an issue in some projects. |
Following up on ^, the initial problem is that All in all, I think the changes in the PR are good but imo we can avoid a breaking change by applying them directly in |
Opened #5135 as an alternative. It would make sense to include the I'm not sure that's worth documenting. The "raw" ERC4626 shouldn't have any concerns about fees, and overriding functions are already assumed under the developer's risk. |
So @cairoeth and myself quickly discussed this change, and if you read the Now given the changeset for this PR, you could override
Under these circumstances, I would not recommend implementing this change. |
My deep feeling is that This PR would increass the coupling between the two. To me the consequence of this coupling are the following:
It is my personal opinion that the first point (wanting consistency) is way more common (and desirable), and this is the one we should favor in our designs. We also identified instances of buggy vaults that would not have been buggy if this PR had been implemented. That is why I continue to support this change. If you can find any example of real production code (not POCs) that this would break, I'd love to study them! |
@cairoeth WDYT?
PR Checklist
npx changeset add
)