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

Draft sp-time #3298

Closed
wants to merge 3 commits into from
Closed

Draft sp-time #3298

wants to merge 3 commits into from

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Feb 12, 2024

Ideas about Time types for #3268

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
}

/// Calculates the time since a given instant.
pub trait Since<D: Duration> {
Copy link
Contributor

@gpestana gpestana Feb 12, 2024

Choose a reason for hiding this comment

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

Expecting this to be highly WIP but IMO a Duration trait should expose both fn since and fn until, which probably can be implemented by default given CheckedAd and CheckedSub. In any case, I'm not seeing any case where having a representation of a duration without being able to "traverse time" is useful.

Copy link
Member Author

@ggwpez ggwpez Feb 12, 2024

Choose a reason for hiding this comment

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

Expecting this to be highly WIP

Yes thanks for the feedback 😄

I would also rather like to use CheckedAdd and CheckedSub, but they can only operate on (Instant, Instant) and not (Instant, Duration). So I think that we need new traits for this.
These Since and Until are currently used to glue together the Instant and the Duration types.

The Add trait can specify the right-hand operand, but the Checked* traits somehow dont: https://docs.rs/num/latest/num/trait.CheckedAdd.html

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah kinda sucks that they don't.

what about Instant and Duration being the same type/trait? Can you give some examples of why there needs to be two traits for these two concepts?

Copy link
Member Author

@ggwpez ggwpez Feb 29, 2024

Choose a reason for hiding this comment

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

I put some points down here: #3298 (comment)
I think if we use the same type, then we can only ever have Duration. Since what should it return when we subtract two Instants?
And that duration would need to be anchored somewhere, maybe at genesis time. But then we only ever have relative time and not absolute. It seems a bit weird to me, and the fact that Rust, C++ and other major ecosystems all split it into two types makes me think that they are onto something.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member Author

ggwpez commented Feb 15, 2024

@xlc PTAL

@ggwpez ggwpez marked this pull request as ready for review February 18, 2024 22:54
@ggwpez ggwpez self-assigned this Feb 21, 2024
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I suggest a few more steps to be built with the PoC before further commenting. At the moment it is a bit too raw and it doens't provide an easy way for me to imagine how the traits are used.

I think implementing a OwnBlockNumber, RelayBlockNumber would help (ofc with todos!() in most places) and possibly refactoring one call in scheduler.schedule to showcase the pallet level.

My gut feeling is that there might be a bit of what I call "over-trait-ification" going on, as in too many things are traits: Instant and Duration could be the same concept, Since and Until can be something akin to Add, Sub or standalone methods on the right trait.

@ggwpez
Copy link
Member Author

ggwpez commented Feb 25, 2024

My gut feeling is that there might be a bit of what I call "over-trait-ification" going on

Yea i see it similar. Not sure where to make the cut though.

Instant and Duration could be the same concept,

I thought about this for a while and i think that they cannot. Otherwise we cannot prevent usage errors: When subtracting two Instants, the result is a Duration. Now if the result would be of the same type, then it could be accidentally re-interpreted as an Instant. This would remove semantic clarity that the type system provides us.
Also it is idiomatically the same as the Rust std.

Since and Until can be something akin to Add, Sub or standalone methods on the right trait.

Yea probably. But Since and Until still need to be own things; AFAIK we could re-use the Add and Sub traits, but not the CheckedAdd/Sub or Saturating traits, since they only work when the input and output types match.
But in our case we would like to have Instant - Instant = Duration and Instant + Duration = Instant.
But i will try to make them functions on the type directly.

@xlc
Copy link
Contributor

xlc commented Feb 26, 2024

I guess it looks fine but this is very abstracted and may not be actually what people needs and the only way to tell is to apply it somewhere.

Also maybe there are already time crate exists that we can just use it?

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez requested a review from a team as a code owner February 26, 2024 14:26
@ggwpez
Copy link
Member Author

ggwpez commented Feb 29, 2024

Also maybe there are already time crate exists that we can just use it?

Yea could be. If there is some no-std. But we would still need to wrap it into a type to make it SCALE en/decodable, right?

@xlc
Copy link
Contributor

xlc commented Feb 29, 2024

Yeah that’s right. But at least we are not trying to make a new API.

@ggwpez
Copy link
Member Author

ggwpez commented Jul 17, 2024

Simply using the relay block number seems to be good enough for most cases currently. Closing this until we need a more sophisticated approach.

@ggwpez ggwpez closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants