Skip to content
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

Merged
merged 6 commits into from
Aug 29, 2024

Conversation

LGLO
Copy link
Contributor

@LGLO LGLO commented Aug 12, 2024

Description

Add starting_timestamp function for Slot 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 on SlotDuration which is a parameter to the added function. This function can be seen as dual to existing fn 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 and D0-easy but I cannot add them using GH interface.

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T
    required)
    • External contributors: ask maintainers to put the right label on your PR.
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@seadanda seadanda added the T0-node This PR/Issue is related to the topic “node”. label Aug 12, 2024
@ggwpez ggwpez requested a review from a team August 12, 2024 12:42
Comment on lines 94 to 95
/// The first timestamp of the slot. Returns None if would overflow for given SlotDuration.
pub fn first_timestamp(&self, slot_duration: SlotDuration) -> Option<Timestamp> {
Copy link
Member

@davxy davxy Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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)

Copy link
Member

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.

Copy link
Contributor Author

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]>
@github-actions github-actions bot requested a review from davxy August 14, 2024 09:48
Copy link

Review required! Latest push from author must always be reviewed

@github-actions github-actions bot requested review from bkchr and davxy August 19, 2024 09:29
@LGLO LGLO requested a review from gilescope August 26, 2024 13:18
@bkchr bkchr enabled auto-merge August 28, 2024 21:22
@bkchr
Copy link
Member

bkchr commented Aug 29, 2024

@LGLO can you please merge master again?

@LGLO
Copy link
Contributor Author

LGLO commented Aug 29, 2024

@bkchr done

@bkchr bkchr added this pull request to the merge queue Aug 29, 2024
Merged via the queue into paritytech:master with commit 9374643 Aug 29, 2024
183 of 184 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants