-
Notifications
You must be signed in to change notification settings - Fork 195
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
Feat/withdrawal credentials #904
base: develop
Are you sure you want to change the base?
Changes from 7 commits
9b454a8
3bfe5ac
4420a7c
1a394bf
5183e89
2fc90ec
5888fac
c251b90
9cf5ea4
1b2dd97
d26dddc
66ccbcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,6 +9,8 @@ import "@openzeppelin/contracts-v4.4/token/ERC721/IERC721.sol"; | |||||
import "@openzeppelin/contracts-v4.4/token/ERC20/utils/SafeERC20.sol"; | ||||||
|
||||||
import {Versioned} from "./utils/Versioned.sol"; | ||||||
import {TriggerableWithdrawals} from "./lib/TriggerableWithdrawals.sol"; | ||||||
import { ILidoLocator } from "../common/interfaces/ILidoLocator.sol"; | ||||||
|
||||||
interface ILido { | ||||||
/** | ||||||
|
@@ -27,6 +29,7 @@ contract WithdrawalVault is Versioned { | |||||
|
||||||
ILido public immutable LIDO; | ||||||
address public immutable TREASURY; | ||||||
ILidoLocator public immutable LOCATOR; | ||||||
|
||||||
// Events | ||||||
/** | ||||||
|
@@ -42,26 +45,24 @@ contract WithdrawalVault is Versioned { | |||||
event ERC721Recovered(address indexed requestedBy, address indexed token, uint256 tokenId); | ||||||
|
||||||
// Errors | ||||||
error LidoZeroAddress(); | ||||||
error TreasuryZeroAddress(); | ||||||
error ZeroAddress(); | ||||||
error NotLido(); | ||||||
error NotValidatorExitBus(); | ||||||
error NotEnoughEther(uint256 requested, uint256 balance); | ||||||
error ZeroAmount(); | ||||||
|
||||||
/** | ||||||
* @param _lido the Lido token (stETH) address | ||||||
* @param _treasury the Lido treasury address (see ERC20/ERC721-recovery interfaces) | ||||||
*/ | ||||||
constructor(ILido _lido, address _treasury) { | ||||||
if (address(_lido) == address(0)) { | ||||||
revert LidoZeroAddress(); | ||||||
} | ||||||
if (_treasury == address(0)) { | ||||||
revert TreasuryZeroAddress(); | ||||||
} | ||||||
constructor(address _lido, address _treasury, address _locator) { | ||||||
_requireNonZero(_lido); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
_requireNonZero(_treasury); | ||||||
_requireNonZero(_locator); | ||||||
|
||||||
LIDO = _lido; | ||||||
LIDO = ILido(_lido); | ||||||
TREASURY = _treasury; | ||||||
LOCATOR = ILidoLocator(_locator); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -70,6 +71,12 @@ contract WithdrawalVault is Versioned { | |||||
*/ | ||||||
function initialize() external { | ||||||
_initializeContractVersionTo(1); | ||||||
_updateContractVersion(2); | ||||||
} | ||||||
|
||||||
function finalizeUpgrade_v2() external { | ||||||
_checkContractVersion(1); | ||||||
_updateContractVersion(2); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -122,4 +129,27 @@ contract WithdrawalVault is Versioned { | |||||
|
||||||
_token.transferFrom(address(this), TREASURY, _tokenId); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @dev Adds full withdrawal requests for the provided public keys. | ||||||
* The validator will fully withdraw and exit its duties as a validator. | ||||||
* @param pubkeys An array of public keys for the validators requesting full withdrawals. | ||||||
*/ | ||||||
function addFullWithdrawalRequests( | ||||||
bytes[] calldata pubkeys | ||||||
) external payable { | ||||||
if(msg.sender != LOCATOR.validatorsExitBusOracle()) { | ||||||
revert NotValidatorExitBus(); | ||||||
} | ||||||
|
||||||
TriggerableWithdrawals.addFullWithdrawalRequests(pubkeys, msg.value); | ||||||
} | ||||||
|
||||||
function getWithdrawalRequestFee() external view returns (uint256) { | ||||||
return TriggerableWithdrawals.getWithdrawalRequestFee(); | ||||||
} | ||||||
|
||||||
function _requireNonZero(address _address) internal pure { | ||||||
if (_address == address(0)) revert ZeroAddress(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,148 @@ | ||||||||
// SPDX-FileCopyrightText: 2023 Lido <info@lido.fi> | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
// SPDX-License-Identifier: GPL-3.0 | ||||||||
|
||||||||
pragma solidity 0.8.9; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Also, consider moving to common libs? |
||||||||
|
||||||||
library TriggerableWithdrawals { | ||||||||
address constant WITHDRAWAL_REQUEST = 0x0c15F14308530b7CDB8460094BbB9cC28b9AaaAA; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add some comment with "validation" link?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May this contract address be changed on testnets to something else (as, for example, with the deposit contract on Holesky)? We'll have to use a separate library for it? |
||||||||
|
||||||||
error MismatchedArrayLengths(uint256 keysCount, uint256 amountsCount); | ||||||||
error InsufficientBalance(uint256 balance, uint256 totalWithdrawalFee); | ||||||||
error FeeNotEnough(uint256 minFeePerRequest, uint256 requestCount, uint256 providedTotalFee); | ||||||||
|
||||||||
error WithdrawalRequestFeeReadFailed(); | ||||||||
error InvalidPubkeyLength(bytes pubkey); | ||||||||
error WithdrawalRequestAdditionFailed(bytes pubkey, uint256 amount); | ||||||||
error NoWithdrawalRequests(); | ||||||||
error PartialWithdrawalRequired(bytes pubkey); | ||||||||
|
||||||||
event WithdrawalRequestAdded(bytes pubkey, uint256 amount); | ||||||||
|
||||||||
/** | ||||||||
* @dev Adds full withdrawal requests for the provided public keys. | ||||||||
* The validator will fully withdraw and exit its duties as a validator. | ||||||||
* @param pubkeys An array of public keys for the validators requesting full withdrawals. | ||||||||
*/ | ||||||||
function addFullWithdrawalRequests( | ||||||||
bytes[] calldata pubkeys, | ||||||||
uint256 totalWithdrawalFee | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 // 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See no reason to pass it inside the function when it can definitely be checked before and after the function call in the WithdrawalVault itself. It's kinda alien constraint for the raw withdrawal request creation library. E.g. in the vaults we don't care where the funds for the gas will come from and we don't need to check it at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The EIP-7002 specification does not impose an upper limit on the withdrawal request fee; this was intentionally designed for flexibility. The library implementation proposed in this PR follows the EIP-7002 specification and does not add any extra restrictions on withdrawal requests. If we do not want the general purpose library, we can remove control over the request fee, and narrow the library's functionality to allow only requests with minimal fees. This would simplify the code slightly, but also limit the library's potential use cases. I am not confident that we will not need control over the request fee in the future, @folkyatina what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After an internal discussion, we agreed to follow the eip 7002 specification and keep control over the request fee amount, but pass the request fee instead of the total withdrawal fee to simplify library implementation. |
||||||||
) internal { | ||||||||
uint64[] memory amounts = new uint64[](pubkeys.length); | ||||||||
_addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee); | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* @dev Adds partial withdrawal requests for the provided public keys with corresponding amounts. | ||||||||
* A partial withdrawal is any withdrawal where the amount is greater than zero. | ||||||||
* A full withdrawal is any withdrawal where the amount is zero. | ||||||||
* This allows withdrawal of any balance exceeding 32 ETH (e.g., if a validator has 35 ETH, up to 3 ETH can be withdrawn). | ||||||||
* However, the protocol enforces a minimum balance of 32 ETH per validator, even if a higher amount is requested. | ||||||||
* @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( | ||||||||
bytes[] calldata pubkeys, | ||||||||
uint64[] calldata amounts, | ||||||||
uint256 totalWithdrawalFee | ||||||||
) internal { | ||||||||
_requireArrayLengthsMatch(pubkeys, amounts); | ||||||||
|
||||||||
for (uint256 i = 0; i < amounts.length; i++) { | ||||||||
if (amounts[i] == 0) { | ||||||||
revert PartialWithdrawalRequired(pubkeys[i]); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
_addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee); | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* @dev Adds partial or full withdrawal requests for the provided public keys with corresponding amounts. | ||||||||
* A partial withdrawal is any withdrawal where the amount is greater than zero. | ||||||||
* This allows withdrawal of any balance exceeding 32 ETH (e.g., if a validator has 35 ETH, up to 3 ETH can be withdrawn). | ||||||||
* However, the protocol enforces a minimum balance of 32 ETH per validator, even if a higher amount is requested. | ||||||||
* @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 addWithdrawalRequests( | ||||||||
bytes[] calldata pubkeys, | ||||||||
uint64[] calldata amounts, | ||||||||
uint256 totalWithdrawalFee | ||||||||
) internal { | ||||||||
_requireArrayLengthsMatch(pubkeys, amounts); | ||||||||
_addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee); | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* @dev Retrieves the current withdrawal request fee. | ||||||||
* @return The minimum fee required per withdrawal request. | ||||||||
*/ | ||||||||
function getWithdrawalRequestFee() internal view returns (uint256) { | ||||||||
(bool success, bytes memory feeData) = WITHDRAWAL_REQUEST.staticcall(""); | ||||||||
|
||||||||
if (!success) { | ||||||||
revert WithdrawalRequestFeeReadFailed(); | ||||||||
} | ||||||||
|
||||||||
return abi.decode(feeData, (uint256)); | ||||||||
} | ||||||||
|
||||||||
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(); | ||||||||
folkyatina marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
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]; | ||||||||
folkyatina marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
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); | ||||||||
} | ||||||||
|
||||||||
|
||||||||
function _requireArrayLengthsMatch( | ||||||||
bytes[] calldata pubkeys, | ||||||||
uint64[] calldata amounts | ||||||||
) internal pure { | ||||||||
if (pubkeys.length != amounts.length) { | ||||||||
revert MismatchedArrayLengths(pubkeys.length, amounts.length); | ||||||||
} | ||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
pragma solidity 0.8.9; | ||
|
||
import {TriggerableWithdrawals} from "contracts/0.8.9/lib/TriggerableWithdrawals.sol"; | ||
|
||
contract TriggerableWithdrawals_Harness { | ||
function addFullWithdrawalRequests( | ||
bytes[] calldata pubkeys, | ||
uint256 totalWithdrawalFee | ||
) external { | ||
TriggerableWithdrawals.addFullWithdrawalRequests(pubkeys, totalWithdrawalFee); | ||
} | ||
|
||
function addPartialWithdrawalRequests( | ||
bytes[] calldata pubkeys, | ||
uint64[] calldata amounts, | ||
uint256 totalWithdrawalFee | ||
) external { | ||
TriggerableWithdrawals.addPartialWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee); | ||
} | ||
|
||
function addWithdrawalRequests( | ||
bytes[] calldata pubkeys, | ||
uint64[] calldata amounts, | ||
uint256 totalWithdrawalFee | ||
) external { | ||
TriggerableWithdrawals.addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee); | ||
} | ||
|
||
function getWithdrawalRequestFee() external view returns (uint256) { | ||
return TriggerableWithdrawals.getWithdrawalRequestFee(); | ||
} | ||
|
||
function getWithdrawalsContractAddress() public pure returns (address) { | ||
return TriggerableWithdrawals.WITHDRAWAL_REQUEST; | ||
} | ||
|
||
function deposit() external payable {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
// SPDX-License-Identifier: UNLICENSED | ||
pragma solidity 0.8.9; | ||
|
||
/** | ||
* @notice This is an mock of EIP-7002's pre-deploy contract. | ||
*/ | ||
contract WithdrawalsPredeployed_Mock { | ||
uint256 public fee; | ||
bool public failOnAddRequest; | ||
bool public failOnGetFee; | ||
|
||
event eip7002WithdrawalRequestAdded(bytes request, uint256 fee); | ||
|
||
function setFailOnAddRequest(bool _failOnAddRequest) external { | ||
failOnAddRequest = _failOnAddRequest; | ||
} | ||
|
||
function setFailOnGetFee(bool _failOnGetFee) external { | ||
failOnGetFee = _failOnGetFee; | ||
} | ||
|
||
function setFee(uint256 _fee) external { | ||
require(_fee > 0, "fee must be greater than 0"); | ||
fee = _fee; | ||
} | ||
|
||
fallback(bytes calldata input) external payable returns (bytes memory output){ | ||
if (input.length == 0) { | ||
require(!failOnGetFee, "fail on get fee"); | ||
|
||
output = abi.encode(fee); | ||
return output; | ||
} | ||
|
||
require(!failOnAddRequest, "fail on add request"); | ||
|
||
require(input.length == 56, "Invalid callData length"); | ||
|
||
emit eip7002WithdrawalRequestAdded( | ||
input, | ||
msg.value | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { ContractTransactionReceipt } from "ethers"; | ||
import { ethers } from "hardhat"; | ||
|
||
import { findEventsWithInterfaces } from "lib"; | ||
|
||
const withdrawalRequestEventABI = ["event WithdrawalRequestAdded(bytes pubkey, uint256 amount)"]; | ||
const withdrawalRequestEventInterface = new ethers.Interface(withdrawalRequestEventABI); | ||
type WithdrawalRequestEvents = "WithdrawalRequestAdded"; | ||
|
||
export function findEvents(receipt: ContractTransactionReceipt, event: WithdrawalRequestEvents) { | ||
return findEventsWithInterfaces(receipt!, event, [withdrawalRequestEventInterface]); | ||
} | ||
|
||
const eip7002TriggerableWithdrawalMockEventABI = ["event eip7002WithdrawalRequestAdded(bytes request, uint256 fee)"]; | ||
const eip7002TriggerableWithdrawalMockInterface = new ethers.Interface(eip7002TriggerableWithdrawalMockEventABI); | ||
type Eip7002WithdrawalEvents = "eip7002WithdrawalRequestAdded"; | ||
|
||
export function findEip7002TriggerableWithdrawalMockEvents( | ||
receipt: ContractTransactionReceipt, | ||
event: Eip7002WithdrawalEvents, | ||
) { | ||
return findEventsWithInterfaces(receipt!, event, [eip7002TriggerableWithdrawalMockInterface]); | ||
} |
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.