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

Start to add support for Electra #131

Merged
merged 9 commits into from
May 6, 2024
Merged

Start to add support for Electra #131

merged 9 commits into from
May 6, 2024

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Apr 29, 2024

This PR makes the necessary changes (adds new structures and updates versioned structures) for Electra. Sorry, it contains a lot of new files and it lacks tests. I would like to pull in these changes into an electra branch, so that multiple people (myself included) can add tests later, in smaller PRs. After this PR gets merged, I'll make the equivalent PR for go-builder-client. We need to wait because go-eth2-client is a dependency and it currently needs a replace directive.

spec/phase0/attestation.go Outdated Show resolved Hide resolved
api/v1/deneb/generate.go Outdated Show resolved Hide resolved
spec/phase0/types.go Outdated Show resolved Hide resolved
@jtraglia
Copy link
Contributor Author

I added two nolint:gocyclo comments for these:

spec/versionedsignedbeaconblock.go:272:1: cyclomatic complexity 31 of func `(*VersionedSignedBeaconBlock).Attestations` is high (> 30) (gocyclo)
func (v *VersionedSignedBeaconBlock) Attestations() ([]VersionedAttestation, error) {
^
spec/electra/beaconblockbody_json.go:71:1: cyclomatic complexity 31 of func `(*BeaconBlockBody).UnmarshalJSON` is high (> 30) (gocyclo)
func (b *BeaconBlockBody) UnmarshalJSON(input []byte) error {
^

Not really sure how I would make these less complex.

@mcdee
Copy link
Contributor

mcdee commented May 5, 2024

The Electra changes themselves look good.

There are a fair number of non-Electra changes in here. Would it be possible to remove these from this PR, to avoid additional pain when we come to merge this branch back into master? Anything non-cosmetic can be pushed to a separate PR to be included directly into master. Thanks.

@jtraglia
Copy link
Contributor Author

jtraglia commented May 5, 2024

Yes, good suggestion. I'll make those changes first thing tomorrow morning.

@jtraglia
Copy link
Contributor Author

jtraglia commented May 6, 2024

Yes, good suggestion. I'll make those changes first thing tomorrow morning.

I've undone the non-electra changes except for the new Epoch marshaling functions in spec/phase0/types.go.

I made these PRs to master:

@mcdee mcdee merged commit 2ff35ee into attestantio:electra May 6, 2024
3 checks passed
@jtraglia jtraglia deleted the electra branch May 6, 2024 18:59
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.

2 participants