Skip to content

WIP: Deneb protos separated from deneb-integration#12770

Closed
kasey wants to merge 1 commit intodevelopfrom
deneb-integration-proto
Closed

WIP: Deneb protos separated from deneb-integration#12770
kasey wants to merge 1 commit intodevelopfrom
deneb-integration-proto

Conversation

@kasey
Copy link
Collaborator

@kasey kasey commented Aug 22, 2023

What type of PR is this?
Other

What does this PR do? Why is it needed?
This PR separates deneb-integration protobuf changes to reduce the line count in the diff to develop.

Other notes for review

DO NOT MERGE

The plan for this branch is to provide a reference point for a more reviewable deneb-integration diff - a 3rd branch deneb-integration-remaining is on the way, which will be set up in a PR against this branch (deneb-integration-proto). We can use that PR to get a cleaner diff (no proto changes). If deneb-integration-remaining diffs cleanly against deneb-integration, we can confidently fast-forward merge deneb-integration knowing that it has had this extra round of final review.

This will break attribution/history and likely cause a messy rebase. This is a second attempt after trying to also integrate consensus-types and gateway changes, which spiraled out into other areas of the code.

Files from the proto package that were left behind to avoid breaking changes when the rest of the branch is missing:

proto/eth/service/beacon_chain_service.pb.go
proto/eth/service/beacon_chain_service.pb.gw.go
proto/eth/service/beacon_chain_service.proto

@kasey kasey requested review from a team and prestonvanloon as code owners August 22, 2023 02:05
@kasey kasey requested review from potuz and rauljordan August 22, 2023 02:05
@kasey kasey changed the title add protos, not consenus types or gw code Deneb protos separated from deneb-integration Aug 22, 2023
@kasey kasey changed the title Deneb protos separated from deneb-integration WIP: Deneb protos separated from deneb-integration Aug 22, 2023
// MarshalJSON --
func (p *PayloadAttributesV2) MarshalJSON() ([]byte, error) {
withdrawals := p.Withdrawals
if withdrawals == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this added in ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

presumably this fixes an issue where an empty withdrawals list results in a nil slice. You're probably wondering why this isn't needed in capella/develop though, cc @terencechain who is on the blame.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kasey let's revert this. It's irreverent. Same for V3 as well. Nice catch guys

"SignedBlindedBeaconBlockCapella",
"SignedBeaconBlockDeneb",
"SignedBlindedBeaconBlockDeneb",
"BlsToExecutionChange",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we add this in deneb ?

@kasey kasey closed this Aug 28, 2023
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