-
Notifications
You must be signed in to change notification settings - Fork 998
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
Fix #1090 #1092
Fix #1090 #1092
Conversation
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.
👍👍
@@ -647,7 +647,7 @@ def get_previous_epoch(state: BeaconState) -> Epoch: | |||
Return the current epoch if it's genesis epoch. | |||
""" | |||
current_epoch = get_current_epoch(state) | |||
return (current_epoch - 1) if current_epoch > GENESIS_EPOCH else current_epoch | |||
return GENESIS_EPOCH if current_epoch == GENESIS_EPOCH else current_epoch - 1 |
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.
Tiny tiny nitpick: the previous version looks fine to me, and slightly more efficient since we expect in most cases, current_epoch
is greater than GENESIS_EPOCH
.
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 was trying to make it clear that current_epoch - 1
is not even "considered" unless current_epoch != GENESIS
. Not too fussed either way :)
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.
So in python, the left side is never run if the conditional is False, but I think this makes it slightly clearer to be careful with the subtraction. I'm oky with as is
Avoid signed integers