Refactor ERC4626-maxWithdraw to reflect other functions overrides #5130
Conversation
🦋 Changeset detectedLatest commit: e20baa8 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.
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)
There was a problem hiding this comment.
It would be useful to add doc stating that only previewRedeem(..) needs to be override, something close to this comment or close to a previous comment from you "Someone that want to change the behvarior of both functions only has one function to override, and both behavior would be consistent with one another" into the top documentation of the contract (before or after [CAUTION]).
There was a problem hiding this comment.
does that look good ? fa67c3f
Amxx
changed the title
Co-authored-by: Ernesto García <ernestognw@gmail.com>
|
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! |
|
This is likelly going to be being delayed, from 5.2 to 5.3. So far:
We are delaying this because we are waiting for example of the second. At some point we need to stop delaying that more. If |
| 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.
It would be useful to add doc stating that only previewRedeem(..) needs to be override, something close to this comment or close to a previous comment from you "Someone that want to change the behvarior of both functions only has one function to override, and both behavior would be consistent with one another" into the top documentation of the contract (before or after [CAUTION]).
| function previewWithdraw(uint256 assets) public view virtual returns (uint256) { | ||
| return _convertToShares(assets, Math.Rounding.Ceil); | ||
| } | ||
|
|
||
| /** @dev See {IERC4626-previewRedeem}. */ | ||
| /// @inheritdoc IERC4626 |
There was a problem hiding this comment.
We should add a note here that if a developer chooses to make previewRedeem revert under certain scenarios, they should ensure maxWithdraw won't revert.
| function previewWithdraw(uint256 assets) public view virtual returns (uint256) { | ||
| return _convertToShares(assets, Math.Rounding.Ceil); | ||
| } | ||
|
|
||
| /** @dev See {IERC4626-previewRedeem}. */ | ||
| /// @inheritdoc IERC4626 |
There was a problem hiding this comment.
| /// @inheritdoc IERC4626 | |
| /** | |
| * @dev See {IERC4626-previewRedeem}. | |
| * NOTE: If this function is overridden to introduce reverting logic, ensure that `maxWithdraw` is overridden as necessary to return successfully in all situations. | |
| */ |
There was a problem hiding this comment.
This messes up the documentation. Instead, I'm proposing to add an "override guide" at the contract level: fa67c3f
Fell free to propose adding to it.
…xx/openzeppelin-contracts into refactor/erc4626-max-withdraw
|
Note Reviews pausedUse the following commands to manage reviews:
Walkthrough
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contracts/token/ERC20/extensions/ERC4626.sol (2)
122-150: NatSpec inheritance: verify docs output preserves important local notes.Switching to
@inheritdocis fine, but some generators only lift the next@devfrom the interface. Confirm the rendered docs still include any OZ‑specific notes where needed (you already kept a local block fordecimals()).Also applies to: 157-161, 162-181
177-180: Add a local note on previewRedeem about maxWithdraw dependency.Make the dependency explicit where users look for it. This mirrors prior feedback and reduces surprises.
- /// @inheritdoc IERC4626 + /** + * @inheritdoc IERC4626 + * NOTE: {maxWithdraw} relies on this function. If you change rounding, fees, or add edge-case logic here, + * ensure `maxWithdraw` remains non-reverting and consistent with `maxRedeem`. + */ function previewRedeem(uint256 shares) public view virtual returns (uint256) { return _convertToAssets(shares, Math.Rounding.Floor); }
🧹 Nitpick comments (3)
.changeset/shiny-dolphins-lick.md (1)
1-6: Strengthen the changeset with migration notes and cautions.This subtly changes behavior for vaults that override previewRedeem/maxRedeem (e.g., to include fees/limits). Add a brief “Potential impact” and “Action for custom vaults” to guide upgraders.
--- 'openzeppelin-solidity': minor --- `ERC4626`: compute `maxWithdraw` using `maxRedeem` and `previewRedeem` so that changes to the preview functions affect the max functions. + +### Potential impact +- Vaults overriding `previewRedeem` and/or `maxRedeem` will now affect `maxWithdraw`. This is typically desired (e.g., to include fees and limits), but note that `maxWithdraw` MUST NOT revert. + +### Action for custom vaults +- Ensure your `previewRedeem` implementation complies with the ERC‑4626 requirement “MUST NOT revert”. If you intentionally add reverting logic in `previewRedeem`, also override `maxWithdraw` to keep it non‑reverting in all situations.contracts/token/ERC20/extensions/ERC4626.sol (2)
47-58: Document the revert-coupling risk and single-override guidance.Good addition. Please explicitly note that preview functions must not revert, and that reverting previewRedeem can make maxWithdraw revert after this change. This aligns expectations and answers prior concerns.
* Note: When overriding this contract, some elements have to be considered * * * When overriding the behavior of the deposit or withdraw mechanism, one should override the internal functions. * Overriding {_deposit} automatically affects both {deposit} and {mint}. Similarly, overriding {_withdraw} * automatically affects both {withdraw} and {redeem}. Having inconsistent behavior between the {deposit} and {mint} * or between {withdraw} and {redeem} can lead to bugs. Overall it is not recommended to override the public facing * functions. * * * {maxWithdraw} depends on {maxRedeem}. Therefore, overriding {maxRedeem} only is enough. On the other hand, * overriding {maxWithdraw} only would have no effect on {maxRedeem}, and could create an inconsistency between the two * functions. + * + * * Per ERC‑4626, preview functions (e.g., {previewRedeem}) MUST NOT revert and MUST NOT depend on msg.sender. If you + * introduce reverting logic in {previewRedeem}, override {maxWithdraw} accordingly to preserve its MUST‑NOT‑revert + * guarantee.
152-155: Behavioral change LGTM; optional defensive non-reverting guard.The new composition correctly propagates fees/limits via maxRedeem/previewRedeem. Optional: enforce the MUST‑NOT‑revert property even if a custom previewRedeem erroneously reverts, by using an external self-call with try/catch and falling back to 0.
- function maxWithdraw(address owner) public view virtual returns (uint256) { - return previewRedeem(maxRedeem(owner)); - } + function maxWithdraw(address owner) public view virtual returns (uint256) { + uint256 shares = maxRedeem(owner); + // Defensive: preserve MUST-NOT-revert even if an override of previewRedeem reverts. + try IERC4626(address(this)).previewRedeem(shares) returns (uint256 assets) { + return assets; + } catch { + return 0; + } + }Note: This keeps semantics while avoiding unexpected reverts; cost is an external self-call in a view.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/shiny-dolphins-lick.md(1 hunks)contracts/interfaces/IERC4626.sol(1 hunks)contracts/token/ERC20/extensions/ERC4626.sol(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: tests-foundry
- GitHub Check: tests
🔇 Additional comments (1)
contracts/interfaces/IERC4626.sol (1)
201-215: Typo fix LGTM.Spelling correction in the previewRedeem NatSpec is correct; no interface changes.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
@cairoeth WDYT?
PR Checklist
npx changeset add)