feat: add block_hash and builder_index to block event - #9854
feat: add block_hash and builder_index to block event#9854markolazic01 wants to merge 9 commits into
block_hash and builder_index to block event#9854Conversation
| const gloasFields = isGloasBeaconBlock(block.message) | ||
| ? { | ||
| blockHash: toRootHex(block.message.body.signedExecutionPayloadBid.message.blockHash), | ||
| builderIndex: block.message.body.signedExecutionPayloadBid.message.builderIndex, | ||
| } | ||
| : {}; | ||
| this.emitter.emit(routes.events.EventType.block, { | ||
| block: blockRootHex, | ||
| slot: blockSlot, | ||
| executionOptimistic: blockSummary != null && isOptimisticBlock(blockSummary), | ||
| ...gloasFields, | ||
| }); |
There was a problem hiding this comment.
can this be made type safe somehow?
There was a problem hiding this comment.
ah it probably is fine either way, I don't think there is a good way to enforce the event itself to require blockhash and builderindex after gloas
There was a problem hiding this comment.
I haven't found a better solution than this so far
| blockHash: stringType, | ||
| builderIndex: ssz.BuilderIndex, |
There was a problem hiding this comment.
nit: I would like these fields after block and before executionOptimistic
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
just some quick drive-by comments 😄
|
thank you very much @nflaig 😃 |
|
There was e2e action fail which seems to be fixable with a re-run. |
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
thanks @markolazic01 for looking into this, I am a bit skeptical after seeing the implementation if this is really a good direction, besides the fact that it's kinda implicit that clients should add these fields on the spec side, it seems also kinda error prone on the implementation side, and seeing that it looks a bit hacky even on our code I am not so certain it's a good direction for the spec
| fromJson: (json) => | ||
| (config.getForkSeq((json as {slot: Slot}).slot) >= ForkSeq.gloas ? blockGloas : blockBase).fromJson(json), | ||
| }, | ||
| [EventType.blockGossip]: new ContainerType( |
There was a problem hiding this comment.
kinda warrants the question if we should also update block_gossip event
There was a problem hiding this comment.
makes sense, we should probably do it
I agree, I am happy to reimplement this if you have another approach in mind, at least we discovered that this isn't the best way to do it. We can discuss a new solution on discord. |
## Motivation Split out from the block / builder event PRs (#9854, #9875, #9876) as a standalone change, as suggested by @markolazic01. The `eventstream` handler in `getEventsApi` forwards every emitter event through a single `onEvent({type: topic, message: data})` call, where `topic` is the full `EventType` union and `data` (and therefore `message`) is `any`. That object literal only type-checks via TypeScript's discriminated-union distribution path (`typeRelatedToDiscriminatedType`), which bails out once the number of source discriminant combinations exceeds 25. `EventType` currently has exactly **25** members, so it compiles today. Adding a **26th** event tips it over the cap and fails with: ``` TS2345: Argument of type '{ type: EventType; message: any; }' is not assignable to parameter of type 'BeaconEvent'. Types of property 'type' are incompatible. Type 'EventType' is not assignable to type 'EventType.<lastMember>'. ``` which is why each new-event PR currently has to add this cast. Landing it once here unblocks those PRs without each carrying the change. ## Description Assert `routes.events.BeaconEvent` at the `onEvent` call. The cast only makes explicit the type erasure that already exists at this `chain.emitter` boundary — `message` is `any`, so the topic/message pairing was never verified by the compiler regardless. The only cast-free alternative would be to construct the event per-topic instead of funneling every topic through one union-typed `onEvent({type, message})` call, which is a larger refactor not worth it here. **No runtime or current-compile behavior change** on `unstable` (25 `EventType` members). --- 🤖 Generated with AI assistance --------- Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
2 participants
Motivation
Adapting block event for Gloas PoC.
Description
Introduces 2 new fields,
block_hashandbuilder_indexto the block event.Related wiring and test adaptation.
Skipping oapi spec test for
blockevent until spec tests are updated with new event fields.AI Assistance Disclosure
Used Claude to audit the changes.