[DRAFT] feat(POC) rewards on redemption - #530
Draft
juliaaschmidt wants to merge 3 commits into
Draft
Conversation
Introduce MaxRedeemableETHLockedEvent so queue users keep earning rewards until their slice's underlying ETH becomes inactive. Lock triggers: - full CL exits at exit_epoch - partial withdrawals at PendingPartialWithdrawal.withdrawable_epoch - deposit→redeem rebalances at the rebalance moment Each lock event records a per-slice maxRedeemableEth cap pegged to the rate at lock time. _reportWithdrawToRedeemManager now exchanges at min(currentRate, cap); rate excess stays with active LsETH holders. Override semantics: for post-upgrade slices the lock cap replaces the request-time maxRedeemableEth (so users see queue-time appreciation). Pre-upgrade requests are unchanged — no lock event ever covers their range, so they fall back to the per-request snapshot. Claim recursion now walks three independent height stacks (requests, lock events, withdrawal events), sub-splitting on whichever boundary comes first. NextLockHeight is bootstrapped to RedeemDemand at the V1_3 init so pre-upgrade slices are never inadvertently covered.
…ally Drop the MaxRedeemableETHLockedDemand stored counter (and its SetMaxRedeemableETHLockedDemand event) in favor of computing the value from NextLockHeight, the current matching head, and the first lock event's height. The stored counter assumed the lock-covered region was contiguous with the queue head — incorrect when pre-upgrade requests sit ahead of the first lock event. Rewrite getEffectiveCapForDemand as a walk over MaxRedeemableETHLockedStack that correctly handles gaps (pre-upgrade tail or slices not yet locked) by falling back to per-request maxRedeemableEth for those regions. Update lockMaxRedeemableETH's over-lock guard to use the height-based check NextLockHeight + _lsETHToLock <= head + RedeemDemand, which is position-aware and independent of any stored demand counter. Remove the now-unused reportWithdraw locked-demand decrement.
Add 15 tests covering the new MaxRedeemableETH lock mechanism: - lockMaxRedeemableETH state mutation (asserts no swap, no burn) - override-not-min cap semantics at claim - pre-upgrade tail behavior (no retroactive value transfer) - multi-event-per-request claim recursion across rates R1<R2<R3 - interleaved lock + withdrawal event boundaries - getEffectiveCapForDemand aggregation across multiple lock events and pre-upgrade fallback regions - over-lock revert path on both inactivity and rebalance triggers - rebalance-sourced lock attribution - two lock events in one report - reportWithdraw's effect on the dynamically-computed locked demand - pre-upgrade tail at reportWithdraw A _submitAsPreUpgrade helper simulates the pre-V1_3 queue state via direct NextLockHeight storage manipulation so post-upgrade tests can reproduce the mixed pre/post case.
Contributor
|
Warning Review limit reached
More reviews will be available in 9 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
juliaaschmidt
marked this pull request as draft
June 2, 2026 11:09
juliaaschmidt
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a “rewards-on-redemption” mechanism by introducing per-slice max-redeemable ETH caps (“lock events”) that can override per-request caps when validators become inactive or when deposit→redeem rebalancing occurs. The change extends the oracle report payload to carry “newly inactive” principal signals, adds new RedeemManager state for lock tracking, and updates River’s redeem-manager funding logic and tests alongside a version bump to 1.4.0.
Changes:
- Added RedeemManager lock-event storage (
MaxRedeemableETHLockedStack,NextLockHeight) plus new APIs (lockMaxRedeemableETH,getEffectiveCapForDemand, etc.) and integrated lock-aware claiming. - Extended oracle reporting (
IOracleManagerV1) and OracleManager flow to persist and act on “newly inactive” principal, with River implementing the hook that appends lock events. - Added extensive test coverage for rewards-on-redemption behavior and bumped contract/test version assertions to
1.4.0.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/src/RedeemManager.1.sol | Adds lock-event state, cap aggregation, lock-aware claim logic, and a V1_3 initializer. |
| contracts/src/River.1.sol | Uses effective caps in redeem-manager funding flow and emits lock events for inactivity/rebalancing slices. |
| contracts/src/components/OracleManager.1.sol | Persists new “newly inactive” fields and calls a new hook to lock caps. |
| contracts/src/interfaces/IRedeemManager.1.sol | Exposes new lock/cap APIs and events/errors for the new mechanism. |
| contracts/src/interfaces/components/IOracleManager.1.sol | Extends report structs with “newly inactive” principal fields (plus docs). |
| contracts/src/state/redeemManager/NextLockHeight.sol | New unstructured storage helper for tracking the next lock-event height. |
| contracts/src/state/redeemManager/MaxRedeemableETHLockedStack.sol | New unstructured storage helper for lock-event stack. |
| contracts/test/RedeemManager.1.t.sol | Adds a dedicated rewards-on-redemption test suite plus version bump. |
| contracts/test/River.1.t.sol | Updates version assertion to 1.4.0. |
| contracts/test/components/OracleManager.1.t.sol | Adds an override hook emission to test the new lock hook wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+79
to
+81
| function initializeRedeemManagerV1_3() external init(2) { | ||
| NextLockHeight.set(RedeemDemand.get()); | ||
| } |
Comment on lines
+681
to
+685
| // For lock-covered slices the lock event is the active cap; do not drain per-request | ||
| // maxRedeemableEth (it remains as the fallback for any non-lock-covered tail). | ||
| // For non-lock-covered slices, preserve existing semantics — decrement by the paid amount. | ||
| if (!_params.hasLockEvent) { | ||
| _params.redeemRequest.maxRedeemableEth -= vars.ethAmount; |
Comment on lines
+536
to
+540
| // ETH amount bounded by the effective per-slice cap (lock events + per-request fallback) | ||
| uint256 capEth = redeemManager_.getEffectiveCapForDemand(suppliedRedeemManagerDemand); | ||
| // exchange at min(current rate, cap) — when the rate has appreciated past the cap, the | ||
| // excess never leaves River and stays with active LsETH holders | ||
| uint256 suppliedRedeemManagerDemandInEth = LibUint256.min(currentRateEth, capEth); |
Comment on lines
+898
to
+902
| function _aggregateUnlockedMaxRedeemableEth(uint256 startHeight, uint256 amount) | ||
| internal | ||
| view | ||
| returns (uint256) | ||
| { |
Comment on lines
+161
to
+164
| // ETH-denominated principal from validators that reached exit_epoch since the previous accepted report | ||
| // drives the inactivity portion of the MaxRedeemableETHLockedEvent emitted in this report | ||
| // bounded on-chain by the delta of validatorsExitedBalance vs the previous report | ||
| uint256 newlyInactiveExitedETH; |
Comment on lines
+165
to
+169
| // ETH-denominated principal from partial withdrawals (EIP-7002 / Pectra 0x02 sweeps) that reached | ||
| // their PendingPartialWithdrawal.withdrawable_epoch since the previous accepted report — i.e. the slot | ||
| // when process_withdrawals decrements the validator's balance and queues the sweep | ||
| // bounded on-chain by the per-report partial-withdrawal principal tracker (Pectra-era field) | ||
| uint256 newlyInactivePartialWithdrawalPrincipal; |
Comment on lines
+2475
to
+2486
| // Stash NextLockHeight, set to 0 to simulate the pre-V1_3 state, submit, then restore. | ||
| // We also have to re-bootstrap NextLockHeight to the new RedeemDemand after the submission | ||
| // to mirror what V1_3 init would have done at upgrade time. | ||
| bytes32 nextLockHeightSlot = bytes32(uint256(keccak256("river.state.nextLockHeight")) - 1); | ||
| uint256 saved = uint256(vm.load(address(redeemManager), nextLockHeightSlot)); | ||
| vm.store(address(redeemManager), nextLockHeightSlot, bytes32(uint256(0))); | ||
| id = _submitRequest(user, amount); | ||
| // After the "pre-upgrade" request lands, set NextLockHeight to current RedeemDemand | ||
| // (this is what initializeRedeemManagerV1_3 would have set at upgrade). | ||
| vm.store(address(redeemManager), nextLockHeightSlot, bytes32(redeemManager.getRedeemDemand())); | ||
| // suppress unused-var warning | ||
| saved; |
Comment on lines
+350
to
+356
| uint256 head = _currentMatchingHead(); | ||
| uint256 queueEnd = head + RedeemDemand.get(); | ||
| uint256 nextLockHeight = NextLockHeight.get(); | ||
| if (nextLockHeight + _lsETHToLock > queueEnd) { | ||
| uint256 unlockedHead = queueEnd > nextLockHeight ? queueEnd - nextLockHeight : 0; | ||
| revert LockExceedsRedeemDemand(_lsETHToLock, unlockedHead); | ||
| } |
Member
|
@mischat-galaxy @julia-galaxy I tried to update this branch against its base branch, but GitHub reported merge conflicts. I have not attempted conflict resolution. |
Member
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces a new contract, updates existing workflow triggers, and makes minor versioning and logic improvements. The most significant change is the addition of the
AttestationVerifierV1contract, which handles attestation quorum and BLS deposit message validation for River, helping to keep River's bytecode size manageable. Additionally, several GitHub Actions workflows are updated to trigger on thev2/pectrabranch, and a small logic fix and version bump are included.Key changes:
New Features
AttestationVerifierV1contract incontracts/src/AttestationVerifier.1.sol. This contract is responsible for validating attestation quorums and BLS deposit messages on behalf of River, including admin and quorum management, EIP-712 domain separation, and BLS verification logic. This modularizes the attestation logic, keeping River's bytecode under EIP-170 limits.Workflow and CI/CD Updates
Coverage.yaml,Deploy.yaml,Format.yaml,HeavyTests.yaml,Lint.yaml,Mythril.yaml,Tests.yaml,foundry-gas-diff.yml) to trigger on pushes and pull requests to thev2/pectrabranch in addition tomain. This ensures CI runs for both main and the new feature branch. [1] [2] [3] [4] [5] [6] [7] [8] [9]foundry-gas-diff.yml, changed the base branch for gas diff comparisons to dynamically use the PR base or current branch, improving accuracy of gas usage reports.Certora.yamlto explicitly specify themainbranch for workflow triggers.Contract Logic and Versioning
RiverV1Harness.sol, improved consolidation buffer handling by pulling funds from the consolidation coverage fund if the buffer is greater than zero and updating the buffer accordingly.Allowlist.1.solfrom1.2.1to1.3.0.Notice
Pull Request Type
Breaking changes (if applicable)
Testing
Manual tests (if applicable)
Additional comments