-
Notifications
You must be signed in to change notification settings - Fork 63
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 SSZ support to builder api #104
Conversation
In support of this change. It's nicely backwards-compatible, doesn't add significant risk, decreases latency and could simplify CL flows. |
Full support, it's a great improvement over the existing API and should be particularly useful for larger blocks. Maybe worth adding an example payload for SSZ as well? |
I gave this a try but it has it's limitations since you can't display binary data on the explorer. The solution seems to be to use external examples so we could add ssz to existing examples and reference the files in the ssz examples. However, there seems to be an issue swagger-api/swagger-ui#5433, the examples won't be displayed on the explorer, it would be great if it would show an option to download the example file but it only shows no example available. There might be a fix for this at some point like swagger-api/swagger-js#2013. For now could do something like here nflaig/builder-specs@ssz-support...nflaig:builder-specs:ssz-examples with the caveat that it won't be available on the explorer. |
Would it make sense to mark the json api deprecated with this same PR? That way we can push to get json removed in the next fork. |
In my opinion, having JSON as an option is a good thing, it can be quite useful for debugging. If this reduces complexity significantly for some implementation it could be considered, however in Lodestar for example it does not make a difference at all as we have to keep JSON support for beacon-api anyways. |
Im in support of this. In Lighthouse its not a problem for us to support both JSON and SSZ |
Lodestar supports this in the latest release (v1.23.0) in case anyone needs a client to test with. |
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.
generally looks good!
had one question about adding a 415 code to the headers endpoint
This is a proposal to add SSZ support to builder API in a backward compatible way without requiring to bump the apis to v2.
It outlines what is expected from the client and how a well-behaved server should handle different content types, headers, and use appropriate error codes to allow the client to handle the error.
Notable changes are
submitBlindedBlock
allows to SSZ encoding for both request and response bodygetHeader
allows SSZ encoding of response bodyEth-Consensus-Version
to responses, only required if response body is SSZ encoded but preferably should be added to all responses eventuallySince we can't know if a builder implemented the changes as per spec, the client can determine if SSZ is supported by requesting an SSZ encoded response in the
getHeader
request. Preferably this should be done setting aAccept
header with multiple entries and quality values likeAccept: application/octet-stream;q=1.0,application/json;q=0.9
to give the builder the option to send a JSON response.Related to #53 and #54