[Mev Boost\Builder] validator registration beacon rest api (server side) #5545
Conversation
| } | ||
|
|
||
| @SuppressWarnings("JavaCase") | ||
| public static class SignedValidatorRegistrationOpenApiSchema { |
There was a problem hiding this comment.
you can put this in schema with the rest of the objects if you like, most of the time the annotated objects are outside of the class...
|
|
||
| @JsonProperty("gas_limit") | ||
| @Schema(type = "string", format = "uint64", example = EXAMPLE_UINT64) | ||
| UInt64 gas_limit; |
There was a problem hiding this comment.
most of the time schema objects have these as public final, the reason for that is then you don't also need to do the @JsonProperty annotation (or you need a getter, one of the 3 options)
There was a problem hiding this comment.
I defined them public only, so I don't have to define a constructor and I don't have to use @JsonProperty
There was a problem hiding this comment.
seems to be the most compact version :)
There was a problem hiding this comment.
Looks good generally - I just think we're starting to create schema definitions that aren't actually tied to the spec version so probably need to break the rule of putting them into SchemaDefinitions. Previously all SSZ was used with the spec and so was inherently tied to the milestone but now we're just using it to define a REST API.
| if (spec.isMilestoneSupported(SpecMilestone.BELLATRIX)) { | ||
| addMigratedEndpoint(new PostRegisterValidator(dataProvider, spec, schemaCache)); | ||
| } |
There was a problem hiding this comment.
I suspect we should always add the endpoint but then just return bad request if bellatrix isn't enabled. It's weird to get a 404 based on the network config.
| @@ -288,6 +290,20 @@ public ExecutionPayloadHeader deserializeJsonExecutionPayloadHeader( | |||
| .jsonDeserialize(objectMapper.createParser(jsonFile)); | |||
| } | |||
|
|
|||
| public SszList<SignedValidatorRegistration> deserializeSignedValidatorRegistrations( | |||
There was a problem hiding this comment.
The other deserialise methods here are detecting based on slot automatically so I'm not sure I'd add this here since it's not actually tied to a milestone - it's more tied to the API version.
I actually wonder if this is a case where the schema shouldn't be part of SchemaDefinitions. If we put it in SchemaDefinitions we can only change the API at milestone boundaries but I'm pretty sure that's not how the API is going to evolve. And if it wasn't in SchemaDefinitions we could make this API work regardless of whether Bellatrix is enabled (though it probably wouldn't have any actual effect until Bellatrix was active).
There was a problem hiding this comment.
yep, that was my feeling. Going to move them in API itself
tbenr
force-pushed
the
beacon_node_register_validator
branch
from
7f021d6 to
ab0734c
Compare
tbenr
force-pushed
the
beacon_node_register_validator
branch
from
e37c3fd to
5735ff6
Compare
| try { | ||
| request.respondAsync( | ||
| validatorDataProvider | ||
| .registerValidators(request.getRequestBody()) | ||
| .thenApply(AsyncApiResponse::respondOk)); | ||
| } catch (final JsonProcessingException jsonProcessingException) { | ||
| request.respondError(SC_BAD_REQUEST, jsonProcessingException.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
@rolfyone is it correct my bad request handling in case of wrong input?
tbenr
force-pushed
the
beacon_node_register_validator
branch
from
d54820e to
96e56a5
Compare
This draft PR introduces:
SszListofSignedValidatorRegistrationValidatorRegistrationand related Ssz schemas totech.pegasys.teku.spec.schemas.ApiSchemasin next PR we migrate all Ssz API objects (non-milestone specific) to this new class
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog