Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Feat/withdrawal credentials #904
Changes from 4 commits
9b454a8
3bfe5ac
4420a7c
1a394bf
5183e89
2fc90ec
5888fac
c251b90
9cf5ea4
1b2dd97
d26dddc
66ccbcf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
Also, consider moving to common libs?
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.
Add some comment with "validation" link?
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.
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?
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.
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 comment
The 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
WithdrawalVault.sol
balance remains unaffected, preventing issues with the Oracle’s accounting. Consequently, the entiremsg.value
sent is used as the fee for withdrawal requests:Other vaults could employ the strategy you mentioned, assuming all required Ether is already in the contract’s balance:
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 comment
The 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 comment
The 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 comment
The 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.