-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Builder ssz #14976
base: develop
Are you sure you want to change the base?
Builder ssz #14976
Conversation
opt := func(r *http.Request) { | ||
r.Header.Set(api.VersionHeader, version.String(sb.Version())) | ||
r.Header.Set("Content-Type", api.OctetStreamMediaType) | ||
r.Header.Set("Accept", api.OctetStreamMediaType) |
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.
should the Accept also have json to properly process error message?
// EnableBuilderSSZ enables Builder APIs to send and receive in SSZ format | ||
EnableBuilderSSZ = &cli.BoolFlag{ | ||
Name: "enable-builder-ssz", | ||
Aliases: []string{"builder-ssz"}, |
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.
Why defining an alias 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.
just as a shorter name, some other clients have it as this I think.
errMalformedHostname = errors.New("hostname must include port, separated by one colon, like example.com:3500") | ||
errMalformedRequest = errors.New("required request data are missing") | ||
errNotBlinded = errors.New("submitted block is not blinded") | ||
blockToPayloadMapping = map[int]int{ |
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.
Is it possible to use a function instead like:
if blockVersion >= version.Deneb {
return version.Deneb
}
return blockVersion
Rationale: It will lighten the burden for the boilerplate creation in future forks.
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.
let me try that
return nil, errors.Wrap(err, fmt.Sprintf("unsupported header version %s", versionHeader)) | ||
} | ||
|
||
switch { |
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.
Isn't it more idiomatic to have multiple if
s starting with if ver >= version.Electra
then if ver == version.Deneb
, if ver == version.Capella
... ?
forkVersion int, | ||
) (interfaces.ExecutionData, *v1.BlobsBundle, error) { | ||
switch { | ||
case forkVersion >= version.Deneb: |
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.
Same question here for if vs switch.
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'm fine with going with if, i'll change it
Co-authored-by: Manu NALEPA <[email protected]>
Co-authored-by: Manu NALEPA <[email protected]>
What type of PR is this?
Feature
What does this PR do? Why is it needed?
implements ssz mode for using a builder setup, the following endpoints will request and receive responses in ssz format when the
--enable-builder-ssz=true
flag is passed to the beacon node/eth/v1/builder/header/{{.Slot}}/{{.ParentHash}}/{{.Pubkey}}
/eth/v1/builder/blinded_blocks
/eth/v1/builder/validators
note: this pr does not update builder E2E to support ssz, this is another todo item for proper testing
Which issues(s) does this PR fix?
Fixes # ethereum/builder-specs#104, ethereum/builder-specs#110
Other notes for review
Acknowledgements