ERC-7540: Epoch-based fulfillment strategy#230
Conversation
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>
|
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:
WalkthroughThree new contracts are introduced to implement ERC-7540 epoch-based asynchronous deposits and redemptions. An abstract Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant ERC7540EpochDeposit
participant EpochQueue
participant Fulfillment
Controller->>ERC7540EpochDeposit: requestDeposit(assets)
ERC7540EpochDeposit->>ERC7540EpochDeposit: Calculate currentDepositEpoch
ERC7540EpochDeposit->>ERC7540EpochDeposit: Record assets to epoch.requests
ERC7540EpochDeposit->>EpochQueue: Enqueue epochId if new (respecting _requestQueueLimit)
Note over ERC7540EpochDeposit: Deposit request batched by epoch
Fulfillment->>ERC7540EpochDeposit: _fulfillDeposit(epochId, totalShares)
ERC7540EpochDeposit->>ERC7540EpochDeposit: Set epoch.totalShares (now claimable)
Note over ERC7540EpochDeposit: Epoch becomes eligible for claims
Controller->>ERC7540EpochDeposit: claimDeposit() or maxClaimableDeposit()
loop For each queued epoch
ERC7540EpochDeposit->>ERC7540EpochDeposit: Check if totalShares > 0 (fulfilled)
ERC7540EpochDeposit->>ERC7540EpochDeposit: Pro-rata convert: shares = (requests × totalShares) / totalAssets
ERC7540EpochDeposit->>ERC7540EpochDeposit: Reduce epoch.requests and totalAssets
end
ERC7540EpochDeposit->>Controller: Return minted shares
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/ERC7540EpochDeposit.sol`:
- Around line 183-188: Compute batchShares using the pre-mutation epoch totals
(use details.totalAssets and details.totalShares before any -= on
details.totalAssets/details.totalShares/requests[controller]/assets) so
conversions use the locked epoch rate; i.e., calculate batchShares =
batchAssets.mulDiv(details.totalShares, details.totalAssets,
Math.Rounding.Floor) before you decrement details.requests[controller],
details.totalAssets, details.totalShares, and assets, and then perform the
saturatingSub-style decrements after that calculation (apply same fix for the
other similar block around lines 211-217).
- Around line 58-74: The epoch fulfilment sentinel must not rely on totalShares
== 0 because _fulfillDeposit can legitimately set totalShares to 0; add an
explicit boolean (e.g., EpochDepositMetadata.fulfilled) to the
EpochDepositMetadata struct, set it to true in _fulfillDeposit, and update
_pendingDepositRequest, _claimableDepositRequest, and _assetsToFulfillDeposit to
check details.fulfilled (false = pending, true = claimable) instead of comparing
totalShares; also ensure any logic that previously relied on totalShares==0 is
adjusted to use the new fulfilled flag and that _fulfillDeposit still records
totalShares correctly while marking fulfilled.
In `@contracts/token/ERC20/extensions/ERC7540EpochRedeem.sol`:
- Around line 59-75: The code treats details.totalAssets == 0 as the sole
sentinel for "fulfilled", which breaks when _fulfillRedeem writes totalAssets ==
0; to fix, add an explicit fulfilled flag to EpochRedeemMetadata (e.g., bool
fulfilled), set details.fulfilled = true inside _fulfillRedeem whenever the
epoch is processed (even if totalAssets == 0), and change _pendingRedeemRequest
and _claimableRedeemRequest to check details.fulfilled (pending when !fulfilled,
claimable when fulfilled). Also update any logic in _sharesToFulfillRedeem and
other spots that currently rely on totalAssets == 0 to use the new fulfilled
flag while still reading totalAssets for amounts.
- Around line 187-193: The batch share conversion uses mutated epoch totals
causing divide-by-zero and wrong rates; before mutating details.totalAssets or
details.totalShares, snapshot the epoch totals (e.g., uint256 epochAssets =
details.totalAssets; uint256 epochShares = details.totalShares) and compute
batchShares = batchAssets.mulDiv(epochShares, epochAssets, Math.Rounding.Floor)
using those snapshots, then decrement details.totalAssets/details.totalShares
(and details.requests[controller]) with saturating subtraction to avoid
underflow; apply the same change to the other conversion block that mirrors this
logic.
🪄 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: 421b1500-d74a-47ef-8a85-bf011a63b917
📒 Files selected for processing (3)
contracts/mocks/token/ERC7540EpochMock.solcontracts/token/ERC20/extensions/ERC7540EpochDeposit.solcontracts/token/ERC20/extensions/ERC7540EpochRedeem.sol
…ent events
Splits the combined require in `_fulfillDeposit` / `_fulfillRedeem` into
three typed errors (`*TooEarly`, `*EmptyEpoch`, `*AlreadyFulfilled`) so
callers can distinguish failure modes, and emits
`Epoch{Deposit,Redeem}Fulfilled` on success.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Folds the drained-epoch case (`totalAssets == 0` for deposit /
`totalShares == 0` for redeem) into the normal consume flow rather than
short-circuiting with a separate branch:
* Ternaries on `requested` and the cross-dim mulDiv yield 0 in the
drained state, so the iteration pops the queue without consuming
user input or hitting a division-by-zero.
* The storage decrement on `requests[controller]` subtracts the full
stored slot when fully claimed, clearing stuck dust in the same line
that does the normal-case decrement.
* `saturatingSub` absorbs the 1-wei ceil/floor excess in the cross-dim
paths into the shared totals, so `totalAssets`/`totalShares` reach 0
cleanly and the {_fulfill*} `EmptyEpoch` guard stays unambiguous.
`_pending{Deposit,Redeem}Request` now additionally check the opposite
total to distinguish "pending" from "fully-claimed post-fulfillment".
`_asyncMax{Mint,Withdraw}` skip drained epochs to avoid mulDiv revert.
NOTE in both files documents the residual-dust bound, the example case
that triggers it, why this isn't an ERC-4626 inflation attack (the
per-epoch totals are donation-proof), and {_decimalsOffset} as the knob
for finer per-claim granularity.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Worth flagging that we still don't have a way to observe the epochs a user has pending requests in — the |
luiz-lvj
left a comment
There was a problem hiding this comment.
Hey @ernestognw, what is the expected behavior if there are two epochs in the queue and the admin, by mistake or something else, calls _fulfillDeposit on the newer epoch first? As of now, it would result in losses of the oldest epoch, which is not desired. This happens because:
- _consumeClaimable* consumes .front() (oldest) without checking it's fulfilled.
- _asyncMax* sums _claimableDepositRequest across the whole queue, skipping pending epochs but continuing to count fulfilled ones behind them.
Imo we could either revert if the admin tries to do that or only ever look at the contiguous fulfilled prefix (which I prefer). If the current behavior is intended we should at least document it.
Hey @luiz-lvj, thanks for the review, this is a great finding. I believe we just didn't consider an out of order fulfillment because it's arguably on the integrator side but it's true that we can make it safe from the base layer. I believe the right behavior for the I updated accordingly and added tests for these cases. |
Summary
ERC7540EpochDepositandERC7540EpochRedeem— epoch-based batch fulfillment strategies for ERC-7540 async vaultsERC7540EpochMockfor testingERC7540and specific implementations #228 to reduce complexity of the base PRProduction equivalents: Cove, Amphor, Lagoon.
Known pending work
requirestatements in_fulfillDeposit/_fulfillRedeemwith custom errors — typed errors*TooEarly,*EmptyEpoch,*AlreadyFulfilledare now thrown_fulfillDeposit/_fulfillRedeem—ERC7540Epoch{Deposit,Redeem}Fulfilledemitted with the locked ratesaturatingSubis needed for the subtraction operations in_consumeClaimable*loops — resolved viasaturatingSub+ drained-epoch handling; residual-dust bound is documented as a NOTE in both contracts_epochstotals —totalDepositAssets/totalDepositSharesandtotalRedeemAssets/totalRedeemSharesexpose the per-epoch state_memberOf(per-controller epoch queue) — depends on Addvalues(start, end)slice accessor toDoubleEndedQueueopenzeppelin-contracts#6522 for paginatedDoubleEndedQueueaccessERC7540EpochDeposit.test.js(435 lines) andERC7540EpochRedeem.test.js(410 lines)Test plan
_requestQueueLimitenforcement (32 default)shouldSupportInterfacesin the includedshouldBehaveLikeERC7540{Operator,Deposit,Redeem}helpers🤖 Generated with Claude Code
Summary by CodeRabbit