Use sensible maths for from_rational

primitives/arithmetic/src/helpers_128bit.rs

@@ -182,8 +182,8 @@ mod double128 {
 	}
 }
 
-/// Returns `a * b / c` and `(a * b) % c` (wrapping to 128 bits) or `None` in the case of
-/// overflow and c = 0.
+/// Returns `a * b / c` (wrapping to 128 bits) or `None` in the case of
+/// overflow.
 pub const fn multiply_by_rational_with_rounding(
 	a: u128,
 	b: u128,

primitives/arithmetic/src/lib.rs

@@ -45,7 +45,7 @@ pub use per_things::{
 	InnerOf, MultiplyArg, PerThing, PerU16, Perbill, Percent, Permill, Perquintill, RationalArg,
 	ReciprocalArg, Rounding, SignedRounding, UpperOf,
 };
-pub use rational::{Rational128, RationalInfinite};
+pub use rational::{Rational128, RationalInfinite, MultiplyRational};
 
 use sp_std::{cmp::Ordering, fmt::Debug, prelude::*};
 use traits::{BaseArithmetic, One, SaturatedConversion, Unsigned, Zero};

primitives/arithmetic/src/per_things.rs

@@ -26,7 +26,7 @@ use codec::{CompactAs, Encode};
 use num_traits::{Pow, SaturatingAdd, SaturatingSub};
 use sp_std::{
 	fmt, ops,
-	ops::{Add, AddAssign, Div, Rem, Sub},
+	ops::{Add, Sub},
 	prelude::*,
 };
 
@@ -46,6 +46,7 @@ pub trait RationalArg:
 	+ Unsigned
 	+ Zero
 	+ One
+	+ crate::MultiplyRational
 {
 }
 
@@ -58,7 +59,8 @@ impl<
 			+ ops::AddAssign<Self>
 			+ Unsigned
 			+ Zero
-			+ One,
+			+ One
+			+ crate::MultiplyRational,
 	> RationalArg for T
 {
 }
@@ -105,7 +107,7 @@ pub trait PerThing:
 	+ Pow<usize, Output = Self>
 {
 	/// The data type used to build this per-thingy.
-	type Inner: BaseArithmetic + Unsigned + Copy + Into<u128> + fmt::Debug;
+	type Inner: BaseArithmetic + Unsigned + Copy + Into<u128> + fmt::Debug + crate::MultiplyRational;
 
 	/// A data type larger than `Self::Inner`, used to avoid overflow in some computations.
 	/// It must be able to compute `ACCURACY^2`.
@@ -115,7 +117,8 @@ pub trait PerThing:
 		+ TryInto<Self::Inner>
 		+ UniqueSaturatedInto<Self::Inner>
 		+ Unsigned
-		+ fmt::Debug;
+		+ fmt::Debug
+		+ crate::MultiplyRational;
 
 	/// The accuracy of this type.
 	const ACCURACY: Self::Inner;
@@ -538,39 +541,6 @@ where
 	rem_mul_div_inner.into()
 }
 
-/// Just a simple generic integer divide with custom rounding.
-fn div_rounded<N>(n: N, d: N, r: Rounding) -> N
-where
-	N: Clone
-		+ Eq
-		+ Ord
-		+ Zero
-		+ One
-		+ AddAssign
-		+ Add<Output = N>
-		+ Rem<Output = N>
-		+ Div<Output = N>,
-{
-	let mut o = n.clone() / d.clone();
-	use Rounding::*;
-	let two = || N::one() + N::one();
-	if match r {
-		Up => !((n % d).is_zero()),
-		NearestPrefDown => {
-			let rem = n % d.clone();
-			rem > d / two()
-		},
-		NearestPrefUp => {
-			let rem = n % d.clone();
-			rem >= d.clone() / two() + d % two()
-		},
-		Down => false,
-	} {
-		o += N::one()
-	}
-	o
-}
-
 macro_rules! implement_per_thing {
 	(
 		$name:ident,
@@ -687,38 +657,17 @@ macro_rules! implement_per_thing {
 					+ ops::AddAssign<N>
 					+ Unsigned
 					+ Zero
-					+ One,
+					+ One
+					+ $crate::MultiplyRational,
 				Self::Inner: Into<N>
 			{
 				// q cannot be zero.
 				if q.is_zero() { return Err(()) }
 				// p should not be bigger than q.
 				if p > q { return Err(()) }
 
-				let factor = div_rounded::<N>(q.clone(), $max.into(), Rounding::Up).max(One::one());
-
-				// q cannot overflow: (q / (q/$max)) < $max. p < q hence p also cannot overflow.
-				let q_reduce: $type = div_rounded(q, factor.clone(), r)
-					.try_into()
-					.map_err(|_| "Failed to convert")
-					.expect(
-						"`q / ceil(q/$max) < $max`; macro prevents any type being created that \
-						does not satisfy this; qed"
-					);
-				let p_reduce: $type = div_rounded(p, factor, r)
-					.try_into()
-					.map_err(|_| "Failed to convert")
-					.expect(
-						"`p / ceil(p/$max) < $max`; macro prevents any type being created that \
-						does not satisfy this; qed"
-					);
-
-				// `p_reduced` and `q_reduced` are within `Self::Inner`. Multiplication by another
-				// `$max` will always fit in `$upper_type`. This is guaranteed by the macro tests.
-				let n = p_reduce as $upper_type * <$upper_type>::from($max);
-				let d = q_reduce as $upper_type;
-				let part = div_rounded(n, d, r);
-				Ok($name(part as Self::Inner))
+				let max: N = $max.into();
+				max.multiply_rational(p, q, r).ok_or(())?.try_into().map(|x| $name(x)).map_err(|_| ())
 			}
 		}
 
@@ -1862,6 +1811,14 @@ fn from_rational_with_rounding_works_in_extreme_case() {
 	}
 }
 
+#[test]
+fn from_rational_with_rounding_breakage() {
+	let n = 372633774963620730670986667244911905u128;
+	let d = 512593663333074177468745541591173060u128;
+	let q = Perquintill::from_rational_with_rounding(n, d, Rounding::Down).unwrap();
+	assert!(q * d <= n);
+}
+
 implement_per_thing!(Percent, test_per_cent, [u32, u64, u128], 100u8, u8, u16, "_Percent_",);
 implement_per_thing_with_perthousand!(
 	PerU16,

primitives/arithmetic/src/rational.rs

@@ -284,6 +284,50 @@ impl PartialEq for Rational128 {
 	}
 }
 
+pub trait MultiplyRational: Sized {
+	fn multiply_rational(self, n: Self, d: Self, r: Rounding) -> Option<Self>;
+}
+
+macro_rules! impl_rrm {
+	($ulow:ty, $uhi:ty) => {
+		impl MultiplyRational for $ulow {
+			fn multiply_rational(self, n: Self, d: Self, r: Rounding) -> Option<Self> {
+				let sn = (self as $uhi) * (n as $uhi);
+				let mut result = sn / (d as $uhi);
+				let remainder = (sn % (d as $uhi)) as $ulow;
+				if match r {
+					Rounding::Up => remainder > 0,
+					// cannot be `(d + 1) / 2` since `d` might be `max_value` and overflow.
+					Rounding::NearestPrefUp => remainder >= d / 2 + d % 2,
+					Rounding::NearestPrefDown => remainder > d / 2,
+					Rounding::Down => false,
+				} {
+					result = match result.checked_add(1) {
+						Some(v) => v,
+						None => return None,
+					};
+				}
+				if result > (<$ulow>::max_value() as $uhi) {
+					None
+				} else {
+					Some(result as $ulow)
+				}
+			}
+		}
+	}
+}
+
+impl_rrm!(u8, u16);
+impl_rrm!(u16, u32);
+impl_rrm!(u32, u64);
+impl_rrm!(u64, u128);
+
+impl MultiplyRational for u128 {
+	fn multiply_rational(self, n: Self, d: Self, r: Rounding) -> Option<Self> {
+		crate::helpers_128bit::multiply_by_rational_with_rounding(self, n, d, r)
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::{helpers_128bit::*, *};