-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Builder ssz #14976
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
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?
Co-authored-by: Manu NALEPA <[email protected]>
Co-authored-by: Manu NALEPA <[email protected]>
api/client/builder/client.go
Outdated
if ver >= version.Electra { | ||
return c.parseHeaderElectra(data) | ||
case ver >= version.Deneb: | ||
} else if ver >= 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.
Nit: else
not needed.
Could be:
if ver >= version.Electra {
return ...
}
if ver >= version.Deneb {
return ...
}
...
return nil, fmt.Errorf("unsupported header version %s", versionHeader)
|
||
var errResponseVersionMismatch = errors.New("builder API response uses a different version than requested in " + api.VersionHeader + " header") | ||
|
||
func getVersionsBlockToPayload(blockVersion int) (int, error) { |
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 comment about else.
beacon-chain/builder/option.go
Outdated
if endpoint != "" { | ||
var opts []builder.ClientOpt | ||
if sszEnabled { | ||
log.Info("Using Builder APIs with SSZ enabled.") |
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.
No period .
at the end of single sentence log.
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.
All logs should have a prefix. Here could be builder
.
* wip * refactoring functions for easier readability * allow ssz for register validator * changelog * adding in blinded block submission tests * adding in tests for header ssz * fixing linting and tests * adding in custom errors and fixing ssz validator registration * Update api/client/builder/client.go Co-authored-by: Manu NALEPA <[email protected]> * Update api/client/builder/client.go Co-authored-by: Manu NALEPA <[email protected]> * manu's feedback * linting * adding in info log to notify that this setting was turned on * fixing linting * manu's feedback * fixing import --------- 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