BYOV Exit Flow (BS-2454) #341
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request refactors validator exit request handling from accepting a single count to processing batched, per-operator allocations with keeper authorization. It introduces comprehensive validation (ordering, funding limits, demand consistency) and updates state management accordingly, while removing the previous validator selection logic. Changes
Sequence DiagramsequenceDiagram
participant Keeper as Keeper
participant Registry as OperatorsRegistry
participant River as RiverAddress
Keeper->>Registry: requestValidatorExits(allocations[])
activate Registry
Registry->>River: Verify keeper authorization
activate River
River-->>Registry: keeper confirmed
deactivate River
Registry->>Registry: Validate demand > 0
Registry->>Registry: Iterate allocations
loop For each allocation
Registry->>Registry: Verify operatorIndex ordering
Registry->>Registry: Check operator is active
Registry->>Registry: Validate exit count ≤ (funded - requested)
Registry->>Registry: Update operator.requestedExits
Registry->>Registry: Emit RequestedValidatorExits
end
Registry->>Registry: Verify totalExits ≤ currentDemand
Registry->>Registry: Reduce currentValidatorExitsDemand
Registry->>Registry: Update TotalValidatorExitsRequested
Registry->>Registry: Emit UpdatedTotalValidatorExitsRequested
deactivate Registry
Keeper-->>Keeper: Exit requests processed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/byov-v1-clean #341 +/- ##
======================================================
+ Coverage 94.90% 95.25% +0.34%
======================================================
Files 77 77
Lines 1807 1771 -36
Branches 244 241 -3
======================================================
- Hits 1715 1687 -28
+ Misses 32 31 -1
+ Partials 60 53 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/OperatorsRegistry.1.sol (1)
568-675:⚠️ Potential issue | 🔴 CriticalCI is failing: “Stack too deep” in
_setStoppedValidatorCounts.
This is a blocker. The compiler error points to Line 623, which is inside this function. Please refactor to reduce stack usage (e.g., split into smaller internal helpers, or move more state into a struct passed by reference). EnablingviaIRis an alternative if acceptable for your build.
🧹 Nitpick comments (2)
contracts/src/interfaces/IOperatorRegistry.1.sol (1)
358-362: Update the docstring to reflect explicit per-operator allocations.
The comment still mentions a selection algorithm, which no longer applies.✍️ Suggested doc tweak
- /// `@notice` The selection algorithm will pick validators based on their active validator counts - /// `@notice` This value is computed by using the count of funded keys and taking into account the stopped validator counts and exit requests + /// `@notice` Caller supplies explicit per-operator exit allocations (sorted by operator index) + /// `@notice` Total allocated exits must not exceed current exit demandcontracts/src/OperatorsRegistry.1.sol (1)
468-509: Avoid emitting no-op exit events for zero allocations.
Zero-count entries currently still touch storage and emitRequestedValidatorExits, which can be misleading and waste gas. Consider skipping them (or explicitly reverting) to keep the event stream clean.♻️ Possible refinement (skip zero-count entries)
- uint256 count = _allocations[i].validatorCount; - suppliedExitCount += count; + uint256 count = _allocations[i].validatorCount; if (i > 0 && !(operatorIndex > prevOperatorIndex)) { revert UnorderedOperatorList(); } prevOperatorIndex = operatorIndex; OperatorsV2.Operator storage operator = OperatorsV2.get(operatorIndex); if (!operator.active) { revert InactiveOperator(operatorIndex); } + if (count == 0) { + continue; + } + suppliedExitCount += count;
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@contracts/src/OperatorsRegistry.1.sol`:
- Around line 485-488: The revert ExitsRequestedExceedsFundedCount currently
passes operator.funded which is the total funded count instead of the available
exits; change the third argument to the available count (operator.funded -
operator.requestedExits) or compute a local variable like available =
operator.funded - operator.requestedExits and pass that to
ExitsRequestedExceedsFundedCount(operatorIndex, count, available) so the error
reports the correct available exits; update the revert call in the same block
where the if checks count > (operator.funded - operator.requestedExits).
- Around line 490-491: operator.requestedExits is being increased by casting
count (from OperatorAllocation.validatorCount, a uint256) to uint32 which can
truncate; add a guard to ensure count <= type(uint32).max before casting (revert
with a clear message) or use a SafeCast-to-uint32 utility to safely convert, or
change OperatorAllocation.validatorCount to uint32 at the type level; update the
logic around operator.requestedExits and the emit
RequestedValidatorExits(operatorIndex, operator.requestedExits) accordingly so
no silent truncation can occur.
🧹 Nitpick comments (3)
contracts/src/interfaces/IOperatorRegistry.1.sol (1)
199-208: Inconsistent error naming:ExitsRequestedExceedDemandvsExitsRequestedExceedsFundedCount.
ExitsRequestedExceedsFundedCountuses "Exceeds" (with 's'), whileExitsRequestedExceedDemanduses "Exceed" (without 's'). Consider renaming toExitsRequestedExceedsDemandfor consistency.📝 Suggested fix for naming consistency
- error ExitsRequestedExceedDemand(uint256 requested, uint256 demand); + error ExitsRequestedExceedsDemand(uint256 requested, uint256 demand);Note: This requires updating the corresponding revert statement in
OperatorsRegistry.1.solat line 496.contracts/src/OperatorsRegistry.1.sol (1)
494-497: Demand check occurs after per-operator state mutations.The loop at lines 468-492 modifies
operator.requestedExitsand emitsRequestedValidatorExitsevents before the demand check at line 495. While the entire transaction reverts on failure (so state is consistent), emitted events during a reverted transaction can cause confusion in monitoring systems that process pending transactions.This is a minor concern since reverted transactions don't persist events, but reordering the validation to happen before state mutations would be cleaner.
contracts/test/OperatorsRegistry.1.t.sol (1)
2717-2722: Consider moving event declarations to the test base class.The events
UpdatedRequestedValidatorExitsUponStoppedandSetCurrentValidatorExitsDemandare declared within theOperatorsRegistryV1TestDistributioncontract. Since these are also used in the main implementation, consider declaring them inOperatorsRegistryV1TestBasefor reuse across test contracts.
juliaaschmidt
left a comment
juliaaschmidt
left a comment
There was a problem hiding this comment.
LGTM ! 🔥 Great job!
2 participants
Description
This pull request updates the validator exit request mechanism in the operators registry to allow more granular, operator-specific exit requests and improves validation and testing for this process. The main changes include changing the
requestValidatorExitsfunction to accept explicit operator allocations, adding new error types for better input validation, and updating tests to reflect the new interface and logic.Validator exit request improvements
requestValidatorExitsfunction inOperatorsRegistryV1and its interface to accept an array ofOperatorAllocationstructs instead of a single count, allowing precise allocation of exit requests per operator. The function now validates that allocations are non-empty, ordered, do not exceed operator-funded counts, and do not exceed overall exit demand. [1] [2]IOperatorsRegistryV1for invalid exit requests:ExitsRequestedExceedsFundedCount,ExitsRequestedExceedsDemand, and improved input validation withInvalidEmptyArrayandUnorderedOperatorList.Testing and mocks
RiverMockcontract to support a keeper address and expose it viagetKeeper, supporting the new access control in exit requests. [1] [2]OperatorsRegistry.1.t.solto initialize and use the keeper address, and modified all tests to use the newrequestValidatorExitsinterface with operator allocations and the keeper as the caller. [1] [2] [3] [4] [5] [6] [7] [8]Notice
Pull Request Type
Breaking changes (if applicable)
The signature for the function
requestValidatorExitshave been changed.Testing
Summary by CodeRabbit
API Changes
Validation & Error Handling
Improvements
Access Control