-
Notifications
You must be signed in to change notification settings - Fork 790
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
Enhance typing of CL requests / beacon/execution payload handling / Dedicated Util Withdrawal + Deposit Request Classes #3398
Conversation
Codecov ReportAttention: Patch coverage is
|
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.
I got two small comments, but I don't think I'm deep enough in this code (yet) to give an in-depth review, so I think better wait until @acolytec3 takes a look 😄
const hasDepositRequests = depositRequests !== undefined && depositRequests !== null | ||
const hasWithdrawalRequests = withdrawalRequests !== undefined && withdrawalRequests !== null | ||
const requests = | ||
hasDepositRequests || hasWithdrawalRequests ? ([] as CLRequest<CLRequestType>[]) : undefined |
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.
Shouldn't this just check if there is a requestsRoot
?
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.
well it can check that as well but I thought this check to be more thorough. though i have no strong opinions and can do requestroot check
} | ||
|
||
if (this.requests !== undefined) { | ||
for (const request of this.requests) { |
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.
These requests should be ordered. I'm not sure if a switch / case
structure is the "nicest" way to handle this 🤔
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 sorting is verified as part of block validations so we don't need to worry and keep things simple here
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.
LGTM
This is really a great one! 👍❤️🙂🙏 |
No description provided.