Per-things trait. #4904
Per-things trait. #4904
Conversation
kianenigma
added
A0-please_review
kianenigma
requested review from
andresilva,
Demi-Marie and
mxinden
as code owners
kianenigma
requested review from
expenses and
gui1117
and removed request for
andresilva,
mxinden and
Demi-Marie
primitives/arithmetic/src/traits.rs
Outdated
| /// Arithmetic types do all the usual stuff you'd expect numbers to do. They are guaranteed to | ||
| /// be able to represent at least `u32` values without loss, hence the trait implies `From<u32>` | ||
| /// and smaller ints. All other conversions are fallible. | ||
| pub trait SimpleArithmetic: |
There was a problem hiding this comment.
So, SimpleArithmetic is already somewhat not that expressive. But splitting this now into 3 traits who all are doing basically the same.
We should up with something better.
- Have one trait, probably just called
Arithmetic. MiniArithmeticis just a subset ofSimpleArithmeticor? We probably can just mergeArithmeticandMiniArithmetic.SimpleArithmeticshould get some better name, IDK :D The name should give a clear indication what the difference of the trait is.
There was a problem hiding this comment.
-
Agree that the
Simpleprefix is not cool. -
Mini Arithmetic is not a subset. it is actually a superset logically, since it has the same operations but can be any size, while Simple Arithmetic can be u32 or bigger. But this is not visible trait-wise. Neither can be composed of the other one, at least in the current format.
Hence, I created the third one:
BasicArithmeticis just operations with no assumption on sizeMiniandSimplecan go on an compose it + add limitation on size.
Aside from the fact that Simple is not a nice prefix, I think this is actually quite nice and clean.
There was a problem hiding this comment.
- Yeah, I actually meant it the way you described it :D
Mini isn't a nice prefix as well :P Maybe U8Arithmetic + U32Arithmetic? To make it clear on reading?
There was a problem hiding this comment.
ahh and the fact that the prefix Mini also doesn't make much sense anymore :D
How about this
RuntimeArithmetic for the one which is u32 and above, since we also use it for blocknumber etc.
Arithmetic for the one which is generic on size.
primitives/arithmetic/src/traits.rs
Outdated
|
|
||
| /// Same as [`RuntimeArithmetic`] for integer types of any size, without any assumption of their size | ||
| /// being at least `u32`. | ||
| pub trait AnyArithmetic: |
There was a problem hiding this comment.
I still don't understand the reason for this?
Why do we need this? And it puts a requirement on the size by adding From<u8> :P
All these trait bounds can be moved to the BaseArithmetic, beside of the From<u8>. Will also this could be moved, because u8 is the smallest possible type.
There was a problem hiding this comment.
Let me see if I get you right:
What you recommend is putting all the TryInto<_>, TryFrom<_>, UniqueSaturatedInto<_> and UniqueSaturatedFrom<_>s in Base for all the 6 major integer types, since we don't know the size there, right? and then we can say AtLeast32Bit: Base + From<u8> + From<u16> + From<u32>.
The only minor problem with that would be that some of the traits would be duplicate and don't really make sense. I.e. a type ends up being From<u32> + TryFrom<u32>. Is this cool?
There was a problem hiding this comment.
- Yeah base takes all these trait bounds and I also would give it
From<u8>(there is no datatype that is smaller than u8) - Every type implements
From<Self>and also
impl<T, U> TryFrom<U> for T where
U: Into<T>,
This means that every type automatically implements TryFrom for all types it implements From for.
primitives/arithmetic/src/traits.rs
Outdated
| /// Arithmetic types do all the usual stuff you'd expect numbers to do. They are guaranteed to | ||
| /// be able to represent at least `u32` values without loss, hence the trait implies `From<u32>` | ||
| /// and smaller ints. All other conversions are fallible. | ||
| pub trait RuntimeArithmetic: |
There was a problem hiding this comment.
I think we should call it AtLeast32Bit. The name RuntimeArithmetic doesn't make sense here, as this is nothing we enforce in the runtime.
There was a problem hiding this comment.
32BitArithmetic?
There was a problem hiding this comment.
That is not right, because we are not doing 32bit arithmetic here :D We just add From<u32> and From<u16>. That does say anything about the arithmetic.
There was a problem hiding this comment.
I am not fully happy with AtLeast32Bit but we'll go with it for now since I cannot come up with any other name :D 🤷🏻♂️
There was a problem hiding this comment.
Yeah me neither :D But I also don't have any better idea :(
* Give perthigns the trait it always deserved. * Make staking and phragmen work with the new generic per_thing * Make everything work together 🔨 * a bit of cleanup * Clean usage * Bump. * Fix name * fix grumbles * hopefully fix the ui test * Some grumbles * revamp traits again * Better naming again.
Makes
Per-XXXimplement a trait for their common functionality.Used in
sp-phragmen, so henceforth the caller of Phragmén can specify their desired accuracy of results. This is needed for #4517. Making a separate PR for the sake of easy review.