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

Minor improvements to EIP-7685 #7142

Merged

Conversation

Gabriel-Trintinalia
Copy link
Contributor

PR description

Address feedback of #7068

@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as draft May 27, 2024 03:59
@Gabriel-Trintinalia Gabriel-Trintinalia changed the title [MINOR] - 7685 feedback Minor improvements to EIP-7685 May 27, 2024
@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as ready for review May 27, 2024 23:04
/**
* Combines two optional lists of requests into a single optional list.
*
* @param maybeDeposits Optional list of deposit requests.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - for consistency should this be renamed to maybeDepositsRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

actually on further consideration I see that matches the class names Deposit and WithdrawalRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

We have Withdrawal and WithdrawalRequest but Deposit is itself the request... it is a bit confusing but probably outside the scope of this PR. non-blocking.

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 will rename Deposit to DepositRequest but I wanted to do that in a different PR so it is easier to review it.

@Gabriel-Trintinalia Gabriel-Trintinalia enabled auto-merge (squash) May 28, 2024 01:57
@Gabriel-Trintinalia Gabriel-Trintinalia merged commit 7a5a3e0 into hyperledger:main May 28, 2024
37 checks passed
jflo pushed a commit to jflo/besu that referenced this pull request May 28, 2024
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
jflo pushed a commit to jflo/besu that referenced this pull request Jun 10, 2024
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants