Feat/withdrawal credentials #904
Conversation
![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: 2fc90ec Minimum allowed coverage is ♻️ This comment has been updated with latest results |
| * @param pubkeys An array of public keys for the validators requesting withdrawals. | ||
| * @param amounts An array of corresponding withdrawal amounts for each public key. | ||
| */ | ||
| function addPartialWithdrawalRequests( |
There was a problem hiding this comment.
What if I want a batch that is mix of partial and full withdrawals?
There was a problem hiding this comment.
The separation of Full and Partial withdrawals was suggested to simplify an interaction with the withdrawal contract.
An approach, proposed in the Triggerable Withdravals document suggests that the Validator Exit Bus will only use full withdrawal. Usages of partial withdrawals might require a different mechanism inside the Lido protocol, which is why this PR proposes to separate those types of requests.
If you consider that a mix of partial and full withdrawals might be useful in the future we can consider multiple approaches.
-
Add the
addWithdravalCredentialsmethod which will consume both partial and full withdrawals. -
In addition to the first approach, remove
addPartialWithdrawalRequestsmethod, so theaddWithdravalCredentialswill be used for both partials and mix (partial + full) withdrawal requests.
What do you think will be an optimal approach?
There was a problem hiding this comment.
1st one will be fine. I think that this kind of constraint is not required in vaults.
There was a problem hiding this comment.
Yes, I am also considering the first approach, noted, thanks!
| feeToSend += unallocatedFee; | ||
| } | ||
|
|
||
| bytes memory callData = abi.encodePacked(pubkey, amount); |
There was a problem hiding this comment.
memory expansion in a loop can eat a lot of gas very quickly. Maybe worth to optimize like in BeaconChainDepositor.
| @@ -27,6 +28,7 @@ contract WithdrawalVault is Versioned { | |||
|
|
|||
| ILido public immutable LIDO; | |||
| address public immutable TREASURY; | |||
| address public immutable VALIDATORS_EXIT_BUS; | |||
There was a problem hiding this comment.
It may be better to store locator here
| 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).
| emit WithdrawalRequestAdded(pubkey, amount); | ||
| } | ||
|
|
||
| assert(address(this).balance == prevBalance); |
There was a problem hiding this comment.
Is it possible to calculate the fee exactly at the moment of signing the tx? Shouldn't we ease the constraint and add some refund capability here?
There was a problem hiding this comment.
Great question.
Calls to the system contract require a fee payment defined by the current contract state. Overpaid fees are not returned to the caller. It is not generally possible to compute the exact required fee amount ahead of time.
When adding a withdrawal request from a contract, the contract can perform a read operation to check for the current fee and then pay an exact amount.
When adding a withdrawal request from EOA request may result in overpayment of fees. But the gas cost of returning the overage would likely be higher than the overage itself.
Proposed in this PR implementation does not imply any restriction on the caller, but makes sure that the withdrawal credential balance (accounting) will not be affected.
In the proposed Triggerable Withdrawal implementation, the dedicated bot will provide the fee within the withdrawal request transaction and, a small overpayment is possible. This is the simplest approach, which is currently considered for Triggerable Withdrawal implementation.
Other withdrawal contracts may consider the first approach without overpayment.
There was a problem hiding this comment.
For vaults, it will be good to exclude msg.value or fees' amount assumptions from this method. I mean, it's ok that we can't foresee the price, but in the same time, I'd like that the excessive amount just stay on the vault if any and will be available for withdraw later.
So, I'd rather just check if the balance is enough to add all requests and does not impose any assumptions on if the balance was topped up in this tx or before.
There was a problem hiding this comment.
Good point, I agree. The fee allocation strategy should be decoupled from the withdrawals library, allowing each contract to determine its own approach based on its design and requirements. To address these changes, I suggest adding a new totalWithdrawalFee parameter, which represents the total amount that will be spent on the EIP-7002 withdrawal requests.
The following pseudo-code demonstrates the approach:
library WithdrawalRequests {
function addWithdrawalRequests(
bytes[] calldata pubkeys,
uint64[] memory amounts,
uint256 totalWithdrawalFee
) internal {
// Implementation that uses totalWithdrawalFee to cover EIP-7002 withdrawal requests
...
}
function getWithdrawalRequestFee() internal pure returns (uint256) {
// Returns the minimum required fee per request
...
}
}
contract A {
function addWithdrawalRequests(
bytes[] calldata pubkeys,
uint64[] calldata amounts
) external payable {
// Use the entire sent amount (msg.value) as the total fee for withdrawal requests
uint256 totalWithdrawalFee = msg.value;
WithdrawalRequests.addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee);
}
}
contract B {
function addWithdrawalRequests(
bytes[] calldata pubkeys,
uint64[] calldata amounts
) external {
// Use the minimum required fee per request
uint256 minFeePerRequest = WithdrawalRequests.getWithdrawalRequestFee();
uint256 totalWithdrawalFee = minFeePerRequest * pubkeys.length;
WithdrawalRequests.addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee);
}
}
// Additional contracts can implement other fee allocation strategies as needed.There was a problem hiding this comment.
Done. Here is some additional discussion on this topic: #904 (comment)
| function _addWithdrawalRequests( | ||
| bytes[] calldata pubkeys, | ||
| uint64[] memory amounts, | ||
| uint256 totalWithdrawalFee | ||
| ) internal { | ||
| uint256 keysCount = pubkeys.length; | ||
| if (keysCount == 0) { | ||
| revert NoWithdrawalRequests(); | ||
| } | ||
|
|
||
| if(address(this).balance < totalWithdrawalFee) { | ||
| revert InsufficientBalance(address(this).balance, totalWithdrawalFee); | ||
| } | ||
|
|
||
| uint256 minFeePerRequest = getWithdrawalRequestFee(); | ||
| if (minFeePerRequest * keysCount > totalWithdrawalFee) { | ||
| revert FeeNotEnough(minFeePerRequest, keysCount, totalWithdrawalFee); | ||
| } | ||
|
|
||
| uint256 feePerRequest = totalWithdrawalFee / keysCount; | ||
| uint256 unallocatedFee = totalWithdrawalFee % keysCount; | ||
| uint256 prevBalance = address(this).balance - totalWithdrawalFee; | ||
|
|
||
| for (uint256 i = 0; i < keysCount; ++i) { | ||
| bytes memory pubkey = pubkeys[i]; | ||
| uint64 amount = amounts[i]; | ||
|
|
||
| if(pubkey.length != 48) { | ||
| revert InvalidPubkeyLength(pubkey); | ||
| } | ||
|
|
||
| uint256 feeToSend = feePerRequest; | ||
|
|
||
| if (i == keysCount - 1) { | ||
| feeToSend += unallocatedFee; | ||
| } | ||
|
|
||
| bytes memory callData = abi.encodePacked(pubkey, amount); | ||
| (bool success, ) = WITHDRAWAL_REQUEST.call{value: feeToSend}(callData); | ||
|
|
||
| if (!success) { | ||
| revert WithdrawalRequestAdditionFailed(pubkey, amount); | ||
| } | ||
|
|
||
| emit WithdrawalRequestAdded(pubkey, amount); | ||
| } | ||
|
|
||
| assert(address(this).balance == prevBalance); | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
| */ | ||
| function addFullWithdrawalRequests( | ||
| bytes[] calldata pubkeys, | ||
| uint256 totalWithdrawalFee |
There was a problem hiding this comment.
It think that this parameter is not required at all. We can safely assume that all required ether is on the balance of the contract and we'll revert if it's not true and if we need some additional constraints (like, msg.value == fee), we can add it in the contract that use that lib.
There was a problem hiding this comment.
This parameter was introduced to decouple the fee allocation strategy from the withdrawals library, discussion. It enables contracts to employ different allocation strategies.
The proposed Validator Exitt Bus Triggerable Withdrawal implementation assumes that the withdrawal fee is provided by the actor who triggers the withdrawals. In this approach, the WithdrawalVault.sol balance remains unaffected, preventing issues with the Oracle’s accounting. Consequently, the entire msg.value sent is used as the fee for withdrawal requests:
// Simplified pseudo-code
function addWithdrawalRequests(
bytes[] calldata pubkeys,
uint64[] calldata amounts
) external payable {
// Use the entire sent amount (msg.value) as the total fee for withdrawal requests
uint256 totalWithdrawalFee = msg.value;
WithdrawalRequests.addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee);
}Other vaults could employ the strategy you mentioned, assuming all required Ether is already in the contract’s balance:
// Simplified pseudo-code
function addWithdrawalRequests(
bytes[] calldata pubkeys,
uint64[] calldata amounts
) external {
// Use the minimum required fee per request
uint256 minFeePerRequest = WithdrawalRequests.getWithdrawalRequestFee();
uint256 totalWithdrawalFee = minFeePerRequest * pubkeys.length;
WithdrawalRequests.addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee);
}When the withdrawal fee is specified explicitly, any fee allocation strategy can be used. The library ensures that the provided fee sufficiently covers all requests and that the exact fee amount is spent.
| uint256 prevBalance = address(this).balance - totalWithdrawalFee; | ||
|
|
||
| for (uint256 i = 0; i < keysCount; ++i) { | ||
| bytes memory pubkey = pubkeys[i]; |
There was a problem hiding this comment.
Unnecessary memory extension?
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.