Remove mixed type arithmetic Decimal * Uint128

[webmaster128]

There is no natural return type for Decimal * Uint128. It could be Decimal, Uint128 or even Uint512. See https://stackoverflow.com/a/44552464/2013738 for a more elaborate description of the problem.

The choice of Decimal * Uint128 = Uint128 has no reason other than its first use case. It has often confused users who expect Decimal * Uint128 = Decimal.

The better solution for the integer math is something like:

let a: Uint128;
let b: Decimal;

let c = a.multiply_ratio(b.numerator(), b.denominator());

The better solution for the decimal math is something like:

let a: Uint128;
let b: Decimal;

let c = Decimal::try_into(a)? * b;

[ethanfrey]

The better solution for the integer math is something like:
let c = a.multiply_ratio(b.numerator(), b.denomnator());

This is not a "better solution" in my mind. Very cumbersome to use and not adding clarity to devs.

If you want to make let c = a.multiply_ratio(b); that starts getting usable (if verbose).
People should not be forced to deconstruct Decimal to be able to do basic math

[webmaster128]

Yeah, this was not meant as the final API. It should just showcase how Decimal * Uint128 = Uint128 and Decimal * Uint128 = Decimal can be done and the implementation for both operation is there. It can be implemented with something like

fn multiply_by(self, rhs: impl Fraction) -> Uint128

instead to keep the caller code concise.

[ewoolsey]

Along these lines, it would be very useful to have some ceil and floor multiplication for Uints and Decimals. Something like this:

fn mul_ceiled(self, rhs: impl Fraction) -> Self;
fn mul_floored(self, rhs: impl Fraction) -> Self;

This would allow for a lot of flexibility when dealing with rounding errors etc.

[webmaster128]

@uint is 👆 something a macro implementation could help us with? I think we need those 4 functions implemented for Uint64, Uint128 and Uint256:

fn mul_ceiled(self, rhs: impl Fraction) -> Self;
fn mul_floored(self, rhs: impl Fraction) -> Self;
fn checked_mul_ceiled(self, rhs: impl Fraction) -> Result<Self, EnumSinceWeHaveTwoDifferentErrorCases>;
fn checked_mul_floored(self, rhs: impl Fraction) -> Result<Self, EnumSinceWeHaveTwoDifferentErrorCases>;

With those in place, we can confidentally remove the mixed type operator implementation in 2.0.

[uint]

@uint is point_up_2 something a macro implementation could help us with?

Probably! Like an impl_arithmetic!(u64) macro called in every Uint* module?

[webmaster128]

Yeah, or better specifically for the new additions impl_mul_arithmetic!(u64) such that we get them implemented consistently. We discussed deduplicating the Uint* implementations before and I think this is a good opportunity to try this approach. But I'm not that confident writing macros myself due to lack of experience.

Note that Uint512 should not get them since it does not have a full_mul implementation. If we add that we also need Uint1024 and the series would go on indefinitely.

[uint]

Isn't Uint128::checked_multiply_ratio already a floor multiplication? Ah, I see, we want to avoid deconstructing Decimal. Fair.

[uint]

@webmaster128 I took a shot at it yesterday. I think it's not as simple as reusing the logic in checked_multiply_ratio. E.g. if you have

let left: Uint64;
let right: Decimal; // or even Decimal256
let result = left.checked_mul_floored(right).unwrap();

I suspect the multiplication of left * right.numerator() might overflow quite easily, and full_mul doesn't help.

[webmaster128]

Right, this is something I vaguely imagined to become a problem.

So what we do currently support is basically

Uint64 * Fraction<u64>
Uint128 * Fraction<u128>
Uint256 * Fraction<Uint256>

with (more or less) Decimal being a Fraction<u128> and Decimal256 being a Fraction<Uint256>. Also by widening we can easily support

Uint128 * Fraction<u64>
Uint256 * Fraction<u64>
Uint256 * Fraction<u128>

So the number if bits in the integer needs to be >= the number of bits of the decimal. We are lacking an easy way to get

Uint64 * Fraction<u128> (i.e. Uint64 * Decimal)
Uint64 * Fraction<Uint256> (i.e. Uint64 * Decimal256)
Uint128 * Fraction<Uint256> (i.e. Uint128 * Decimal256)

However, if we do not do the multiplication size based on the integer size (current self.full_mul behaviour) but based on the larger of the two, we can get this to work. A simple implementation just takes 256*256=512 bit multiplication to cover all the cases at once.

[grod220]

Hey folks, tried my hand at this here: #1566 for Uint128.

A few things:

• It looks like the primary use case in this issue with the Uint# * Decimal use case. For our contracts, we are trying to support a Uint128 * Fraction(Uint128, Uint128) use case (Support for ceil division #1540). This means, we need a struct that implements "Fraction". I tried implementing a generic struct for this (renaming the interface to Fractional), but the inv() method is problematic. Would like some feedback on this approach, possibly another direction is better or maybe we should move inv() to another trait for Decimal only.

• ̶T̶h̶i̶s̶ ̶d̶o̶e̶s̶ ̶n̶o̶t̶ ̶s̶u̶p̶p̶o̶r̶t̶ ̶m̶u̶l̶t̶i̶p̶l̶y̶i̶n̶g̶ ̶h̶i̶g̶h̶e̶r̶ ̶b̶i̶t̶ ̶n̶u̶m̶b̶e̶r̶s̶.̶ ̶A̶n̶y̶ ̶s̶u̶g̶g̶e̶s̶t̶i̶o̶n̶s̶ ̶o̶n̶ ̶h̶o̶w̶ ̶t̶o̶ ̶h̶a̶n̶d̶l̶e̶ ̶t̶h̶i̶s̶?̶ ̶D̶o̶ ̶y̶o̶u̶ ̶s̶u̶g̶g̶e̶s̶t̶ ̶c̶a̶s̶t̶i̶n̶g̶ ̶e̶v̶e̶r̶y̶t̶h̶i̶n̶g̶ ̶t̶o̶ ̶a̶ ̶U̶i̶n̶t̶5̶1̶2̶?̶ (Macro update below)

• I made the unchecked versions simply the unwrapped of the checked. I wonder if these are necessary at all though and checked should be the default with the consumers unwrapping themselves.

[grod220]

Just pushed an update to that PR that adds a macro. If it's acceptable to cast to Uint512 for all int sizes, we simply need to wrap all of the Uint structs in fraction_math!(Uint128);. Think this is what folks were suggesting earlier in the thread.

Edit: Ah, looks like it may be better to start a generic arithmetic macro instead?

[webmaster128]

• I made the unchecked versions simply the unwrapped of the checked. I wonder if these are necessary at all though and checked should be the default with the consumers unwrapping themselves.

Checked math only makes sense if the contract actually handles the error cases. If they just ? them to top level, you can also just panic. In both cases the contract execution is terminated, the transaction is reverted and an error message is logged. When you multiply with constants like 1/3 you know the operation can never fail. So I'd implement both for consistency with other math operations.