Substrate: Review occurences of `UniqueSaturated[From, Into]` trait usage for potential lossy casts

[tomusdrw]

Consider replacing with Into/From, perhaps As can be removed completely.

[shawntabrizi]

I am un-assigning myself if anyone else wants to grab this issue. Otherwise, once I finish other stuff on my plate, I will look to pick this up again.

[thiolliere]

timestamp module: we use as to convert between T::Moment and InherentType (=u64)
currenctly if T::Moment is subset of u64 then it is OK, if it is overset like u128 then we can provide a Inherent which will be validated but actually in the far future.

two solutions:

• make InherentType as a Vec and encode/decode it into Moment and make all operation using T::Moment type
• Bound T::Moment something like Into (which is and must be implemented for subset of u64 only) to convert the moment to u64 is a safe way.

[thiolliere]

Perbill, Permill, Perquitill: the mul operation is error prone to me: as when multiplying N to Perbill(x) we do: convert N to u64, multiply to x and divide by bill.
This was actually introduced by me in https://github.com/paritytech/substrate/pull/1610/files

I propose to implement Mul like it was in the old way and and CheckedMul with a saturating_mul and and bound to CheckedMul.

[thiolliere]

I mean we can easily do that like this, and even implement some saturating things...

iff --git a/core/sr-primitives/src/lib.rs b/core/sr-primitives/src/lib.rs
index efaf4146f..cc6755953 100644
--- a/core/sr-primitives/src/lib.rs
+++ b/core/sr-primitives/src/lib.rs
@@ -139,11 +139,11 @@ impl Permill {

 impl<N> ::rstd::ops::Mul<N> for Permill
 where
-       N: traits::As<u64>
+       N: traits::As<u64> + ::rstd::ops::Mul<N, Output=N> + ::rstd::ops::Div<N, Output=N>
 {
        type Output = N;
        fn mul(self, b: N) -> Self::Output {
-               <N as traits::As<u64>>::sa(b.as_().saturating_mul(self.0 as u64) / 1_000_000)
+               b * <N as traits::As<u64>>::sa(self.0 as u64) / <N as traits::As<u64>>::sa(1_000_000)
        }
 }

@@ -213,11 +213,11 @@ impl Perbill {

 impl<N> ::rstd::ops::Mul<N> for Perbill
 where
-       N: traits::As<u64>
+       N: traits::As<u64> + ::rstd::ops::Mul<N, Output=N> + ::rstd::ops::Div<N, Output=N>
 {
        type Output = N;
        fn mul(self, b: N) -> Self::Output {
-               <N as traits::As<u64>>::sa(b.as_().saturating_mul(self.0 as u64) / 1_000_000_000)
+               b * <N as traits::As<u64>>::sa(self.0 as u64) / <N as traits::As<u64>>::sa(1_000_000_000)
        }
 }

@@ -293,11 +293,11 @@ impl ::rstd::ops::Deref for Perquintill {

 impl<N> ::rstd::ops::Mul<N> for Perquintill
 where
-       N: traits::As<u64>
+       N: traits::As<u64> + ::rstd::ops::Mul<N, Output=N> + ::rstd::ops::Div<N, Output=N>
 {
        type Output = N;
        fn mul(self, b: N) -> Self::Output {
-               <N as traits::As<u64>>::sa(b.as_().saturating_mul(self.0) / QUINTILLION)
+               b * <N as traits::As<u64>>::sa(self.0 as u64) / <N as traits::As<u64>>::sa(QUINTILLION)
        }
 }

[kianenigma]

I also have a dilemma in which the Balance type being inherently interchangeable As and From u64 matters. The operations I need must be safe, not saturating but overflowing combination of multiplications and additions between Balance and Perquintill, both of which can be u64 at the moment.

Regardless of how that will be solved, I see it very likely that I will depend on the Balance being a u64. Hence, if this is subject to change, perhaps it is good to decide on this first rather than creating more artifacts that actually depend on Balance <-> u64.

[gavofyork]

Balance is u128 in polkadot and node (not u64), but yeah it's generic so it could be anything.

[xlc]

I don't understand how Balance: As<u64> + As<usize> is safe. That means the maximum numeric type for Balance have to be the smaller value of u64 or usize, otherwise the conversion will trunk the value and cause issue. But it is defined as u128 in substrate node.

I can see sa is used to convert usize to Balance. In that case wouldn't From<usize> better? That also means the Balance type should be at least as large as usize to avoid lossy casting.

[tomusdrw]

@xlc safety depends on the way it's used, that why the issue says: review the occurencies, check if they are safe and covert the type bounds to ensure that they are always safe. Cause currently As bound implies a 1-1 conversion which obviously is incorrect in some cases.

[xlc]

I just tried to use From<usize> and it doesn't work. So I kind of get now why As trait exists.
But still I think it better to separate As into two traits similar to From and Into.
Then Balance: AsFrom<u64> + AsFrom<usize> clearly means Balance needs be at least as large as u64 and usize. There is no confusion that Balance may need to be able to convert into u64 because it is unsafe and shouldn't be supported at type level.

[thiolliere]

TryFrom is stable now in rust 1.34

TryFrom allow to try to cast from larger types to smaller ones:
impl TryFrom<u128> for u8

Thus we could be able to move from As to From From, TryInto, it seems

[kianenigma]

+1 This is a pretty damn good update.

[thiolliere]

From what as been discussed in #2322 and # #2205 and on riot channel, it seems the direction is to remove As<u64> in simple arithmetic and in Currency.

I still don't know

• simple arithmetic should implement TryFrom<u64> ? I don't understand the meaning of it is that it can be convert exept if it overflow the type capacity (u8 will return Ok for integer < 256 ?)
• Currency should implement From<u64> ? maybe it make sense to enforce that a currency can contains at least one()+one()+one ... for u64::max ? probably not.

And then convert from currency or simplearithmetic to u64 should be made with a Convert associated type in modules that requires it ?

[thiolliere]

One of my concern about this refactor was how can modules extend Balance type bounded traits. If a moodule wants the balance to implement a specific trait it can do in two manners:

let's module want balance to have As<u8> (well bad example as module should get rid of As but it could be anything else)

1 - Adding an associated type

Module add an associated type Balance which bound everything that currency::balance requires + what user want more.
and probably also NegativeImbalance and PositiveImbalance.
And then bound Currency this way: Currency<AccountId, Balance=Self::Balance, PositiveImbalance=Self::PositiveImbalance, NegativeImbalance=Self::NegativeImabalance>
This should work, it is not super handy though, and when implementing module trait in node/runtime those associated type will need to be added.

2 - Declaring an extended trait (better I think)

diff --git a/srml/treasury/src/lib.rs b/srml/treasury/src/lib.rs
index 39253ac4d..1c5989265 100644
--- a/srml/treasury/src/lib.rs
+++ b/srml/treasury/src/lib.rs
@@ -65,18 +65,30 @@ use rstd::prelude::*;
 use srml_support::{StorageValue, StorageMap, decl_module, decl_storage, decl_event, ensure};
 use srml_support::traits::{Currency, ReservableCurrency, OnDilution, OnUnbalanced, Imbalance};
 use runtime_primitives::{Permill,
-       traits::{Zero, EnsureOrigin, StaticLookup, Saturating, CheckedSub, CheckedMul}
+       traits::{Zero, EnsureOrigin, StaticLookup, Saturating, CheckedSub, CheckedMul, As}
 };
 use parity_codec::{Encode, Decode};
 use system::ensure_signed;

+pub trait Extended<AccountId>: Currency<AccountId> {
+       type ExtendedBalance: From<Self::Balance> + As<u8>;
+}
+
+impl<AccountId, T> Extended<AccountId> for T
+where
+       T: Currency<AccountId>,
+       <T as Currency<AccountId>>::Balance: As<u8>,
+{
+       type ExtendedBalance = <T as Currency<AccountId>>::Balance;
+}
+
 type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as system::Trait>::AccountId>>::Balance;
 type PositiveImbalanceOf<T> = <<T as Trait>::Currency as Currency<<T as system::Trait>::AccountId>>::PositiveImbalance;
 type NegativeImbalanceOf<T> = <<T as Trait>::Currency as Currency<<T as system::Trait>::AccountId>>::NegativeImbalance;

 pub trait Trait: system::Trait {
        /// The staking balance.
-       type Currency: Currency<Self::AccountId> + ReservableCurrency<Self::AccountId>;
+       type Currency: Extended<Self::AccountId> + ReservableCurrency<Self::AccountId>;

        /// Origin from which approvals must come.
        type ApproveOrigin: EnsureOrigin<Self::Origin>;

[tomusdrw]

The close criteria of this issue are not really clear, but I'd be up for closing it, since I believe it has been addressed already and the situation is much better (As is no longer used afair).

[kianenigma]

I think it should be follow up of https://github.com/paritytech/substrate/pull/4281.

We generally fallback to saturating to avoid overflow panic. But one should make sure that the loss of saturation is sensible and there is no other way around id + it is clearly documented, if it is part of a public API.

cc @DemiMarie-parity

[Demi-Marie]

@kianenigma This does fall under #4281.