New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Overhaul blinded types into multi-fork #4565
Conversation
21ed66c
to
a7f7775
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Some minor comments
@@ -443,7 +443,12 @@ export function getReturnTypes(): ReturnTypes<Api> { | |||
getSyncCommitteeDuties: ContainerDataExecutionOptimistic(ArrayOf(SyncDuty)), | |||
produceBlock: ContainerData(ssz.phase0.BeaconBlock), | |||
produceBlockV2: WithVersion((fork: ForkName) => ssz[fork].BeaconBlock), | |||
produceBlindedBlock: WithVersion((_fork: ForkName) => ssz.bellatrix.BlindedBeaconBlock), | |||
produceBlindedBlock: WithVersion((fork: ForkName) => { | |||
if (fork === ForkName.phase0 || fork === ForkName.altair) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ForkSeq and < ForkSeq.bellatrix comparator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for packages/config/src/forkConfig/index.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in packages/config/src/forkConfig/index.ts
typescript can't infer with seq
comparision that fork is not pre bellatrix causing type issues for accessing ssz.allForksBlinded[forkName]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also same here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated the bellatrix blinded types for phase0 and altair for type consistency, the check here is no more required, while the check in packages/config/src/forkConfig/index.ts
is now using seq
comparision. let me know if this approach is good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, yeah then it's okay typesafety always wins
altair: { | ||
BeaconBlockBody: bellatrix.BlindedBeaconBlockBody, | ||
BeaconBlock: bellatrix.BlindedBeaconBlock, | ||
SignedBeaconBlock: bellatrix.SignedBlindedBeaconBlock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually prefer your original approach of using multiple ForkName comparisions and only defining the types for forks with blinded blocks. Sorry for the back and forth!
efa304a
to
ac307a2
Compare
The blinded types currently used (for dev simplicity) are only of
bellatrix
This PR overhauls blinded types into multi-fork in preparation for the
capella
hardfork