Skip to content

Commit

Permalink
Addressed PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
pinebit committed Jan 24, 2025
1 parent 0b164bd commit 4f03f52
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 30 deletions.
35 changes: 19 additions & 16 deletions src/owr/OptimisticWithdrawalRecipientV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
// Failed to call system contract get_fee()
error InvalidRequest_SystemGetFee();

// Insufficient fee value to conclude the request
// Insufficient fee provided in the call's value to conclude the request
error InvalidRequest_NotEnoughFee();

// Failed to call system contract add_consolidation_request()
Expand Down Expand Up @@ -176,11 +176,11 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
_distributeFunds(PULL);
}

/// Request validators consolidation with system contract
/// Request validators consolidation with the EIP7251 system contract
/// @dev all source validators will be consolidated into the target validator
/// the caller must compute fee before calling and send sufficient msg.value amount
/// excessed amount will be refunded
/// @param sourcePubKeys source validators public keys to be consolidated
/// the caller must compute the fee before calling and send a sufficient msg.value amount
/// excess amount will be refunded
/// @param sourcePubKeys validator public keys to be consolidated
/// @param targetPubKey target validator public key
function requestConsolidation(
bytes[] calldata sourcePubKeys,
Expand All @@ -195,22 +195,24 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
uint256 _currentFee = _computeSystemContractFee(consolidationSystemContract);
if (_currentFee > remainingFee) revert InvalidRequest_NotEnoughFee();

_requestConsolidation(sourcePubKeys[i], targetPubKey, _currentFee);
remainingFee -= _currentFee;
_requestConsolidation(sourcePubKeys[i], targetPubKey, _currentFee);

unchecked {
++i;
}
}

// Future optimization idea: do not send if gas cost exceeds the value.
if (remainingFee > 0) payable(msg.sender).transfer(remainingFee);
}

/// Request partial/full withdrawal with system contract
/// @dev the caller must compute fee before calling and send sufficient msg.value amount
/// excessed amount will be refunded
/// Request partial/full withdrawal from the EIP7002 system contract
/// @dev the caller must compute the fee before calling and send a sufficient msg.value amount
/// excess amount will be refunded
/// withdrawals that leave a validator with (0..32) ether will cause the transaction to fail
/// @param pubKeys validator public keys
/// @param amounts withdrawal amounts
/// @param amounts withdrawal amounts in gwei
function requestWithdrawal(
bytes[] calldata pubKeys,
uint64[] calldata amounts
Expand All @@ -224,14 +226,15 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
uint256 _currentFee = _computeSystemContractFee(withdrawalSystemContract);
if (_currentFee > remainingFee) revert InvalidRequest_NotEnoughFee();

_requestWithdrawal(pubKeys[i], amounts[i], _currentFee);
remainingFee -= _currentFee;
_requestWithdrawal(pubKeys[i], amounts[i], _currentFee);

unchecked {
++i;
}
}

// Future optimization idea: do not send if gas cost exceeds the value.
if (remainingFee > 0) payable(msg.sender).transfer(remainingFee);
}

Expand Down Expand Up @@ -269,15 +272,15 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {
/// Withdraw token balance for account
/// @param account Address to withdraw on behalf of
function withdraw(address account) external {
uint256 tokenAmount = pullBalances[account];
uint256 amount = pullBalances[account];
unchecked {
// shouldn't underflow; fundsPendingWithdrawal = sum(pullBalances)
fundsPendingWithdrawal -= uint128(tokenAmount);
fundsPendingWithdrawal -= uint128(amount);
}
pullBalances[account] = 0;
account.safeTransferETH(tokenAmount);
account.safeTransferETH(amount);

emit Withdrawal(account, tokenAmount);
emit Withdrawal(account, amount);
}

/// -----------------------------------------------------------------------
Expand All @@ -302,7 +305,7 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles {

/// Returns the balance for account `account`
/// @param account Account to return balance for
/// @return Account's balance OWR token
/// @return Account's withdrawable ether balance
function getPullBalance(address account) external view returns (uint256) {
return pullBalances[account];
}
Expand Down
8 changes: 3 additions & 5 deletions src/owr/OptimisticWithdrawalRecipientV2Factory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,12 @@ contract OptimisticWithdrawalRecipientV2Factory {
/// @param recoveryAddress Address to recover tokens to
/// If this address is 0x0, recovery of unrelated tokens can be completed by
/// either the principal or reward recipients. If this address is set, only
/// this address can recover
/// tokens (or ether) that isn't the token of the OWRecipient contract
/// this address can recover ERC20 tokens allocated to the OWRV2 contract
/// @param principalRecipient Address to distribute principal payments to
/// @param rewardRecipient Address to distribute reward payments to
/// @param amountOfPrincipalStake Absolute amount of stake to be paid to
/// principal recipient (multiple of 32 ETH)
/// (reward recipient has no threshold & receives all residual flows)
/// it cannot be greater than uint96
/// principal recipient (reward recipient has no threshold &
/// receives all residual flows) it cannot be greater than uint96
/// @param owner Owner of the new OptimisticWithdrawalRecipientV2 clone
/// @return owr Address of new OptimisticWithdrawalRecipientV2 clone
function createOWRecipient(
Expand Down
10 changes: 1 addition & 9 deletions src/test/owr/OptimisticWithdrawalRecipientV2.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {IENSReverseRegistrar} from "../../interfaces/IENSReverseRegistrar.sol";
contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
using SafeTransferLib for address;

event ReceiveETH(uint256 amount);
event DistributeFunds(uint256 principalPayout, uint256 rewardPayout, uint256 pullFlowFlag);
event RecoverNonOWRecipientFunds(address indexed nonOWRToken, address indexed recipient, uint256 amount);
event ConsolidationRequested(address indexed requester, bytes indexed source, bytes indexed target);
Expand Down Expand Up @@ -244,7 +243,7 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
bytes[] memory pubkeys = new bytes[](1);
uint64[] memory amounts = new uint64[](1);
bytes memory pubkey = new bytes(48);
uint64 amount = 1234;
uint64 amount = 1234; // gwei
for (uint256 i = 0; i < 48; i++) {
pubkey[i] = bytes1(0xAB);
}
Expand Down Expand Up @@ -316,13 +315,6 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test {
assertEq(address(owrETH).balance, 1 ether);
}

function testEmitOnReceiveETH() public {
vm.expectEmit(true, true, true, true);
emit ReceiveETH(1 ether);

address(owrETH).safeTransferETH(1 ether);
}

function testReceiveERC20() public {
address(mERC20).safeTransfer(address(owrETH), 1 ether);
assertEq(mERC20.balanceOf(address(owrETH)), 1 ether);
Expand Down

0 comments on commit 4f03f52

Please sign in to comment.