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

Experimental: Strongly typed types of time #1907

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jul 11, 2024

Description

The idea of this PR is to have Millis and Seconds as different types for the compiler. This helps to never mix by error different duration units and improve the readability of some parts of the codebase.

To achieve this, we need:

  • A wrapper type over integers that allows us different integer-like types to tell the compiler not to mix types (check num_wrapper.rs file).
  • Some explicit conversions from seconds to millis that will change the value accordingly
  • Implement everything from BaseArithmetic to be able to use this types as normal u64 (i.e. give to pallet_timestamp's Moment the new Millis type.

How times should be used now in the code base?

  • If the pallet just carries a time unit, without knowing what unit is, use a generic Moment (probably most of the times a <<T as Config>::Time as Time>::Moment), as usual. You can use the trait TimeUnit to get the time as the unit you want.
  • If the pallet uses a moment type that specifically represents millis, seconds or days, you should use cfg_primitives::{Millis, Seconds, Days, ..}. There is no need for an associated type in the pallet.

Blockers

It seems like parity_scale_codec doesn't allow to implement HasCompact trait required by BasicArithmetic:

Remainders

  • Requirement::DelayTime uses a u32 to hold seconds when it should be a u64 wrapped into Seconds type. Nevertheless, this requires a migration in an storage of pool-system.

@lemunozm lemunozm added Q5-hard Can be done by an experienced coder with a good knowledge of the codebase. P2-nice-to-have Issue is worth doing. I6-refactoring Code needs refactoring. labels Jul 11, 2024
@lemunozm lemunozm self-assigned this Jul 11, 2024
@lemunozm lemunozm force-pushed the exp/time-safety-types branch from f18650a to 19a3acd Compare July 11, 2024 15:19
Comment on lines 24 to 29
/// ```
/// # use crate::num_wrapper::NumWrapper;
///
/// struct Id1;
/// struct Id2;
///
/// type FooU64 = NumWrapper<u64, Id1>;
/// type BarU64 = NumWrapper<u64, Id2>;
/// ```
Copy link
Contributor Author

@lemunozm lemunozm Jul 11, 2024

Choose a reason for hiding this comment

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

FooU64 and BarU64 are different types for the compiler, and you can implement different traits to them. In memory and for serde and codec there are just plain u64

@lemunozm lemunozm force-pushed the exp/time-safety-types branch 2 times, most recently from babfbbe to 09cda36 Compare August 2, 2024 07:23
libs/utils/src/time.rs Outdated Show resolved Hide resolved
@lemunozm lemunozm force-pushed the exp/time-safety-types branch 3 times, most recently from fb6d6f5 to 6a790fa Compare August 5, 2024 14:33
@lemunozm lemunozm force-pushed the exp/time-safety-types branch from 431c9dc to 7dfc4a4 Compare August 19, 2024 08:04
@lemunozm lemunozm force-pushed the exp/time-safety-types branch from 7dfc4a4 to 73a732f Compare August 19, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I6-refactoring Code needs refactoring. P2-nice-to-have Issue is worth doing. Q5-hard Can be done by an experienced coder with a good knowledge of the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant