-
Notifications
You must be signed in to change notification settings - Fork 65
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
update electra specs to v1.5.0-alpha.6
#163
Conversation
Don't you need the |
All changes to the so I guess, no? :D |
@pk910 when this is ready please tag me as a reviewer and I'll merge it. Thanks. |
Oh yea, it's ready @mcdee :) However, I haven't been able to test it yet as there is no client ready for alpha.6 |
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.
LGTM! A couple questions & one suggestion.
if int(d) >= len(ForkChoiceNodeValidityStrings) { | ||
if uint64(d) >= uint64(len(ForkChoiceNodeValidityStrings)) { |
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.
What's the rationale for this change? I see this in a few other spots too.
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.
This change is coming from master (50179b8).
I've merged in master due to unrelated linter issues that were already fixed in master.
sorry for the messed up diff.
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.
Ah okay, that makes sense. No worries!
intVal, err := strconv.ParseUint(v, 10, 64) | ||
if err == nil && intVal != 0 { | ||
intVal, err := strconv.ParseInt(v, 10, 64) | ||
if err == nil && intVal >= 0 { |
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.
This allows a zero value now. Is that intended?
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 as above. coming from master (50179b8).
@pk910, I've made a similar PR for I noticed this when integrating your branch (this PR) into |
Yea, this looks right. The |
Oh sorry for the confusion, I saw the errors, then realized that Everything is sorted out now! |
Updates electra types for latest
v1.5.0-alpha.6
spec release:electra.ExecutionPayload
&electra.ExecutionPayloadHeader
electra.ExecutionRequests
type & added toelectra.BeaconBlockBody