Remove all intermediary ChainEvents #4973
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
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
force-pushed
the
dapplion/direct-events
branch
from
40b0630 to
caa0d3a
Compare
commit 40b0630 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed Jan 4 20:50:58 2023 +0800 Update tests commit a6df872 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed Jan 4 12:55:07 2023 +0800 Make enum const commit c13f17c Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed Jan 4 12:40:17 2023 +0800 Remove middleware ChainEvents commit 41d11aa Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed Jan 4 11:28:10 2023 +0800 Remove intermediary event ChainEvent.forkChoiceReorg
dapplion
force-pushed
the
dapplion/direct-events
branch
from
caa0d3a to
1bbe1ec
Compare
dapplion
commented
Jan 5, 2023
| @@ -350,3 +349,26 @@ describe("executionEngine / ExecutionEngineHttp", function () { | |||
| console.log("\n\nDone\n\n"); | |||
| } | |||
| }); | |||
|
|
|||
| async function retrieveCanonicalWithdrawals(bn: BeaconNode, fromSlot: Slot, toSlot: Slot): Promise<number> { | |||
There was a problem hiding this comment.
@g11tech Can you review if this assertion is good for your test? Re-wrote the previous event based, into an after the fact one
dapplion
commented
Jan 5, 2023
| @@ -217,6 +219,9 @@ describe("executionEngine / ExecutionEngineHttp", function () { | |||
| // delay a bit so regular sync sees it's up to date and sync is completed from the beginning | |||
| const genesisSlotsDelay = 8; | |||
|
|
|||
| // TODO for g11tech: Why 4? Provide rationale for the number | |||
There was a problem hiding this comment.
@g11tech: Why 4? Provide rationale for the number
There was a problem hiding this comment.
from the test length run i noted that if there is no issue, withdrawal blocks come out to be 6, so i chose 4 as safe number incase there are any skipped slots for whatever reasons (sometimes have seen it happening)
twoeths
approved these changes
Jan 5, 2023
nflaig
reviewed
Jan 23, 2023
| @@ -5,7 +5,7 @@ import {RouteDef, TypeJson} from "../../utils/index.js"; | |||
|
|
|||
| // See /packages/api/src/routes/index.ts for reasoning and instructions to add new routes | |||
|
|
|||
| export enum EventType { | |||
| export const enum EventType { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
IMO this should be reinstated as an enum
There was a problem hiding this comment.
Also re-exported as necessary to allow for downstream reuse
There was a problem hiding this comment.
changed in #5042, seems to be re-exported already so we should be fine there
There was a problem hiding this comment.
Potentially unwanted side-effect from attempting to make EventType from API + ChainEvent work for the same handlers. Happy to revert it to enum
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
BeaconNode source emits events meant for the API through intermediary ChainEvents. That's not necessary since the few users of the internal events can just listen to the events defined as in the API.
Description
Remove all intermediary ChainEvents