Update epoch_from_block_number for clarity
#4037
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Upon analyzing the conditional logic of the `epoch_from_block_height` it seems that we really want the epoch to increase on the successor of the multiples of the `epoch_height`. The conditional branches make this less obvious with 3 separate computed results for the calculation. It seems that this can be achieved with a single statement. In addition, extra clarity as to the parameter's intents can be added by renaming `epoch_height` to something that indicates what the parameter is symbolizing.
Ayiga
force-pushed
the
ts/enh/epoch_from_block_number_adjustments
branch
from
0d422fa to
27c53a0
Compare
pls148
reviewed
Jan 18, 2025
crates/types/src/utils.rs
Outdated
| block_number / epoch_height + 1 | ||
| } | ||
| pub fn epoch_from_block_number(block_number: u64, blocks_per_epoch: u64) -> u64 { | ||
| (block_number + blocks_per_epoch - 1) / blocks_per_epoch |
There was a problem hiding this comment.
This would break if blocks_per_epoch is 0, which happens in cases when epochs aren't configured yet.
Also I just merged a change that adds a new fn option_epoch_from_block_number right after this one, I'd rebase and change that one too
There was a problem hiding this comment.
Oh, you are right. I was reaching the zero check as being against block_number instead of epoch_height for some reason. Good catch on my misinterpretation.
With the previous change the explicit check for `blocks_per_epoch` being zero was removed, thereby removing the protection of division by 0. This check should be re-added making this change somewhat less valuable as we're unable to remove all conditionals. This change has been made to re-add the initial conditional in order to ensure compatibility with the previous implementation.
ss-es
approved these changes
Jan 21, 2025
|
approved, though +1 to merging in main and updating the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4036
This PR:
Implements the suggested improvements that I created in issue #4036.
Specifically, it renames
epoch_heighttoblocks_per_epoch, and it replaces the conditional logic with the 3 separate cases with a single calculation.This PR does not:
The overall logic / return value of the function. Mathematically they are the same. The only potential downsize is removing one epoch of potential usage from consideration before integer overflow occurs.