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

Add custom json marshalling for versioned structs #493

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

avalonche
Copy link
Collaborator

@avalonche avalonche commented Aug 2, 2023

📝 Summary

The request bodies for signed beacon blocks (used to publish to the beacon block) and signed blinded beacon blocks (request body for getPayload) does not have a "version" field and thus need custom json marshalling.

The version field in the struct helps with consolidating type conversions coming in later PRs.

⛱ Motivation and Context

📚 References


✅ I have run these commands

  • make lint
  • make test-race
  • go mod tidy
  • I have seen and agree to CONTRIBUTING.md

@avalonche avalonche requested review from jtraglia, michaelneuder and metachris and removed request for michaelneuder August 2, 2023 02:57
@avalonche avalonche mentioned this pull request Aug 2, 2023
17 tasks
Copy link
Collaborator

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

It's sad that we have to make wrapped structs to implement marshaling functions. I suppose these new structs could be easily deleted if go-eth2-client adds support for this.

LGTM, with a few nits 👍

Comment on lines +269 to +270
}
capella := new(capella.SubmitBlockRequest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: for consistency with the other funcs, please add a blank line between these two blocks.

Or delete the blank lines between these blocks of code in the other functions.

require.NoError(t, err)
expectedJSONBytes := buffer.Bytes()

require.Equal(t, expectedJSONBytes, bytes.ToLower(marshalledJSONBytes))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that bytes.ToLower is necessary because the proposer fee recipient is mixed-case (checksummed).

return nil
}

capella := new(apiv1capella.SignedBlindedBeaconBlock)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: these variables (deneb and capella) collide with package names.

I think we should fix this by renaming the packages (in a later PR).

@avalonche
Copy link
Collaborator Author

go-eth2-client can support it but it'll be inconsistent with the marshalling of other versioned structs, this is more reflective of the way the API specs are inconsistent in using the version field

@avalonche avalonche merged commit deb8687 into develop Aug 2, 2023
2 checks passed
@avalonche avalonche deleted the custom-marshalling branch August 2, 2023 14:26
austonst pushed a commit to aestus-relay/mev-boost-relay that referenced this pull request Jan 25, 2024
avalonche added a commit that referenced this pull request Jan 30, 2024
* Remove bellatrix from wrapper types (#475)

* Remove bellatrix from wrapper types

* remove test logs

* Remove get header wrapper types (#477)

* Remove signed blinded beacon block wrapper (#482)

* remove signed blinded beacon block wrapper type

* linting

* remove signed beacon block wrapper types (#483)

* Remove submit block request wrapper (#485)

* remove submit block request wrapper types

* fix tests

* fix lint

* Upgrade go-boost-utils (#488)

* Upgrade go-boost-utils

* pr comments

* remove commented out code

* Add custom json marshalling for versioned structs (#493)

* Add deneb signature checking for block contents

* Add deneb support for type conversions

* Add redis and database tests to store deneb payloads

* Block submission to v3 validation endpoint

* Update signed block conversions

* Replace some expectCont with expectOk (#509)

* Allow fork epochs to be 0

* Make attestantio import names consistent (#510)

* Make attestantio import names consistent

* Fix linter errors & two comments

* Fix mistake in redis prefix name (#517)

* Fix mistake in redis prefix for deneb

* Fix typo in prefix

* rebase conflicts from main

* update submit block request

* bug fixes

* fix blob sidecar signature

* ssz encode request to publish block

* use v2 publish endpoint by default

* go mod tidy

* update relay to latest builder-specs

* update go mod

* fix lint and tests

* switch to json encoding instead of ssz for block publishing v2

* add blob logging

* address pr comments

* Handle no deneb fork schedule from beacon client (#572)

* backwards compatibility if no deneb schedule

* Update services/api/service.go

Co-authored-by: Chris Hager <chris@linuxuser.at>

---------

Co-authored-by: Chris Hager <chris@linuxuser.at>

* change specific error log to info because it's expected nowadays (#574)

* Add json and ssz marshalling tests (#573)

* add test vectors

* linting

---------

Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Co-authored-by: Chris Hager <chris@linuxuser.at>
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