Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decimal * Uint128 treats Uint128 as a raw Decimal value #1156

Closed
archseer opened this issue Nov 2, 2021 · 6 comments
Closed

Decimal * Uint128 treats Uint128 as a raw Decimal value #1156

archseer opened this issue Nov 2, 2021 · 6 comments

Comments

@archseer
Copy link

archseer commented Nov 2, 2021

Seems to be a "feature" so raw Uint128 values can be treated as decimals.

// NOTE: using a custom Decimal fork here since the constructor is normally private
   unimplemented!(
        "{} {} {} {}",
        gas_cost * Decimal::one(), // decimal * decimal
        gas_cost * Uint128::from(1u128), // decimal * u128
        gas_cost * Decimal::from_ratio(Uint128::from(1u128), 1u128), // decimal * decimal(u128 / 1)
        gas_cost * Decimal(Uint128::from(1u128)), // decimal * decimal(u128)
    );
//1 'not implemented: 0.00000000000338 0 0.00000000000338 0'

This is very surprising, since the Div implementation doesn't follow this behavior, and Mul doesn't this either.

Similar to #1155 I propose the Decimal() constructor to be made public. The Mul implementation should be fixed, then the old behavior can be more explicit via decimal_val * Decimal(uint_val).

@webmaster128
Copy link
Member

Thanks for brining this up. I'm really unsure what the desired output type is.

There is also no generic and safe conversion

  • Decimal -> Uint128: because information after the decimal point get lost
  • Uint128 -> Decimal: because the range of Decimal is smaller since 18 digits are reserved for the data behind the decimal point

This makes it hard to say: Use this output type that can easily be converted.

@webmaster128
Copy link
Member

Looking at https://stackoverflow.com/a/44552464/2013738, it feels like this should not be implemented at all.

@archseer
Copy link
Author

archseer commented Feb 4, 2022

This should be implemented. I'd expect decimal * uint128 to do a simple multiplication. Equivalent to decimal(decimal.0 * uint128) or decimal * decimal::from_ratio(uint128, 1) but more gas efficient than constructing a int/1 fraction then multiplying two fractions together.

@archseer
Copy link
Author

archseer commented Feb 4, 2022

i.e. I have $0.05 dollars (internally, stored as 5 cents, the lowest possible fraction, with a DECIMALS value of 2). I double my money (2 * 5) => $0.10

@ethanfrey
Copy link
Member

Basically, there are two ways we could want.

200 * 50% = 100 (multiply my number by some decimal) - this is implemented as majority case
25% * 3 = 75% (multiply my percentage by a number to get a bigger percentage) - this is not typical, but should be possible.

Maybe this is included in #1186 ?

But if we have Decimal * Decimal -> Decimal that should be plenty. And you can convert your integer to a decimal with no loss before multiplying. Making it possible, even if not super simple syntax.

@webmaster128
Copy link
Member

The mixed type arithmetic Decimal * Uint128 should be removed when we get the chance. Created a ticket for 2.0: #1485

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants