feat: download blocks as ssz #5923
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twoeths
force-pushed
the
tuyen/download_blocks_as_ssz
branch
2 times, most recently
from
381c6dc
to
fbeb88a
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
matthewkeil
previously approved these changes
Aug 31, 2023
| @@ -30,11 +30,11 @@ export const testData: GenericServerTestCases<Api> = { | |||
| // block | |||
|
|
|||
| getBlock: { | |||
| args: ["head"], | |||
| args: ["head", "json"], | |||
There was a problem hiding this comment.
Should we also check for ssz? Not sure if its needed. What do you think?
There was a problem hiding this comment.
these test data are keyed by api name so we can only test with either "json" or "ssz". This is "json" because by default we use it, and majority of consumers use "json" so I'd go with it to maintain the test case going forward
There was a problem hiding this comment.
It doesn't look like we unit test the ssz format for state either. I will be happy to add a unit test case after this gets merged. Will be good practice for me. I haven't worked with the API code much
lodestar/packages/api/test/unit/beacon/testData/debug.ts
Lines 47 to 54 in 97dfcfc
There was a problem hiding this comment.
This test is just testing the shape/types of the input and output data, not any behavior. Since we added a param to the getBlock functions, we need to add a param here.
This reverts commit fbeb88a.
twoeths
force-pushed
the
tuyen/download_blocks_as_ssz
branch
from
f91f152
to
bf70d88
Compare
matthewkeil
approved these changes
Sep 11, 2023
There was a problem hiding this comment.
Mostly just the question about sharing the types with the debug/state routes. Am approving because those are pretty small. Depending on how you feel you can update and Ill reapprove if you push code. Overall great work and a great example to follow in the future!!! 🚀 You are a 🥷
| ...client, | ||
| async getBlock<T extends ResponseFormat = "json">(blockId: BlockId, format?: T) { | ||
| if (format === "ssz") { | ||
| const res = await httpClient.arrayBuffer({ |
There was a problem hiding this comment.
This is sweet. I didnt know our httpClient did so much under the hood. was looking at how its implemented.
| @@ -31,6 +31,7 @@ import { | |||
| // See /packages/api/src/routes/index.ts for reasoning and instructions to add new routes | |||
|
|
|||
| export type BlockId = RootHex | Slot | "head" | "genesis" | "finalized"; | |||
| export const mimeTypeSSZ = "application/octet-stream"; | |||
There was a problem hiding this comment.
This already exists for state downloads. Do you think we should move this to a CONST and pull both from the same place?
Comment on lines
+55
to
+74
| export type BlockResponse<T extends ResponseFormat = "json"> = T extends "ssz" | ||
| ? ApiClientResponse<{[HttpStatusCode.OK]: Uint8Array}, HttpStatusCode.BAD_REQUEST | HttpStatusCode.NOT_FOUND> | ||
| : ApiClientResponse< | ||
| {[HttpStatusCode.OK]: {data: allForks.SignedBeaconBlock}}, | ||
| HttpStatusCode.BAD_REQUEST | HttpStatusCode.NOT_FOUND | ||
| >; | ||
|
|
||
| export type BlockV2Response<T extends ResponseFormat = "json"> = T extends "ssz" | ||
| ? ApiClientResponse<{[HttpStatusCode.OK]: Uint8Array}, HttpStatusCode.BAD_REQUEST | HttpStatusCode.NOT_FOUND> | ||
| : ApiClientResponse< | ||
| { | ||
| [HttpStatusCode.OK]: { | ||
| data: allForks.SignedBeaconBlock; | ||
| executionOptimistic: ExecutionOptimistic; | ||
| version: ForkName; | ||
| }; | ||
| }, | ||
| HttpStatusCode.BAD_REQUEST | HttpStatusCode.NOT_FOUND | ||
| >; | ||
|
|
There was a problem hiding this comment.
Very nice TS work!!! 🚀 Is very helpful to see how you created these response types
| @@ -246,11 +256,12 @@ export const routesData: RoutesData<Api> = { | |||
|
|
|||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
|
|
|||
| type GetBlockReq = {params: {block_id: string}; headers: {accept?: string}}; | |||
There was a problem hiding this comment.
Out of curiosity why do we use the snake case for url params. Seems like a good pattern and I see that its the norm throughout the codebase. Just want to expand my understanding.
There was a problem hiding this comment.
its part of the beacon api spec
https://github.com/ethereum/beacon-apis
They say: use this specific url path with these specific url query params, should return a response with these codes and these payloads
| @@ -3,6 +3,7 @@ import {Resolves} from "./utils/types.js"; | |||
|
|
|||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
|
|
|||
| export type ResponseFormat = "json" | "ssz"; | |||
There was a problem hiding this comment.
This type is also used by the debug state routes. Should we put this somewhere that both can pull from the same type?
| @@ -30,11 +30,11 @@ export const testData: GenericServerTestCases<Api> = { | |||
| // block | |||
|
|
|||
| getBlock: { | |||
| args: ["head"], | |||
| args: ["head", "json"], | |||
There was a problem hiding this comment.
It doesn't look like we unit test the ssz format for state either. I will be happy to add a unit test case after this gets merged. Will be good practice for me. I haven't worked with the API code much
lodestar/packages/api/test/unit/beacon/testData/debug.ts
Lines 47 to 54 in 97dfcfc
|
🎉 This PR is included in v1.12.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This is the spec, we need to support "Accept" header for
getBlockapis https://ethereum.github.io/beacon-APIs/#/Description
getStatedoes