-
Notifications
You must be signed in to change notification settings - Fork 229
Consolidation request #1093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consolidation request #1093
Conversation
Hardhat Unit Tests Coverage Summary
Diff against master
Results for commit: 5b9d906 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
test/common/lib/triggerableWithdrawals/maxEffectiveBalanceIncreaser.test.ts
Outdated
Show resolved
Hide resolved
const totalFee = BigInt(totalSourcePubkeysCount) * feeForRequest; | ||
await consolidationRequestPredeployed.mock__setFee(feeForRequest); | ||
|
||
await testEIP7251Mock( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move expect statements into the test itself? It's difficult to dive deep just to verify assertions.
scripts/scratch/steps/0160-deploy-max-effective-balance-increaser.ts
Outdated
Show resolved
Hide resolved
scripts/scratch/steps/0160-deploy-max-effective-balance-increaser.ts
Outdated
Show resolved
Hide resolved
test/common/lib/triggerableWithdrawals/maxEffectiveBalanceIncreaser.test.ts
Outdated
Show resolved
Hide resolved
test/common/lib/triggerableWithdrawals/maxEffectiveBalanceIncreaser.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
…reqeust triggers, fix tests, add new, rename contract and event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🫡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
bytes calldata _targetPubkey, | ||
uint256 _feePerRequest | ||
) private { | ||
uint256 sourcePubkeysCount = _validateAndCountPubkeys(_sourcePubkeys); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems calling the_validateAndCountPubkeys
again is redundant, since we already called it in the previous loop, and in this case it is enough to simply get the length without checks
WHAT
Add contract that allows to request consolidations.
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7251.md
HOW
This PR introduces the addConsolidationRequests method for batching consolidation operations. The method is publicly callable, but two key constraints ensure secure and correct execution:
The method can be called by any address, but only requests with correct WC (as verified on the CL side) will have actual effect.
Dashboard.increaseRewardsAdjustment()
:Within addConsolidationRequests, the call to dashboard.increaseRewardsAdjustment() requires the caller (i.e., the contract) to have the NODE_OPERATOR_REWARDS_ADJUST_ROLE on the Dashboard contract.
Consolidation Process Overview
setRewardsAdjustment
to fix it.