Introduce checked_div for PerThings and checked_rational_mul_correction#14673
Introduce checked_div for PerThings and checked_rational_mul_correction#14673
checked_div for PerThings and checked_rational_mul_correction#14673Conversation
Gioyik
changed the title
checked_div for PerThings and checked_rational_mul_correction
|
there are many @ggwpez do you think it's fine to make the switch in those files and include it in this PR or those should stay as |
| /// Take the remainder of `x / denom` and multiply by `numer / denom`. The result can be added | ||
| /// to `x / denom * numer` for an accurate result. | ||
| fn rational_mul_correction<N, P>(x: N, numer: P::Inner, denom: P::Inner, rounding: Rounding) -> N | ||
| fn unchecked_rational_mul_correction<N, P>( |
There was a problem hiding this comment.
Existing Rust idioms have checked_ arithmetic variants with prefixing, not unchecked variants. We will stay with those unless/until a very strong argument to break with Rust idioms are presented.
There was a problem hiding this comment.
super interesting, I was discussing with @ggwpez over chat the rational behind the checked and why functions do not behave as checked ones by default.
this clarifies everything
gavofyork
left a comment
gavofyork
left a comment
There was a problem hiding this comment.
Cannot be accepted in the present state.
|
bot fmt |
|
@kianenigma https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3320751 was started for your command Comment |
|
@kianenigma Command |
|
now there are https://github.com/paritytech/substrate/blob/gio/checked_fn/primitives/arithmetic/src/per_things.rs#L482 do we want to replace with the checked version of the function? |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
| } | ||
|
|
||
| /// Returns 2^128 / a | ||
| /// `checked_div()` could be used here but is not possible to `unwrap()` inside a `const fn` |
There was a problem hiding this comment.
I think you can use a match or if in const functions.
But for this function i would probably make more sense to add a checked_div128 function instead of changing it.
| return Err("Division by zero") | ||
| } | ||
|
|
||
| Ok(multiply_by_rational_with_rounding(a, b, c, r)) |
There was a problem hiding this comment.
As a general consideration of design:
I think its easier to put the actual logic inside the checked_ function such that the unchecked variant is just checked_function().expect() or checked_function.unwrap_or(whatever) since otherwise you have the case like now, where the checked variant does not have any logic and can therefore not ensure that it wont ever panic.
| if c == 0 { | ||
| return None | ||
| } |
There was a problem hiding this comment.
This is actually making it less secure, which is why i would rather put the logic in the checked variant (basically same comment as above).
| if P::Inner::is_zero(&denom) { | ||
| return Err("Division by zero") | ||
| } | ||
| Ok(rational_mul_correction::<N, P>(x, numer, denom, rounding)) |
There was a problem hiding this comment.
Now you have the implication that the underlying function can only error iff denom is null, which is an implementation detail. Again i would rather move the logic into the checked variant and then call it from the unchecked one.
| impl CheckedDiv for $name { | ||
| #[inline] | ||
| fn checked_div(&self, rhs: &Self) -> Option<Self> { | ||
| let q = rhs.0; |
There was a problem hiding this comment.
I think there should be an rhs.is_zero()
| return None | ||
| } | ||
|
|
||
| Some(*self / *rhs) |
There was a problem hiding this comment.
probably want to use the deconstruct() function here to get the inner value.
5 participants
Fixes paritytech/polkadot-sdk#199
Introduces
checked_divandchecked_rational_mul_correctionNotes:
checked_div()inPerThingshave to options to complete the division, however, I am not sure which one would be the best, both seem to output the same.The checked behavior ofchecked_rational_mul_correctioncan be introduced formultiply_by_rational_with_rounding, however it implies changing all the tests and developers using it might need to change it to expectOption<u128>instead ofu128. So I am holding changes if this is not desired.