feat: added OWR pectra v0.1 #145

cosminobol · 2024-10-23T06:41:32Z

No description provided.

src/owr/OptimisticWithdrawalRecipientPectra.sol

src/owr/OptimisticWithdrawalRecipientPectraFactory.sol

src/owr/OptimisticWithdrawalRecipientPectra.sol

samparsky · 2024-11-08T10:34:21Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+
+    (bool ret,) = pectraConsolidationAddress.call{value: msg.value}(abi.encodePacked(source, target));
+    if (!ret) revert InvalidConsolidation_Failed();
+    emit ConsolidationRequested(msg.sender);


let's add source and destination pubkeys too

What do you mean ? Isn't the source and destination the pubKeys already?

src/owr/OptimisticWithdrawalRecipientPectra.sol

samparsky · 2024-11-08T10:36:57Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+  /// Requests principal withdrawal
+  function requestPrincipalWithdrawal(bytes memory pubkey, uint8 amount) external payable onlyOwnerOrRoles(PRINCIPAL_WITHDRAWAL_ROLE) {
+    _requestWithdrawal(pubkey, amount, false);
+    emit PrincipalWithdrawalRequested(msg.sender);


lets include amount here

samparsky · 2024-11-08T10:42:42Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+  }
+
+  /// Requests principal withdrawal
+  function requestPrincipalWithdrawal(bytes memory pubkey, uint8 amount) external payable onlyOwnerOrRoles(PRINCIPAL_WITHDRAWAL_ROLE) {


should this be an array given that multiple validators will use a OWR?

I can add a batchRequest method as well. For both, principal and rewards

OisinKyne

Want to have a think about the contract name, and the name of the two system contracts. Concern about a uint8 knocking around. Think we potentially may need to get smart around the value handling aspect, to ensure we have enough msg.value to pay the basefee, but also don't overpay or get the eth stuck.

Let me know if the feedback is unclear :)

OisinKyne · 2024-11-27T14:36:51Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+  address public immutable pectraWithdrawalAddress;
+  address public immutable pectraConsolidationAddress;


Suggested change

address public immutable pectraWithdrawalAddress;

address public immutable pectraConsolidationAddress;

address public immutable executionLayerWithdrawalSystemContract;

address public immutable consolidationSystemContract;

People won't remember these by the hardfork they come in with. TBH we might come up with a better name for the contract too.

OisinKyne · 2024-11-27T14:43:43Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+  // solhint-disable-next-line no-empty-blocks
+  /// clone implementation doesn't use constructor


Suggested change

// solhint-disable-next-line no-empty-blocks

/// clone implementation doesn't use constructor

/// Sets the system contract addresses for execution layer withdrawals and consolidations.

OisinKyne · 2024-11-27T14:45:43Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+
+  /// emit event when receiving ETH
+  /// @dev implemented w/i clone bytecode
+  /* receive() external payable { */


Suggested change

/* receive() external payable { */

/* receive() external payable { */

Do we not want this address to be payable? I see its commented in OWR too. Certainly no need to emit an event. But wonder would people try and transfer eth to this contract, maybe not having a payable receive or fallback prevents that, idk.

OisinKyne · 2024-11-27T14:57:31Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+    // ;;  +--------+--------+
+    // ;;      48       48
+
+    (bool ret,) = pectraConsolidationAddress.call{value: msg.value}(abi.encodePacked(source, target));


Suggested change

(bool ret,) = pectraConsolidationAddress.call{value: msg.value}(abi.encodePacked(source, target));

(bool ret,) = pectraConsolidationAddress.call{value: msg.value}(abi.encodePacked(source, target));

Its interesting that the fee will have to be passed as a value to the call, not as like gas costs. This might make it difficult to predict. Does the method return value if we overpay? it doesn't look like it to me. https://github.com/lightclient/sys-asm/blob/main/src/consolidations/main.eas#L51

So maybe we should do a read request of the fee (i think that's possible looking at the assembly), then send only that as msg.value? and then return the remainder to the caller?

OisinKyne · 2024-11-28T17:05:31Z

src/test/owr/pectra/PectraWithdrawalMock.sol

+
+import "forge-std/console.sol";
+
+contract PectraWithdrawalMock {


Similar to other places, maybe this should be called an ExecutionLayerWithdrawalSystemContractMock

OisinKyne · 2024-11-28T17:21:06Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+    //  +--------+--------+
+    //  | pubkey | amount |
+    //  +--------+--------+
+    //      48       8


Maybe a stupid question but, are these measured in bits or bytes?

Because uint8, is 8 bits, representing a max number of 255. but is bytes memory pubkey also measured in bits? or is it measured in bytes? A validator pubkey is 96 hex chars, or 48 byte chars, or 384 bits.

Should amount be a uint64? which is 8*8bytes. That seems to make more sense for an amount to withdraw than a number from 0-> 255

OisinKyne · 2024-11-28T17:23:16Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+    if (
+      (!_rewards && amount < BALANCE_CLASSIFICATION_THRESHOLD)
+        || (_rewards && amount >= BALANCE_CLASSIFICATION_THRESHOLD)
+    ) {
+      if (_rewards) revert InvalidPectraWithdrawal_Rewards();
+      else revert InvalidPectraWithdrawal_Principal();
+    }


What are we trying to achieve here? I feel like this could be simplified, I have a hard time parsing it with an if(and or and)(if/else).

OisinKyne · 2024-11-28T17:24:36Z

src/owr/OptimisticWithdrawalRecipientPectraFactory.sol

+/// @author Obol
+/// @notice A factory contract for cheaply deploying
+/// OptimisticWithdrawalRecipientPectra.
+/// @dev This contract uses token = address(0) to refer to ETH.


Suggested change

/// @dev This contract uses token = address(0) to refer to ETH.

This is a stale comment from when the OWRs supported erc20s, and native eth was indicated by using address(0) for the token address, right?

OisinKyne · 2024-11-28T17:36:02Z

src/owr/OptimisticWithdrawalRecipientPectraFactory.sol

+    owr = OptimisticWithdrawalRecipientPectra(address(owrImpl).clone(data));
+    owr.initialize(owner);
+
+    emit CreateOWRecipient(address(owr), recoveryAddress, principalRecipient, rewardRecipient, amountOfPrincipalStake);


Should we log the owner? (how many things can we include in a log? i thought it was 4, 3 if named, but this has 5 and I'm suggesting a 6th

OisinKyne · 2024-11-28T17:39:05Z

src/owr/OptimisticWithdrawalRecipientPectra.sol

+/// based on threshold to it's recipients
+/// @dev Only ETH can be distributed for a given deployment. There is a
+/// recovery method for tokens sent by accident.
+contract OptimisticWithdrawalRecipientPectra is Clone, OwnableRoles {


Suggested change

contract OptimisticWithdrawalRecipientPectra is Clone, OwnableRoles {

contract ControllableWithdrawalRecipient is Clone, OwnableRoles {

Suggested change

contract OptimisticWithdrawalRecipientPectra is Clone, OwnableRoles {

contract EditableWithdrawalRecipient is Clone, OwnableRoles {

Suggested change

contract OptimisticWithdrawalRecipientPectra is Clone, OwnableRoles {

contract ControllableOptimisticWithdrawalRecipient is Clone, OwnableRoles {

Trying to decide on what name might make the most sense, not convinced people will remember these features by the hardfork they are introduced in. Want to keep the withdrawal recipient suffix. Probably should keep the optimistic piece, to indicate that it optimistically assumes rewards are rewards not worst case slashing. Hoping to incorporate the 'exitable' and 'editable' aspects potentially.

Have a think and maybe run by me what option we go with before you make the changes.

cosminobol added 3 commits October 23, 2024 09:38

feat: added OWR pectra v0.1

12ea1e3

patch: added roles to OWR pectra

2a8e906

feat: added consolidation request

abf9064

samparsky reviewed Oct 31, 2024

View reviewed changes

src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved

samparsky requested changes Oct 31, 2024

View reviewed changes

src/owr/OptimisticWithdrawalRecipientPectraFactory.sol Show resolved Hide resolved

src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved

patch: use direct inputs for Pectra calls & make addresses immutable

5b0c2ee

samparsky requested changes Nov 8, 2024

View reviewed changes

cosminobol added 2 commits November 14, 2024 22:56

patch: minor fixes & batch request

e4f1438

chore: removed view modifier

add3a48

OisinKyne reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added OWR pectra v0.1 #145

feat: added OWR pectra v0.1 #145

cosminobol commented Oct 23, 2024

samparsky Nov 8, 2024

cosminobol Nov 10, 2024

samparsky Nov 8, 2024

samparsky Nov 8, 2024

cosminobol Nov 10, 2024

OisinKyne left a comment

OisinKyne Nov 27, 2024

OisinKyne Nov 27, 2024

OisinKyne Nov 27, 2024

OisinKyne Nov 27, 2024

OisinKyne Nov 28, 2024

OisinKyne Nov 28, 2024

OisinKyne Nov 28, 2024

OisinKyne Nov 28, 2024

OisinKyne Nov 28, 2024

OisinKyne Nov 28, 2024

		address public immutable pectraWithdrawalAddress;
		address public immutable pectraConsolidationAddress;

		// solhint-disable-next-line no-empty-blocks
		/// clone implementation doesn't use constructor

	// solhint-disable-next-line no-empty-blocks
	/// clone implementation doesn't use constructor
	/// Sets the system contract addresses for execution layer withdrawals and consolidations.

	/* receive() external payable { */
	/* receive() external payable { */

	(bool ret,) = pectraConsolidationAddress.call{value: msg.value}(abi.encodePacked(source, target));
	(bool ret,) = pectraConsolidationAddress.call{value: msg.value}(abi.encodePacked(source, target));


		import "forge-std/console.sol";

		contract PectraWithdrawalMock {

	contract OptimisticWithdrawalRecipientPectra is Clone, OwnableRoles {
	contract ControllableWithdrawalRecipient is Clone, OwnableRoles {

	contract OptimisticWithdrawalRecipientPectra is Clone, OwnableRoles {
	contract EditableWithdrawalRecipient is Clone, OwnableRoles {

	contract OptimisticWithdrawalRecipientPectra is Clone, OwnableRoles {
	contract ControllableOptimisticWithdrawalRecipient is Clone, OwnableRoles {

feat: added OWR pectra v0.1 #145

Are you sure you want to change the base?

feat: added OWR pectra v0.1 #145

Conversation

cosminobol commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OisinKyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment