Add abstract ERC7540 and specific implementations#228
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:
WalkthroughAdds ERC-7540 asynchronous vault support comprising an interface specification, abstract base implementation supporting both synchronous and asynchronous deposit/redeem flows with operator authorization, and three concrete async strategies using admin fulfillment and time-delayed checkpoints. Changes
Sequence DiagramsequenceDiagram
actor Owner
participant Vault as ERC7540 Vault
participant Controller
participant Operator
Owner->>Vault: requestDeposit(assets, controller, owner)
Vault->>Vault: _requestDeposit(assets, controller, owner)
Vault->>Vault: Record pending assets
Note over Vault: Async processing delay/admin fulfillment
Vault->>Vault: Process request (fulfill/delay completion)
Vault->>Vault: Move assets to claimable state
Owner->>Vault: deposit(assets, receiver, controller)<br/>or claim()
Vault->>Vault: _deposit(caller, receiver, assets, shares)
Vault->>Vault: Decrement claimable, mint shares
Vault-->>Owner: Return shares minted
Operator->>Vault: setOperator(operator, approved)
Vault->>Vault: Update _isOperator mapping
Vault-->>Operator: Emit OperatorSet event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 6
🧹 Nitpick comments (3)
contracts/token/ERC20/extensions/ERC7540DelayDeposit.sol (1)
75-81: Potential underflow in_asyncMaxDepositand_asyncMaxMint.Same concern as in
ERC7540DelayRedeem: these functions compute a subtraction without overflow protection. Consider usingMath.saturatingSubfor defensive safety.♻️ Proposed fix
function _asyncMaxDeposit(address owner) internal view virtual override returns (uint256) { - return _deposits[owner].latest() - _claimedDeposits[owner]; + return Math.saturatingSub(_deposits[owner].latest(), _claimedDeposits[owner]); } function _asyncMaxMint(address owner) internal view virtual override returns (uint256) { - return _deposits[owner].latest() - _claimedDeposits[owner]; + return Math.saturatingSub(_deposits[owner].latest(), _claimedDeposits[owner]); }🤖 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 75 - 81, The _asyncMaxDeposit and _asyncMaxMint functions subtract _claimedDeposits[owner] from _deposits[owner].latest() without guarding against underflow; replace the raw subtraction in both _asyncMaxDeposit and _asyncMaxMint with a saturating subtraction using Math.saturatingSub(_deposits[owner].latest(), _claimedDeposits[owner]) so the result floors at zero and prevents underflow when claimed exceeds latest.contracts/token/ERC20/extensions/ERC7540DelayRedeem.sol (2)
80-86: Potential underflow in_asyncMaxWithdrawand_asyncMaxRedeem.Both functions compute
_redeems[owner].latest() - _claimedRedeems[owner]without overflow protection. While normal operation should maintainclaimed <= latest, usingMath.saturatingSubwould provide defensive safety against edge cases.♻️ Proposed fix
function _asyncMaxWithdraw(address owner) internal view virtual override returns (uint256) { - return _redeems[owner].latest() - _claimedRedeems[owner]; + return Math.saturatingSub(_redeems[owner].latest(), _claimedRedeems[owner]); } function _asyncMaxRedeem(address owner) internal view virtual override returns (uint256) { - return _redeems[owner].latest() - _claimedRedeems[owner]; + return Math.saturatingSub(_redeems[owner].latest(), _claimedRedeems[owner]); }🤖 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 80 - 86, The two functions _asyncMaxWithdraw and _asyncMaxRedeem can underflow when computing _redeems[owner].latest() - _claimedRedeems[owner]; change both to read the values into locals (e.g., uint256 latest = _redeems[owner].latest(); uint256 claimed = _claimedRedeems[owner];) and return Math.saturatingSub(latest, claimed) so subtraction is safe; ensure Math is imported/available in the contract scope.
55-78: Edge case:requestId = 0may produce unexpected results.When
requestId = 0, the conditionrequestId > clock()is always false (sinceclock()returns a positive timestamp). This means:
_pendingRedeemRequest(0, controller)returns0_claimableRedeemRequest(0, controller)calculates based onupperLookup(0)which may return unexpected valuesIf
requestId = 0is a valid sentinel value in this design, consider documenting this behavior. If not, consider adding validation.🤖 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 55 - 78, Both _pendingRedeemRequest and _claimableRedeemRequest mishandle requestId == 0 (upperLookup(0) can be unexpected); add an explicit guard for requestId == 0 in these functions — either require(requestId != 0) if 0 is invalid, or return 0 early when requestId == 0 if it should be treated as a sentinel — and update logic that uses clock(), _redeems[controller].upperLookup(timepoint), and _claimedRedeems[controller] accordingly so upperLookup is never called with 0 or the zero-case behavior is explicit and documented.
🤖 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/ERC7540.sol`:
- Around line 292-310: The mint function inconsistently checks maxMint using
_msgSender() in async mode; change the async branch to use controller instead of
_msgSender() for the maxMint check and in the ERC4626ExceededMaxMint revert
argument so it mirrors deposit behavior (use _isDepositAsync() ? controller :
receiver for the target passed to maxMint and the revert), keeping the rest of
the logic that computes assets (which already uses
maxDeposit(controller)/maxMint(controller)) and the call to _deposit unchanged;
update references in the mint function to use controller when _isDepositAsync()
is true.
- Around line 266-284: In the deposit function, avoid a division-by-zero when
calling Math.mulDiv by validating that maxDeposit(controller) and
maxMint(controller) are non-zero before using them: check (when
_isDepositAsync() is true) that maxAssets != 0 and that both
maxDeposit(controller) and maxMint(controller) are > 0, and revert with a clear
error (e.g., "ERC7540: zero maxDeposit/maxMint") if any are zero; otherwise
proceed to compute shares (keeping the existing branches that call Math.mulDiv
or previewDeposit) so async deposits don't hit a silent division-by-zero in
Math.mulDiv.
In `@contracts/token/ERC20/extensions/ERC7540AdminFulfillDeposit.sol`:
- Around line 47-51: The _deposit override is updating claimable state using
receiver but the rest of the flow (_requestDeposit, _fulfillDeposit) records
claimable amounts under controller, causing accounting drift when receiver !=
controller; change the implementation so _deposits is decremented for the
controller identifier instead of receiver (or ensure controller is forwarded
into this hook), i.e., make _deposit use the same controller key used by
_requestDeposit/_fulfillDeposit (or add/pass an extra internal parameter/hook to
obtain controller) so that _deposits[controller].claimableAssets and
.claimableShares are correctly saturated and then call super._deposit.
In `@contracts/token/ERC20/extensions/ERC7540AdminFulfillRedeem.sol`:
- Around line 46-56: The _withdraw implementation incorrectly updates redeem
tracking for the destination `receiver` instead of the requesting controller;
change the two lines that adjust `_redeems[...].claimableAssets` and
`claimableShares` to use `owner` (the ownerOrController/controller in async
mode) rather than `receiver`, so `_withdraw` aligns with `_requestRedeem` and
`_fulfillRedeem` state tracking and then call `super._withdraw(caller, receiver,
owner, assets, shares)` as before.
In `@contracts/token/ERC20/extensions/ERC7540DelayDeposit.sol`:
- Around line 45-48: The _deposit override increments _claimedDeposits[receiver]
but deposit requests and queries use the controller address (see _requestDeposit
and ERC7540AdminFulfillDeposit), causing mismatched accounting when receiver !=
controller; fix by updating _deposit to increment _claimedDeposits[controller]
(or, if async deposits require receiver to equal controller, add a
require(receiver == controller) check) so the same address is used for tracking;
locate the _deposit function and make the change to use the controller key (or
add the equality guard) and keep the subsequent super._deposit call unchanged.
In `@contracts/token/ERC20/extensions/ERC7540DelayRedeem.sol`:
- Around line 25-27: The override currently sets _isDepositAsync() to true but
this contract implements delayed redeem, so replace that override with an
override of _isRedeemAsync() returning true (and remove or revert the
_isDepositAsync override) so the contract signals async redeem mode; update the
function signature to "function _isRedeemAsync() internal pure virtual override
returns (bool)" and return true to enable async redeem behavior used by
requestRedeem.
---
Nitpick comments:
In `@contracts/token/ERC20/extensions/ERC7540DelayDeposit.sol`:
- Around line 75-81: The _asyncMaxDeposit and _asyncMaxMint functions subtract
_claimedDeposits[owner] from _deposits[owner].latest() without guarding against
underflow; replace the raw subtraction in both _asyncMaxDeposit and
_asyncMaxMint with a saturating subtraction using
Math.saturatingSub(_deposits[owner].latest(), _claimedDeposits[owner]) so the
result floors at zero and prevents underflow when claimed exceeds latest.
In `@contracts/token/ERC20/extensions/ERC7540DelayRedeem.sol`:
- Around line 80-86: The two functions _asyncMaxWithdraw and _asyncMaxRedeem can
underflow when computing _redeems[owner].latest() - _claimedRedeems[owner];
change both to read the values into locals (e.g., uint256 latest =
_redeems[owner].latest(); uint256 claimed = _claimedRedeems[owner];) and return
Math.saturatingSub(latest, claimed) so subtraction is safe; ensure Math is
imported/available in the contract scope.
- Around line 55-78: Both _pendingRedeemRequest and _claimableRedeemRequest
mishandle requestId == 0 (upperLookup(0) can be unexpected); add an explicit
guard for requestId == 0 in these functions — either require(requestId != 0) if
0 is invalid, or return 0 early when requestId == 0 if it should be treated as a
sentinel — and update logic that uses clock(),
_redeems[controller].upperLookup(timepoint), and _claimedRedeems[controller]
accordingly so upperLookup is never called with 0 or the zero-case behavior is
explicit and documented.
🪄 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: bf501ffd-485e-4bd8-bc88-42d485ad4dd7
📒 Files selected for processing (8)
contracts/interfaces/IERC7540.solcontracts/token/ERC20/extensions/ERC7540.solcontracts/token/ERC20/extensions/ERC7540AdminFulfillDeposit.solcontracts/token/ERC20/extensions/ERC7540AdminFulfillRedeem.solcontracts/token/ERC20/extensions/ERC7540DelayDeposit.solcontracts/token/ERC20/extensions/ERC7540DelayRedeem.sollib/@openzeppelin-contractslib/@openzeppelin-contracts-upgradeable
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_computeAsyncMint was subtracting shares from an assets-denominated field, and _computeAsyncWithdraw was subtracting assets from a shares-denominated field. Move the decrement after the unit conversion so the correct value is used. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- _redeemRedeemShareDestination → _redeemShareDestination - _assetsToFullfillDeposit → _assetsToFulfillDeposit - _sharesToFullfillReedem → _sharesToFulfillRedeem Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These functions mutate state (epoch totals, queue pops, request decrements), so "consume" better signals the non-pure nature. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Consolidate claim logic that was previously split between _consumeClaimable* (ratio computation) and _deposit/_withdraw overrides (bookkeeping) into a single _consumeClaimable* hook per subclass. This fixes incorrect receiver-vs-controller keying in Admin and Delay bookkeeping and makes the base ERC7540 stubs revert NotImplemented, consistent with other abstract hooks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ernestognw
changed the title
ERC7540 and specific implementations
| function _pendingDepositRequest( | ||
| uint256 /*requestId*/, | ||
| address /*controller*/ | ||
| ) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _claimableDepositRequest( | ||
| uint256 /*requestId*/, | ||
| address /*controller*/ | ||
| ) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _pendingRedeemRequest( | ||
| uint256 /*requestId*/, | ||
| address /*controller*/ | ||
| ) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _claimableRedeemRequest( | ||
| uint256 /*requestId*/, | ||
| address /*controller*/ | ||
| ) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _consumeClaimableDeposit(uint256 /*assets*/, address /*controller*/) internal virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _consumeClaimableMint(uint256 /*shares*/, address /*controller*/) internal virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _consumeClaimableWithdraw(uint256 /*assets*/, address /*controller*/) internal virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _consumeClaimableRedeem(uint256 /*shares*/, address /*controller*/) internal virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _asyncMaxDeposit(address /*owner*/) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _asyncMaxMint(address /*owner*/) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _asyncMaxWithdraw(address /*owner*/) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function _asyncMaxRedeem(address /*owner*/) internal view virtual returns (uint256) { | ||
| revert NotImplemented(); | ||
| } |
There was a problem hiding this comment.
Would it be better to leave all these functions as unimplemented virtuals rather than reverting?
There was a problem hiding this comment.
If we leave them as unimplemented, the devs will necessarily have to provide an implementation, even if they don't need one. For example, if you want to build a vault that is sync deposit & async redeem, and if there functions are not implemented, you'll have to provide an implementation for all the deposit & mint related function in this, even though you done need them.
An option could be to have ERC7540SyncDeposit and ERC7540SyncRedeem that you use to "fill the hole" when you want a sync behavior.
| _deposits[controller].claimableAssets = Math.saturatingSub(_deposits[controller].claimableAssets, assets); | ||
| _deposits[controller].claimableShares = Math.saturatingSub(_deposits[controller].claimableShares, shares); |
There was a problem hiding this comment.
| _deposits[controller].claimableAssets = Math.saturatingSub(_deposits[controller].claimableAssets, assets); | |
| _deposits[controller].claimableShares = Math.saturatingSub(_deposits[controller].claimableShares, shares); | |
| _deposits[controller].claimableAssets = Math.saturatingSub(_asyncMaxDeposit(controller), assets); | |
| _deposits[controller].claimableShares = Math.saturatingSub(_asyncMaxMint(controller), shares); |
There was a problem hiding this comment.
I think this is a common discussion that we've been facing many time:
When we have some state, a getter for that state, and an update function that modifies the state, should the modify function update what is storage, or what is returned by the getter ?
In the case of ERC20, the balances are updated from storage, and not from balanceOf (which may be overriden).
One of the reason we do things that way, is that if the override on the getter does some change like
function balanceOf(address account) public virtaul override return (uint256) {
return super.balanceOf(account) - _frozen[account];
}then doing the update using the getter function as a reference would "burn" the frozen amount, and would do that each update.
AFAIK, the only "solution" would be to have an internal setter that could be overriden alongside the getter, but that would mean having an internal function that directly sets the storage, which is something that goes against our guidelines.
There was a problem hiding this comment.
Good point, this has indeed come up before. Here's where I land on the getter vs setter question:
Getter (virtual read function): I think providing one is worthwhile. Without it, a developer who wants to change how the value is computed must override every function that reads the private variable, often duplicating logic (e.g. copy, paste, modify). A virtual getter gives a single override point for consistent behavior across all read sites. Not providing one hurts the developer experience more than providing one.
Setter (virtual write function): I'm less convinced this is needed. In practice, a developer overriding the getter is almost certainly introducing their own storage variable. At that point, the original setter writes to a variable the getter no longer reads — it becomes dead code. You could override the setter too to write the new variable, but that feels like unnecessary indirection when the new variable is already directly accessible in the subcontract.
That said, I acknowledge your ERC20 balanceOf example: if a getter override subtracts a frozen amount, writing back the getter's return value double-counts the subtraction on each update. However, I think in this case it shouldn't be an issue because we're using internal getters, and I'd prefer the consistency of having a single read override function
There was a problem hiding this comment.
I'm not seeing the difference between this and the ERC20 example. IMO, the internal getter is just "sugar syntax" for handling the 7540 sync/async mess, and its basically the internal equivalent of maxDeposit/maxMint ?
Are you saying that if someone wanted to replicate the balanceOf override that I described just above, they would do it on the public maxDeposit and not on the internal _asyncMaxDeposit ? I'd say both overrides are equally liquelly, and would result in very different behavior. Advanced users would liquelly spot that, but I'm not sure put them in a situation where they have to worry about that.
Do you have an concrete example where using the internal getter (instead of the sotrage slot) would improve the override experience ? I can't think of any.
| _redeems[controller].claimableAssets = Math.saturatingSub(_redeems[controller].claimableAssets, assets); | ||
| _redeems[controller].claimableShares = Math.saturatingSub(_redeems[controller].claimableShares, shares); |
There was a problem hiding this comment.
| _redeems[controller].claimableAssets = Math.saturatingSub(_redeems[controller].claimableAssets, assets); | |
| _redeems[controller].claimableShares = Math.saturatingSub(_redeems[controller].claimableShares, shares); | |
| _redeems[controller].claimableAssets = Math.saturatingSub(_asyncMaxWithdraw(controller), assets); | |
| _redeems[controller].claimableShares = Math.saturatingSub(_asyncMaxRedeem(controller), shares); |
…pelin/openzeppelin-community-contracts into feat/erc-7540-standalone
| _transfer(_depositShareOrigin(), receiver, shares); | ||
| } | ||
|
|
||
| emit Deposit(caller, receiver, assets, shares); |
There was a problem hiding this comment.
The spec states the following:
When the
Depositevent is emitted, the first parameter MUST be thecontroller, and the second parameter MUST be thereceiver.
Here we're emitting with the caller, which is _msgSender() in both calls to this internal _deposit function.
Maybe we should pass in the controller as an argument? @Amxx
There was a problem hiding this comment.
If _isDepositAsync, then caller is not used. We could refactor caller into callerOrController
Then the calls on line 423 and 459 would become
_deposit(_isDepositAsync() ? controller, _msgSender(), receiver, assets, shares);I honestly though I had done that already. I may have failled to push that, or maybe it was ovewritten by some commit. Anyway, this would be an option that doesn't introduce an extra variable.
| * Reports support for {IERC7540Operator} unconditionally. Support for {IERC7540Deposit} and | ||
| * {IERC7540Redeem} is conditional on the corresponding async selector returning `true`. | ||
| */ | ||
| function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { |
There was a problem hiding this comment.
Shall this report IERC4626 too?
There was a problem hiding this comment.
Vanila's ERC4626 does not, so I wouldn't
(its not part of ERC-4626 specs)
Amxx
force-pushed
the
feat/erc-7540-standalone
branch
from
4e9bdce to
d4260b9
Compare
ERC-7540 mandates that pendingDepositRequest, claimableDepositRequest, pendingRedeemRequest and claimableRedeemRequest MUST NOT revert except on integer overflow. Switch the sync-side branch from reverting with ERC7540SyncDeposit/ERC7540SyncRedeem to returning 0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Trace208 structure used to track requests requires non-decreasing keys. Note that overrides MUST keep the maturity timepoint non-decreasing per controller, otherwise subsequent requests will revert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary by CodeRabbit
Release Notes