Compute claimable consumption rates from internal strategy state#238
Compute claimable consumption rates from internal strategy state#238ernestognw wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughUpdated internal hook consumption logic across four ERC7540 extension contracts to derive pro-rata exchange rates from strategy-state async limits ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/token/ERC20/extensions/ERC7540DelayDeposit.sol`:
- Around line 116-125: The current shortcut treats shares == maxShares as a
full-claim even when maxShares == 0, so mint(0) can consume all remaining assets
if maxMint() rounded to zero; change the logic in the block that computes assets
(using _asyncMaxDeposit, _asyncMaxMint, maxAssets, maxShares, shares,
Math.mulDiv) to first handle the one-sided-zero case: if maxShares == 0 then set
assets = 0 (which also covers the both-zero case), else if shares == maxShares
return maxAssets, otherwise compute Math.mulDiv(...) as before. This mirrors the
redeem-side guard and prevents mint(0) from draining assets when maxShares
rounded to zero.
In `@contracts/token/ERC20/extensions/ERC7540DelayRedeem.sol`:
- Around line 100-109: The fast-path that returns maxShares when assets ==
maxAssets incorrectly triggers for withdraw(0,...) when maxAssets == 0; update
the shortcut in the shares calculation to only bypass Math.mulDiv when the
withdrawable side is non-zero (e.g., change the condition to assets == maxAssets
&& maxAssets != 0), otherwise set shares to 0 or fall through to safe handling
to avoid consuming redeem dust; edit the expression that computes shares (using
_asyncMaxWithdraw, _asyncMaxRedeem, maxAssets, maxShares) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae220f1e-20be-4415-b010-738754ee6438
📒 Files selected for processing (5)
contracts/token/ERC20/extensions/ERC7540.solcontracts/token/ERC20/extensions/ERC7540AdminDeposit.solcontracts/token/ERC20/extensions/ERC7540AdminRedeem.solcontracts/token/ERC20/extensions/ERC7540DelayDeposit.solcontracts/token/ERC20/extensions/ERC7540DelayRedeem.sol
| // Pro-rata is computed against {_asyncMaxDeposit} / {_asyncMaxMint} (strategy state) rather than the | ||
| // public {maxDeposit} / {maxMint}, so cap/pause overrides on the public surface cannot desynchronize | ||
| // the rate used for consumption. When `shares` equals the controller's full claimable balance | ||
| // (including the case where both sides are 0), the entire remaining {_asyncMaxDeposit} is returned | ||
| // directly to avoid division by zero. | ||
| uint256 maxAssets = _asyncMaxDeposit(controller); | ||
| uint256 maxShares = _asyncMaxMint(controller); | ||
| uint256 assets = shares == maxShares | ||
| ? maxAssets | ||
| : Math.mulDiv(shares, maxAssets, maxShares, Math.Rounding.Ceil); |
There was a problem hiding this comment.
Don’t turn mint(0) into a full claim when maxMint() rounds to zero.
Here maxShares is derived with convertToShares(..., Floor), so it can be 0 while maxAssets is still positive. The new shortcut on Line 123 then makes mint(0, ...) consume all remaining claimable assets and mint 0 shares. This needs the same one-sided-zero guard as the redeem side.
Suggested fix
uint256 maxAssets = _asyncMaxDeposit(controller);
uint256 maxShares = _asyncMaxMint(controller);
- uint256 assets = shares == maxShares
- ? maxAssets
- : Math.mulDiv(shares, maxAssets, maxShares, Math.Rounding.Ceil);
+ uint256 assets = maxShares == 0
+ ? 0
+ : shares == maxShares
+ ? maxAssets
+ : Math.mulDiv(shares, maxAssets, maxShares, Math.Rounding.Ceil);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/token/ERC20/extensions/ERC7540DelayDeposit.sol` around lines 116 -
125, The current shortcut treats shares == maxShares as a full-claim even when
maxShares == 0, so mint(0) can consume all remaining assets if maxMint() rounded
to zero; change the logic in the block that computes assets (using
_asyncMaxDeposit, _asyncMaxMint, maxAssets, maxShares, shares, Math.mulDiv) to
first handle the one-sided-zero case: if maxShares == 0 then set assets = 0
(which also covers the both-zero case), else if shares == maxShares return
maxAssets, otherwise compute Math.mulDiv(...) as before. This mirrors the
redeem-side guard and prevents mint(0) from draining assets when maxShares
rounded to zero.
| // Pro-rata is computed against {_asyncMaxWithdraw} / {_asyncMaxRedeem} (strategy state) rather than the | ||
| // public {maxWithdraw} / {maxRedeem}, so cap/pause overrides on the public surface cannot desynchronize | ||
| // the rate used for consumption. When `assets` equals the controller's full claimable balance | ||
| // (including the case where both sides are 0), the entire remaining {_asyncMaxRedeem} is returned | ||
| // directly to avoid division by zero. | ||
| uint256 maxAssets = _asyncMaxWithdraw(controller); | ||
| uint256 maxShares = _asyncMaxRedeem(controller); | ||
| uint256 shares = assets == maxAssets | ||
| ? maxShares | ||
| : Math.mulDiv(assets, maxShares, maxAssets, Math.Rounding.Ceil); |
There was a problem hiding this comment.
Prevent withdraw(0) from consuming redeem dust.
Because _asyncMaxWithdraw() rounds down, maxAssets can be 0 while maxShares is still positive. With the new shortcut on Line 107, withdraw(0, ...) now consumes the entire remaining claim for 0 assets instead of behaving as a no-op/revert. The fast path needs to exclude the one-sided-zero case and only bypass mulDiv when the withdrawable side is nonzero.
Suggested fix
uint256 maxAssets = _asyncMaxWithdraw(controller);
uint256 maxShares = _asyncMaxRedeem(controller);
- uint256 shares = assets == maxAssets
- ? maxShares
- : Math.mulDiv(assets, maxShares, maxAssets, Math.Rounding.Ceil);
+ uint256 shares = maxAssets == 0
+ ? 0
+ : assets == maxAssets
+ ? maxShares
+ : Math.mulDiv(assets, maxShares, maxAssets, Math.Rounding.Ceil);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/token/ERC20/extensions/ERC7540DelayRedeem.sol` around lines 100 -
109, The fast-path that returns maxShares when assets == maxAssets incorrectly
triggers for withdraw(0,...) when maxAssets == 0; update the shortcut in the
shares calculation to only bypass Math.mulDiv when the withdrawable side is
non-zero (e.g., change the condition to assets == maxAssets && maxAssets != 0),
otherwise set shares to 0 or fall through to safe handling to avoid consuming
redeem dust; edit the expression that computes shares (using _asyncMaxWithdraw,
_asyncMaxRedeem, maxAssets, maxShares) accordingly.
|
I'm quite confuse by what this change implies. I designed the My understanding is that this PR considers a case where "the external function is modified (overriden) but the internal is not, and so using the internal is the thing to do". I just can't wrap my head around what it means to override the external instead of the internal, and having two behavior cohexist in the same contract. When there is no override the propose change are fine. They don't modify the behavior at all. What I'm trying to get to is, why would someone override the external instead of the internal, and what would be the best behavior in that case. |
2 participants
Summary by CodeRabbit