refactor: improve types package to use forks as generics #6825
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #6825 +/- ##
============================================
- Coverage 62.67% 62.56% -0.11%
============================================
Files 578 575 -3
Lines 61220 61066 -154
Branches 2113 2116 +3
============================================
- Hits 38367 38207 -160
- Misses 22815 22820 +5
- Partials 38 39 +1 |
|
Converted to draft to resolve huge conflicts after #6749 merged. |
nazarhussain
force-pushed
the
nh/types-refactor
branch
from
2670246
to
b276a09
Compare
There was a problem hiding this comment.
The Motviation of this PR is not obvious to me
Simplify the usage of Fork specific types around the repo.
Looking at the diff, I don't see a single line where it simplified things or removed unsafe type casts (it rather added more). The semantics are mostly the same, we still use "all forks" for types that do not exist on all forks. I don't think that's a big issue though, I always though of "all forks" as "all supported forks" anyways.
The extra type machinery required looks scary to maintain, hoping this just works and does not require type debugging in the future.
Would be great to get more examples on type simplification we plan with this in the future to get a better picture of the whole solution, as I understood from the presentation there are 3 stages.
Is this in any way also related to #6799?
(will do more detailed review once we have more feedback from others)
packages/beacon-node/src/chain/blocks/verifyBlocksSignatures.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/test/e2e/doppelganger/doppelganger.test.ts
Outdated
Show resolved
Hide resolved
It was much clearly explained in the presentation.
We had the usage of
It is not possible because there is no name-space
If you observe where we are passing fork collection to generics are all unsafe types referred via
I don't see it scary, it's simple generic types we used everywhere. During this PR I faced some issues which were mostly assigning
Once you start using these generics types, you will find these more flexible and useful. Anyhow have plans to add docs guiding steps to add more generic types, or adding new forks.
Yes this refactoring will facilitate to have more typesafe code for upcoming forks. |
We should document the main points in the motivation of the PR. I tried to rewatch the recording but it seems to be no longer accessible. Or at least share the slides via link, and reference in PR.
this is what I don't quite get, we are still doing this, but instead of e.g. previous allForks.SignedBlindedBeaconBlock;and now SignedBeaconBlock<ForkAll, "blinded">;what's the difference between those two? we still claim that blinded beacon block exists on all forks but that's not the case |
|
I think the main benefit we get is more expressiveness wrt features that exist in subsets of all forks.
Maybe worth considering just having a single generic param for all generic types here. so, This may be easier devex, since we won't have to remember which generic types have 2 or more params, and which possibilities exist. Rather, these generic types will have a single generic param for the fork. |
The difference of Now if question you are referring why generics, that I explained in my earlier comment as well. The usage of As we add more forks it will be unmanageable to refer those via |
yes, looks good now. This comment was done on an older state of the branch.
How is this example relevant? Of course there are situations where type-casts are fine but claiming those do not reduce type safety unless you use |
|
Curious if you have explored type inference for the cases were we use the object itself to determine the fork. e.g. in // fork based validations
const fork = state.config.getForkSeq(signedBlock.message.slot);
// Only after altair fork, validate tSyncCommitteeSignature
if (fork >= ForkSeq.altair) {
const syncCommitteeSignatureSet = getSyncCommitteeSignatureSet(
state as CachedBeaconStateAltair,
(signedBlock as altair.SignedBeaconBlock).message // <-- would be nice if it could infer altair here as we check fork above
);
// There may be no participants in this syncCommitteeSignature, so it must not be validated
if (syncCommitteeSignatureSet) {
signatureSets.push(syncCommitteeSignatureSet);
}
}
// only after capella fork
if (fork >= ForkSeq.capella) {
const blsToExecutionChangeSignatureSets = getBlsToExecutionChangeSignatureSets(
state.config,
signedBlock as capella.SignedBeaconBlock // <-- same here
);
if (blsToExecutionChangeSignatureSets.length > 0) {
signatureSets.push(...blsToExecutionChangeSignatureSets);
}
}We do a lot of those type casts in different places, while the type casts are safe here would be nice to have. This was definitely impossible before (without generics) could work now...assuming typescript is smart enough but we would likely need a wrapped object like this |
packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts
Outdated
Show resolved
Hide resolved
| @@ -48,4 +49,4 @@ export type LightClientOptimisticUpdate = ValueOf<typeof ssz.LightClientOptimist | |||
| export type LightClientStore = ValueOf<typeof ssz.LightClientStore>; | |||
|
|
|||
| export type ProducedBlobSidecars = Omit<BlobSidecars, "signedBlockHeader" | "kzgCommitmentInclusionProof">; | |||
| export type Contents = Omit<BlockContents, "block">; | |||
| export type Contents = Omit<BlockContents<ForkName.deneb>, "block">; | |||
There was a problem hiding this comment.
we can move this out to ../types as well, also don't need to zero down to deneb type
There was a problem hiding this comment.
That will be breaking change for deneb namespace. Want to avoid any such changes.
There was a problem hiding this comment.
its not breaking since there is just deneb in unstable right now, but ok if you wanna follow it up with another PR to not pile up in this PR
| export const allForksBlobs = pick(typesByFork, ...blobsForks); | ||
| export const allForksBlinded = { | ||
| bellatrix: { | ||
| BeaconBlockBody: bellatrix.BlindedBeaconBlockBody, |
There was a problem hiding this comment.
may be we can just carry Blinded.... keys directly in the typesByFork and refer them everywhere as .Blinded...
so this can also be cleaned up
There was a problem hiding this comment.
Will cleanup these type objects some helpers in config object in a separate PR.
|
🎉 This PR is included in v1.20.0 🎉 |
Motivation
Simplify the usage of Fork specific
typesaround the repo.Description
allForks.phase0.BeaconBlocktoBeaconBlock<ForkName.phase0>BeaconBlock<ForkName.phase0 | ForkName.altair>ForkExecution,ForkLightclientCloses #4656
Steps to test or reproduce