feat: OWR V2 #153
feat: OWR V2 #153
Conversation
| @@ -158,16 +158,17 @@ contract ObolLidoSplitTest is ObolLidoSplitTestHelper, Test { | |||
|
|
|||
| function testFuzz_CanDistributeWithFee( | |||
| address anotherSplit, | |||
| uint256 amountToDistribute, | |||
| uint8 amountToDistributeEth, | |||
There was a problem hiding this comment.
fuzzing was failing for this test reliably, hence reduced the ranges.
There was a problem hiding this comment.
is 8bit very low if this is measured in wei? We should maybe figure out if there could actuall be something amiss at like <=uint96. not worried about uint256.
There was a problem hiding this comment.
This is in ETH. We multiply it below uint256(amountToDistributeEth) * 1 ether;.
| /// principal recipient (multiple of 32 ETH) | ||
| /// (reward recipient has no threshold & receives all residual flows) | ||
| /// it cannot be greater than uint96 | ||
| /// @param owner Owner of the new OptimisticWithdrawalRecipientV2 clone |
There was a problem hiding this comment.
Should we say this address is the admin for the rbac system? (is that the case even or is there a roleAdmin role?)
There was a problem hiding this comment.
The term corresponds to onlyOwnerOrRoles attribute logic and OwnableRoles design which is not under our control. It is sort of Admin role but without explicitly defined "ADMIN_ROLE".
| @@ -158,16 +158,17 @@ contract ObolLidoSplitTest is ObolLidoSplitTestHelper, Test { | |||
|
|
|||
| function testFuzz_CanDistributeWithFee( | |||
| address anotherSplit, | |||
| uint256 amountToDistribute, | |||
| uint8 amountToDistributeEth, | |||
There was a problem hiding this comment.
is 8bit very low if this is measured in wei? We should maybe figure out if there could actuall be something amiss at like <=uint96. not worried about uint256.
| // +--------+--------+ | ||
| // | pubkey | amount | | ||
| // +--------+--------+ | ||
| // 48 8 |
There was a problem hiding this comment.
Okay if this is official and amounts are only 64 bit, maybe they're not wei? and disregard my other comment
There was a problem hiding this comment.
uint64 in according to their example: https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7002.md#fee-overpayment
There was a problem hiding this comment.
Looks like the amount is in GWEI
Sample withdrawal tx: https://explorer.pectra-devnet-5.ethpandaops.io/tx/0xcae1a4da5202658d57ae5daca8cfa1afb24200f3d70aa62747228706026929dc?tab=index
1 ETH is encoded as 0x000000003b9aca00, so it must be GWEI...
There was a problem hiding this comment.
okay, if i'm not mistaken, the EVM native eth is handled in wei, we might want to be careful as we manage the two variations (or at least flag explicitly on the @ dev docs that requestWithdrawals is in gwei as opposed to wei.
| contract OWRV2Reentrancy is Test { | ||
| receive() external payable { | ||
| if (address(this).balance <= 1 ether) OptimisticWithdrawalRecipientV2(msg.sender).distributeFunds(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this a test to see what happens during re-entrancy, or is there a non-re-entrant modifier i didn't spot somewhere? I like to be careful of re-entrancy, but hope we can handle it rather than slapping a modifier on things to prevent it :)
There was a problem hiding this comment.
Yeah it is a test helper, can ignore ^
There was a problem hiding this comment.
The initial test was borrowed from V1 with all necessary helpers adjusted to V2.
| } | ||
|
|
||
| function testCan_OWRIsPayable() public { | ||
| owrETH.distributeFunds{value: 2 ether}(); |
There was a problem hiding this comment.
Weird, is this insta-adding and distributing msg.value? I guess it works? 🤷
There was a problem hiding this comment.
This part came from V1 test, I did not examine it in details.
Summary
In according to the design doc we reviewed internally, introduced the new version of OWR contracts that respect Pectra changes.
Details
Please refer to the internal design doc for further details.
Note: most of the existing code has been copied from V1 (audited version) and remained intact as it has been audited and tested already.
How to test it
forge testticket: #152