feat(BS-2349) BYOV Deposit Flow #339
Open
Conversation
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
…it loop - Validate allocation operator indices are in ascending order - Use unchecked increment in deposit loop for gas savings - Update natspec for getNextValidatorsToDepositFromActiveOperators
Add scenario 6 to mock that returns half of requested keys and test that InvalidPublicKeyCount() is thrown when registry returns fewer keys than requested.
Test that OperatorAllocationsExceedCommittedBalance() is thrown when total requested validators exceed the maximum that can be funded.
… requested Add scenario 7 to mock that returns 4 keys regardless of request and test that InvalidPublicKeyCount() is thrown when registry returns more keys than requested but within maxDepositableCount.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the validator allocation system from accepting single count parameters to accepting arrays of operator-specific allocations, enabling per-operator distribution control. The change affects core registry functions, consensus layer deposits, River protocol, and their corresponding tests and Certora verification specs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
=======================================
Coverage 97.42% 97.43%
=======================================
Files 78 78
Lines 2327 2336 +9
Branches 255 261 +6
=======================================
+ Hits 2267 2276 +9
Misses 59 59
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
juliaaschmidt
added
enhancement
Add tests for UnorderedOperatorList and AllocationWithZeroValidatorCount error cases in depositToConsensusLayerWithDepositRoot to ensure invalid operator allocations are properly rejected.
Add tests for AllocationWithZeroValidatorCount and InactiveOperator error cases in getNextValidatorsToDepositFromActiveOperators and pickNextValidatorsToDeposit functions.
Add test that triggers the early return path in getNextValidatorsToDepositFromActiveOperators when fundableOperatorCount is zero, ensuring empty arrays are returned correctly.
Add tests that allocate validators to non-first operators in the fundable array, ensuring the _updateCountOfPickedValidatorsForEachOperator loop iterates past non-matching operators before finding the target. This improves branch coverage for line 738's condition check.
…tiple fundable operators Improves branch coverage for _updateCountOfPickedValidatorsForEachOperator loop by testing the scenario where multiple operators exist in the fundable array but the requested operator doesn't exist, forcing the loop to iterate through all operators with false condition before reverting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contracts/test/components/ConsensusLayerDepositManager.1.t.sol`:
- Around line 141-147: The test testDepositAllocationFailsWithNotEnoughFunds
uses _createAllocation(0) which will trigger AllocationWithZeroValidatorCount()
before NotEnoughFunds(); update the test to use a non-zero validator allocation
(e.g., _createAllocation(1)) so it exercises the insufficient-funds path and
keeps vm.expectRevert(abi.encodeWithSignature("NotEnoughFunds()")) valid against
depositToConsensusLayerWithDepositRoot on depositManager, or alternatively
change the expected revert to AllocationWithZeroValidatorCount() if the
zero-count case is intended.
🧹 Nitpick comments (3)
contracts/test/OperatorsRegistry.1.t.sol (3)
112-134: Type inconsistency between helper functions.
_createAllocationacceptsuint256 countwhile_createMultiAllocationacceptsuint32[] counts. This inconsistency can cause confusion when writing tests. Consider aligning the types:Suggested improvement
- function _createAllocation(uint256 opIndex, uint256 count) + function _createAllocation(uint256 opIndex, uint32 count) internal pure returns (IOperatorsRegistryV1.OperatorAllocation[] memory) { IOperatorsRegistryV1.OperatorAllocation[] memory allocations = new IOperatorsRegistryV1.OperatorAllocation[](1); allocations[0] = IOperatorsRegistryV1.OperatorAllocation({operatorIndex: opIndex, validatorCount: count}); return allocations; }
122-134: Consider adding array length validation.If
opIndexesandcountshave different lengths, the loop will either revert with an unclear out-of-bounds error or silently ignore extra elements. Adding an explicit check improves test debugging.Suggested improvement
function _createMultiAllocation(uint256[] memory opIndexes, uint32[] memory counts) internal pure returns (IOperatorsRegistryV1.OperatorAllocation[] memory) { + require(opIndexes.length == counts.length, "Array length mismatch"); IOperatorsRegistryV1.OperatorAllocation[] memory allocations = new IOperatorsRegistryV1.OperatorAllocation[](opIndexes.length);
1705-1725: Duplicate helper functions with inconsistent naming.These helpers duplicate the functionality from
OperatorsRegistryV1Tests(lines 112-134) but with different semantics:
- Here,
_createAllocationtakes arrays (like_createMultiAllocationin the other contract)_createMultiAllocationis just a pass-through wrapperConsider extracting these helpers into a shared base contract to avoid duplication and naming confusion:
Suggested refactor
abstract contract AllocationTestHelpers { function _createSingleAllocation(uint256 opIndex, uint32 count) internal pure returns (IOperatorsRegistryV1.OperatorAllocation[] memory) { IOperatorsRegistryV1.OperatorAllocation[] memory allocations = new IOperatorsRegistryV1.OperatorAllocation[](1); allocations[0] = IOperatorsRegistryV1.OperatorAllocation({operatorIndex: opIndex, validatorCount: count}); return allocations; } function _createMultiAllocation(uint256[] memory opIndexes, uint32[] memory counts) internal pure returns (IOperatorsRegistryV1.OperatorAllocation[] memory) { require(opIndexes.length == counts.length, "Array length mismatch"); IOperatorsRegistryV1.OperatorAllocation[] memory allocations = new IOperatorsRegistryV1.OperatorAllocation[](opIndexes.length); for (uint256 i = 0; i < opIndexes.length; ++i) { allocations[i] = IOperatorsRegistryV1.OperatorAllocation({operatorIndex: opIndexes[i], validatorCount: counts[i]}); } return allocations; } }Then have both test contracts inherit from
AllocationTestHelpers.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
certora/specs/OperatorRegistryV1ValidatorStates.spec (1)
450-462:⚠️ Potential issue | 🟠 MajorFix inconsistent validator state accessor and argument type.
getValidatorStateByIndexlikely expects(opIndex, valIndex), but this rule passesvalidatorData(bytes) and still usesgetValidatorStateforstateBefore, making the before/after comparison inconsistent or ill‑typed. If the intent is index-based, addvalIndexwith bounds and use the same accessor both times.🛠️ Proposed fix (index-based, consistent before/after)
rule validatorStateTransition_4_3_M13(method f, env e, calldataarg args) filtered { f -> isMethodID(f, 13) } { require isValidState(); - bytes validatorData; - uint opIndex; + uint opIndex; uint valIndex; require operatorStateIsValid(opIndex); //key <= limit <= funded <= exited require getKeysCount(opIndex) <= 4; //should not be higher than loop_iter - uint stateBefore = getValidatorState(opIndex, validatorData); + require isValIndexInBounds(opIndex, valIndex); + uint stateBefore = getValidatorStateByIndex(opIndex, valIndex); f(e, args); - uint stateAfter = getValidatorStateByIndex(opIndex, validatorData); + uint stateAfter = getValidatorStateByIndex(opIndex, valIndex); assert (stateAfter == 4) => (stateBefore == 3 || stateBefore == 4); }
juliaaschmidt
changed the title
juliaaschmidt
changed the title
…y.1.sol Add 10 new tests covering BYOV allocation logic: - OperatorDoesNotHaveEnoughFundableKeys with partial funding - InactiveOperator when operator exists but deactivated - FundedValidatorKeys event emission - Multi-operator allocation key ordering - Sequential allocations to same operator - Allocation when operator fully funded - View vs state-modifying behavior - Multi-operator with partial funding - Multi-operator second operator exceeds limit - Pick returns empty when no fundable operators
juliaaschmidt
changed the title
…340) * fix(certora): update pickNextValidatorsToDeposit signature for BYOV Update Certora specs to reflect the new function signature that takes OperatorAllocation[] instead of uint256 count. Also removes the unused getMaxValidatorAttributionPerRound harness function. * fix(certora): update depositToConsensusLayerWithDepositRoot signature Update function signature from (uint256, bytes32) to (OperatorAllocation[], bytes32) to match the BYOV deposit changes. * fix(certora): add via-ir compilation and update config for BYOV Add solc_via_ir and optimizer settings to resolve stack-too-deep errors. Update configs to use OperatorsRegistryV1Harness for proper type resolution with the new OperatorAllocation struct parameter types. * bug-fix: add solc_via_ir to CI config files where needed * fix(certora): update pickNextValidatorsToDeposit call sites to use OperatorAllocation[] Update rule call sites in startingValidatorsDecreasesDiscrepancy and witness4_3StartingValidatorsDecreasesDiscrepancy to pass IOperatorsRegistryV1.OperatorAllocation[] instead of uint count, matching the new function signature. * fix(certora): add missing loop increment in getValidatorState harness The loop in getValidatorState was missing the valIndex increment, causing it to only ever check index 0 and potentially infinite loop. * bug-fix: see if removing via ir fixes rule bug --------- Co-authored-by: juliaaschmidt <[email protected]> Co-authored-by: juuliaaschmidt <[email protected]>
mischat
reviewed
Feb 9, 2026
Member
mischat
left a comment
mischat
left a comment
There was a problem hiding this comment.
Some comments from my initial review ... we will over the operator's registry again tomorrow.
Signed-off-by: juliaaschmidt <[email protected]>
Changes to gas cost
🧾 Summary (99% most significant diffs)
Full diff report 👇
|
iamsahu
reviewed
Feb 11, 2026
Contributor
iamsahu
left a comment
iamsahu
left a comment
There was a problem hiding this comment.
Great work!
Apart from extracting the createAllocation logic into a contract that could be utilized across multiple test files the rest looks good.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This pull request introduces a significant refactor to the validator allocation and selection logic across the
OperatorsRegistryV1,RiverV1, andConsensusLayerDepositManagerV1contracts. The main change is moving from a simple count-based approach for validator selection to an explicit operator allocation model, where allocations specify how many validators each operator should provide. This enhances flexibility and control over validator distribution and includes new validation checks and error handling.The most important changes are:
Operator Allocation Refactor
Introduced the
OperatorAllocationstruct to represent explicit allocations of validators per operator, and updated all relevant interfaces and methods to use this model instead of a single count parameter. (IOperatorsRegistryV1,OperatorsRegistryV1,RiverV1,ConsensusLayerDepositManagerV1) [1] [2] [3] [4] [5] [6] [7] [8] [9]Updated the internal selection logic in
OperatorsRegistryV1to process and validate operator allocations, ensuring allocations are ordered, non-zero, and do not exceed available keys. Removed the previous round-robin and max-attribution logic, replacing it with direct allocation checks. [1] [2] [3] [4]Validation and Error Handling
Added new error types for invalid allocations, including unordered operator lists, allocations with zero validator count, and requests exceeding available keys. These errors are now thrown during allocation processing. [1] [2] [3]
In
ConsensusLayerDepositManagerV1, added validation to ensure the sum of all allocations does not exceed the committed balance and that the number of provided public keys matches the requested allocations.Interface and Import Updates
IOperatorsRegistryV1,IConsensusLayerDepositManagerV1) and imports to support the new allocation-based logic. [1] [2] [3] [4]These changes collectively provide a more robust and flexible system for validator selection and attribution, improving both control and safety in validator management.## Description
Notice
Pull Request Type
Breaking changes (if applicable)
Testing
Manual tests (if applicable)
Additional comments
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes