-
Notifications
You must be signed in to change notification settings - Fork 63
feat(pbs): electra support #266
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
Conversation
eserilev
left a comment
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 general I think this looks fine. I tried to pay special attention to electra specific changes
As you mentioned in a previous comment, much of the fork variant complexity can be refactored in a future PR.
crates/common/src/pbs/types/spec.rs
Outdated
| type MaxAttestations = typenum::U128; | ||
|
|
||
| // Ignore Electra | ||
| type MaxValidatorsPerSlot = typenum::U0; |
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.
nit: I'm pretty sure this doesn't matter pre-electra but technically MaxValidatorsPerSlot should still be U131072 (i.e. MaxValidatorsPerCommittee * MaxCommitteesPerSlot) pre-electra
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.
fixed in 061966b
| pub enum VersionedResponse<D, E> { | ||
| #[serde(rename = "deneb")] | ||
| Deneb(D), | ||
| #[serde(rename = "electra")] | ||
| Electra(E), |
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 a future refactor we should be able to revert to the previous VersionedResponse struct definition
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.
yep fair point, my thinking here would be that in this case i can be sure that the version field matches with the correct payload. Although probably could have also had a simple validate check for this tbh. After pectra I'm leaning towards removing all the deneb specific code to avoid cluttering types
| } | ||
|
|
||
| fn extra_validation( | ||
| fn extra_validation_deneb( |
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.
nit: is this validation actually deneb specific?
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.
good point, had changed in a previous refactor. Fixed in 061966b
new electra PR based on #257 from @JasonVranek