Support content negotiation of light client data served via rest api #4641
Conversation
…ate via p2p resp/req
…nt_optimistic_update available via the gossip domain
| @@ -24,5 +25,53 @@ export function getRoutes(config: IChainForkConfig, api: Api): ServerRoutes<Api, | |||
| return Buffer.from(serializeProof(proof)); | |||
| }, | |||
| }, | |||
| getBootstrap: { | |||
| ...serverRoutes.getBootstrap, | |||
There was a problem hiding this comment.
tried de-duplicating some of the code here, but was a bit tricky getting the types to line up right. If this is a blocker, then I'll revisit.
There was a problem hiding this comment.
While this PR fullfils it's intent it's re-using an API (that I introduced in /debug/beacon/states) that doesn't scale really well.
If this feature is not urgent, since it's technically just a bandwidth optimization for the lightclient. We can sit on this and look for a design of dual JSON / SSZ that scales to multiple routes without so much boilerplate code.
There's an open discussion to support dual JSON / SSZ on the engine API + beacon API so we may want to wait for that
@dapplion do you know when this discussion will be concluded? Because I do not think it makes sense to keep this PR indefinitely at limbo. My suggestion is to review and merge so at least we have complaint with this part of the API spec. The optimisation of the API you introduced can follow later. |
|
This PR has two different scopes:
Point 1. I do not like the approach since it doesn't scale and we want to eventually support many more routes with the same functionality. Point 1. is not urgent and can wait to figure out on other design Point 2. is a necessary bug fix to align with the spec, which is (a) urgent, and (b) clearly defined so it can be merged immediately. In general having PRs with multiple scopes is not great but in this case it's an obvious example to split so we can get the bug fix asap. |
|
Both of these changes are bug fixes, in the sense that both are needed due to the fact that the spec got updated/properly defined, and our implementation needed to be modified to be compliant with the updated spec. But If you think one is more "buggier" than the other and deserves urgent attention, while the other does not? Then I guess that's fine. Only that i would go about it differently. I would have the "less than perfect changes" reviewed and merged now, instead of waiting to figure out an improved design, while we keep differing from the spec. But I guess here we disagree. Anyways, here is a separate PR #4877 that adds versioning to the LC API. Please help review 🙏 |
|
Closing as this approach does not scale for reasons outlined above |
Motivation
The REST API for light client related data should also be able to return results as SSZ.
Closes #4635
This PR also includes changes to align the JSON object returned by the API to include version.
Closes #4812