-
Notifications
You must be signed in to change notification settings - Fork 741
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
Add an utility function to get the first timestamp of a slot #5316
Add an utility function to get the first timestamp of a slot #5316
Conversation
8633f8c
to
441b6d3
Compare
/// The first timestamp of the slot. Returns None if would overflow for given SlotDuration. | ||
pub fn first_timestamp(&self, slot_duration: SlotDuration) -> Option<Timestamp> { |
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.
/// The first timestamp of the slot. Returns None if would overflow for given SlotDuration. | |
pub fn first_timestamp(&self, slot_duration: SlotDuration) -> Option<Timestamp> { | |
/// The timestamp of the slot given the duration. | |
/// | |
/// Returns `None` if would overflow for given `SlotDuration`. | |
pub fn to_timestamp(self, slot_duration: SlotDuration) -> Option<Timestamp> { |
I'd rather change the api to this (Slot is Copy)
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.
Or maybe just call it timestamp
. Not really required to have any longer name.
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've renamed to timestamp
, but kept "first" in the documentation, because it returns the first timestamp of the slot
Co-authored-by: Squirrel <[email protected]>
Review required! Latest push from author must always be reviewed |
Co-authored-by: Davide Galassi <[email protected]>
@LGLO can you please merge master again? |
@bkchr done |
9374643
Description
Add
starting_timestamp
function forSlot
type.Integration
This is an addition of public function to a type, so integration should be seamless for idiomatic use of Rust.
Review Notes
Since
Slot
is just a slot number, the it's starting timestamp depends onSlotDuration
which is a parameter to the added function. This function can be seen as dual to existingfn from_timestamp
.Because there is a potential for overflow, the return type is
Option
.Q1: should I introduce tests for in this crate and add cases for both case: overflow (
None
) and no overflow (Some
)?Q2: How can I add labels? IMO they should be
T0-node
andD0-easy
but I cannot add them using GH interface.Checklist
T
required)