[Merge] add merge Beacon API support #4537
Conversation
|
it's looking pretty good! the merge types need a whole lot of schema annotations to illustrate their types. without it, they come out like: In most other areas you'll see that they're basically 'string' type, defined by schema manually on each field... also in beacon state, the enum updated but there's no merge |
tbenr
force-pushed
the
merge_beacon_apis
branch
from
c8a6093
to
cac5e7b
Compare
| @@ -30,39 +28,38 @@ | |||
| public final UInt64 slot; | |||
|
|
|||
| @Schema(type = "string", format = "uint64") | |||
| public final UInt64 proposer_index; | |||
| public final UInt64 proposerIndex; | |||
There was a problem hiding this comment.
so by doing this, we effectively create some duplication - now the interface isn't sure about whether its proposer_index or proposerIndex, so we have both.
Need to at least put @JsonProperty("proposer_index") to define it. this will also get rid of the typing issue appearing because of the proposer_index type not being given a schema definition.
same goes for all renamed fields here.
The standard dictates the names of these fields, so it's important they're consistent.
There was a problem hiding this comment.
the other option is leave them named the way the api requires them, given that it's only an object that the api uses...
data/serializer/src/main/java/tech/pegasys/teku/api/schema/merge/BeaconStateMerge.java
Show resolved
Hide resolved
| this.previousEpochParticipation = previousEpochParticipation; | ||
| this.currentEpochParticipation = currentEpochParticipation; | ||
| this.inactivityScores = inactivityScores; | ||
| this.currentSyncCommittee = currentSyncCommittee; | ||
| this.nextSyncCommittee = nextSyncCommittee; |
There was a problem hiding this comment.
kind of a repeat, but basically all these fields are now incorrect in the api returns, which is why I'm suggesting putting their names back unless we have a good reason.
There was a problem hiding this comment.
at the end I was more close to have cammelCase stuff, if you agree, so lets have it as it is for now and wait for the new API framework to clean everything up
| @Schema( | ||
| name = "parent_hash", | ||
| type = "string", | ||
| format = "byte", | ||
| pattern = PATTERN_BYTES32, | ||
| description = DESCRIPTION_BYTES32) | ||
| public final Bytes32 parentHash; | ||
|
|
There was a problem hiding this comment.
i think all of these have come out incorrectly because of the abstract class usage. i'm not sure how you'd get around that, but the swagger is all busted for this class...
There was a problem hiding this comment.
I guess you are not seeing the latest version, can you do a clean rebuild?
tbenr
force-pushed
the
merge_beacon_apis
branch
2 times, most recently
from
cbf5bcf
to
866d70a
Compare
|
getState API getBlock API @rolfyone to me looks ok now.. hopefully! |
improvement in BeaconBlock* constructors
add BeaconStateMerge in State schema annotation add BeaconBlockMerge in UnsignedBlock schema annotation fix BeaconStateAltair schema properties names
add BeaconStateMerge in State schema annotation add BeaconBlockMerge in UnsignedBlock schema annotation fix BeaconStateAltair schema properties names add UInt256 serializer\deserializer expand BeaconState api UT
…rge, ExecutionPayload and ExecutionPayloadHeader
tbenr
force-pushed
the
merge_beacon_apis
branch
from
8c79eac
to
ecbc02a
Compare
PR Description
add Merge support in REST beacon APIs
related to #4504
SignedBeaconBlockandBeaconBlock*Documentation
documentationlabel to this PR if updates are required.Changelog