GetBlobSidecars API update #8278
Conversation
| .name("GetBlobSidecarsResponse") | ||
| .withField("data", listOf(blobSidecarType), Function.identity()) | ||
| .withOptionalField("version", MILESTONE_TYPE, (b) -> Optional.of(b.getMilestone())) |
There was a problem hiding this comment.
I don't think we need the OptionalField here for version, execution_optimistic and finalized. Can make it similar to GetBlock. The SerializableTypeDefinition is only used for serializing our fields which we know that are always present in our implementation.
There was a problem hiding this comment.
The OpenAPI spec has defined them as optional. So I believe we need to use withOptionalField() to ensure we are compliant, don't we?
There was a problem hiding this comment.
Yes, if we want to deserialize as well. Otherwise, not needed, since we always add them, but not fuzzed about it. Probably defining them as optional makes it more clear.
There was a problem hiding this comment.
I was thinking from the perspective of the open api spec that we generate. If we want to match the "official" spec, we would need the optional field so we would generate the open api schema spec as optional as well.
There was a problem hiding this comment.
Yeah, that makes sense.
| blobSidecars.isEmpty() | ||
| ? Optional.empty() | ||
| : addMetaData( | ||
| //TODO we don't care about metadata here...need to find a way to get around the lack of a block root? |
There was a problem hiding this comment.
Yeah, we use slotSelectorForAll only in our internal /teku/v1/beacon/blob_sidecars/{slot} which doesn't have any of the metadata fields, so I would just add a comment why we don't care about the block root and just default it to ZERO as you do.
|
I think also worth adding to |
|
The changes look good for me. The only thing missing is some tests around the metadata logic in |
|
Changelog and tests added |
| blobSidecarSelectorFactory.headSelector().getBlobSidecars(indices).get(); | ||
| assertThat(result).hasValue(blobSidecars); | ||
| assertThat(Optional.of(result.get().getData())).hasValue(blobSidecars); |
There was a problem hiding this comment.
NIT: I don't understand why we need to wrap it into an Optional for this assertion. Can't we do it like:
assertThat(result.get().getData()).isEqualTo(blobSidecars);
There was a problem hiding this comment.
This applies to other assertions in this class as well but I won't add a comment on each one :)
|
|
||
| assertThat(result).isNotEmpty(); | ||
| final BlobSidecarsAndMetaData blobSidecarsAndMetaData = result.get(); | ||
| assertThat(blobSidecarsAndMetaData.isFinalized()).isEqualTo(true); |
There was a problem hiding this comment.
NIT: you can use .isTrue() instead of .isEqualTo(true).
|
|
||
| assertThat(result).isNotEmpty(); | ||
| final BlobSidecarsAndMetaData blobSidecarsAndMetaData = result.get(); | ||
| assertThat(blobSidecarsAndMetaData.isFinalized()).isEqualTo(false); |
There was a problem hiding this comment.
NIT: can use .isFalse() :)
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
lucassaldanha
force-pushed
the
getBlobSideCar-api-update
branch
from
0bf4b76 to
ffdfc9d
Compare
PR Description
This PR adds the metadata fields to the result of the endpoint
/eth/v1/beacon/blob_sidecars/{block_id}(GetBlobSidecars) updated here: ethereum/beacon-APIs#441Fixed Issue(s)
Fixes #8191
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog