feat: withdrawal credentials #904
Open
Conversation
This file contains 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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: 6cdc711 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
folkyatina
reviewed
Dec 20, 2024
| revert NoWithdrawalRequests(); | ||
| } | ||
|
|
||
| uint256 minFeePerRequest = getWithdrawalRequestFee(); |
There was a problem hiding this comment.
Doesn't fee increase with each request?
There was a problem hiding this comment.
No, the fee will not increase with each request. Inside the transaction, all requests will have the same fee.
EIP 7002 uses block-by-block behavior.
If block N processes X requests, then at the end of block N the number of withdrawal requests that the chain has processed relative to the “targeted” number increases by X - TARGET_WITHDRAWAL_REQUESTS_PER_BLOCK, and so the fee in block N+1 increases by a factor of e**((X - TARGET_WITHDRAWAL_REQUESTS_PER_BLOCK) / WITHDRAWAL_REQUEST_FEE_UPDATE_FRACTION).
folkyatina
reviewed
Jan 3, 2025
folkyatina
requested changes
Jan 14, 2025
Add role ADD_FULL_WITHDRAWAL_REQUEST_ROLE for full withdrawal requests.
…hdrawal-credentials
Access pubkeys and amounts directly instead of copying them to memory.
pass pubkeys as array of bytes
contracts/0.8.9/WithdrawalVault.sol
Outdated
Comment on lines
146
to
174
| function addFullWithdrawalRequests( | ||
| bytes calldata pubkeys | ||
| ) external payable onlyRole(ADD_FULL_WITHDRAWAL_REQUEST_ROLE) { | ||
| uint256 prevBalance = address(this).balance - msg.value; | ||
|
|
||
| uint256 minFeePerRequest = TriggerableWithdrawals.getWithdrawalRequestFee(); | ||
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; | ||
|
|
||
| if(totalFee > msg.value) { | ||
| revert InsufficientTriggerableWithdrawalFee( | ||
| msg.value, | ||
| totalFee, | ||
| pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH | ||
| ); | ||
| } | ||
|
|
||
| TriggerableWithdrawals.addFullWithdrawalRequests(pubkeys, minFeePerRequest); | ||
|
|
||
| uint256 refund = msg.value - totalFee; | ||
| if (refund > 0) { | ||
| (bool success, ) = msg.sender.call{value: refund}(""); | ||
|
|
||
| if (!success) { | ||
| revert TriggerableWithdrawalRefundFailed(); | ||
| } | ||
| } | ||
|
|
||
| assert(address(this).balance == prevBalance); | ||
| } |
Check warning
Code scanning / Slither
Divide before multiply Medium
contracts/0.8.9/WithdrawalVault.sol
Outdated
Comment on lines
146
to
174
| function addFullWithdrawalRequests( | ||
| bytes calldata pubkeys | ||
| ) external payable onlyRole(ADD_FULL_WITHDRAWAL_REQUEST_ROLE) { | ||
| uint256 prevBalance = address(this).balance - msg.value; | ||
|
|
||
| uint256 minFeePerRequest = TriggerableWithdrawals.getWithdrawalRequestFee(); | ||
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; | ||
|
|
||
| if(totalFee > msg.value) { | ||
| revert InsufficientTriggerableWithdrawalFee( | ||
| msg.value, | ||
| totalFee, | ||
| pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH | ||
| ); | ||
| } | ||
|
|
||
| TriggerableWithdrawals.addFullWithdrawalRequests(pubkeys, minFeePerRequest); | ||
|
|
||
| uint256 refund = msg.value - totalFee; | ||
| if (refund > 0) { | ||
| (bool success, ) = msg.sender.call{value: refund}(""); | ||
|
|
||
| if (!success) { | ||
| revert TriggerableWithdrawalRefundFailed(); | ||
| } | ||
| } | ||
|
|
||
| assert(address(this).balance == prevBalance); | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
tamtamchik
reviewed
Jan 23, 2025
tamtamchik
added
next-upgrade
tamtamchik
reviewed
Jan 28, 2025
…hdrawal-credentials
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.
Summary
This PR introduces an early-stage prototype for a potential implementation of EIP-7685: General Purpose Execution Layer Requests. Specifically, it implements EIP-7002: Execution Layer Triggerable Withdrawals within the Lido WithdrawalVault contract, which serves as the withdrawal credentials address for Lido validators.
Approach
The implementation follows the first approach outlined in the ADR for Withdrawal Credentials Contract. This approach was selected due to the following advantages:
Implementation Details
In this iteration, the
WithdrawalVaultcontract supports only full withdrawal requests. Partial withdrawals are not included because they are not part of the proposed Triggerable Withdrawal V1 implementation within the Lido protocol.The following pseudo-code demonstrates the approach:
Key Considerations
The modular design of separate libraries for different withdrawal request types ensures that withdrawal credentials contracts can import and use only the minimal functionality required. For example:
Notes
This PR is an initial prototype and is subject to further iterations based on feedback and evolving requirements.