Skip to content
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: added OWR pectra v0.1 #145

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: added OWR pectra v0.1 #145

wants to merge 9 commits into from

Conversation

cosminobol
Copy link
Contributor

No description provided.

src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@OisinKyne OisinKyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
// ;; +--------+--------+
// ;; 48 48

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't tackled yet. I will check this out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an option for this. We don't have a clear example for how the fee will be computed in production but I followed the eas files of main and fake_expo to understand what they are doing. Did a similar behavior on our side in the last commit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure about it yet. Also they don't mention about it, other than using the fake_expo.eas contract. Which I assume it's similar to a production ready fee computation contract. In case it follows the same interface, our part should work as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @OisinKyne

Maybe @samparsky can you take a look at the last commit as well, please?

src/test/owr/pectra/PectraWithdrawalMock.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectraFactory.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectraFactory.sol Outdated Show resolved Hide resolved
src/owr/OptimisticWithdrawalRecipientPectra.sol Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants