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 constructor is private #1155

Closed
archseer opened this issue Nov 1, 2021 · 3 comments · Fixed by #1264
Closed

Decimal constructor is private #1155

archseer opened this issue Nov 1, 2021 · 3 comments · Fixed by #1264
Assignees

Comments

@archseer
Copy link

archseer commented Nov 1, 2021

The comment on the Mul implementation says

/// Both d*u and u*d with d: Decimal and u: Uint128 returns an Uint128. There is no
/// specific reason for this decision other than the initial use cases we have. If you
/// need a Decimal result for the same calculation, use Decimal(d*u) or Decimal(u*d).

but the constructor Decimal(_) is private.

Because of this there is also no way to construct a decimal statically, let's say if I wanted to I pre-calculate some constants at compile time and I already pre-scaled the value correctly, i.e.

pub const fn decimal(i: usize, decimals: u8) -> u128 {
    let decimals = 10u128.pow(decimals as u32);
    i as u128 * decimals
}

const MY_NUM: Decimal = Decimal(decimal(10, 18));

and it's impossible to convert a raw u128 value without going through from_ratio:

let raw = some_decimal.numerator();
// none of the constants are public so we have to re-declare
const DECIMAL_FRACTIONAL: Uint128 = Uint128::new(1_000_000_000_000_000_000u128); 
Decimal::from_ratio(raw, DECIMAL_FRACTIONAL)
@harryscholes
Copy link

Decimal::from_atomics was recently added to achieve this

/// Creates a decimal from a number of atomic units and the number
/// of decimal places. The inputs will be converted internally to form
/// a decimal with 18 decimal places. So the input 123 and 2 will create
/// the decimal 1.23.
///
/// Using 18 decimal places is slightly more efficient than other values
/// as no internal conversion is necessary.
///
/// ## Examples
///
/// ```
/// # use cosmwasm_std::{Decimal, Uint128};
/// let a = Decimal::from_atomics(Uint128::new(1234), 3).unwrap();
/// assert_eq!(a.to_string(), "1.234");
///
/// let a = Decimal::from_atomics(1234u128, 0).unwrap();
/// assert_eq!(a.to_string(), "1234");
///
/// let a = Decimal::from_atomics(1u64, 18).unwrap();
/// assert_eq!(a.to_string(), "0.000000000000000001");
/// ```
pub fn from_atomics(
atomics: impl Into<Uint128>,
decimal_places: u32,
) -> Result<Self, DecimalRangeExceeded> {
let atomics = atomics.into();
const TEN: Uint128 = Uint128::new(10);
Ok(match decimal_places.cmp(&(Self::DECIMAL_PLACES as u32)) {
Ordering::Less => {
let digits = (Self::DECIMAL_PLACES as u32) - decimal_places; // No overflow because decimal_places < DECIMAL_PLACES
let factor = TEN.checked_pow(digits).unwrap(); // Safe because digits <= 17
Self(
atomics
.checked_mul(factor)
.map_err(|_| DecimalRangeExceeded)?,
)
}
Ordering::Equal => Self(atomics),
Ordering::Greater => {
let digits = decimal_places - (Self::DECIMAL_PLACES as u32); // No overflow because decimal_places > DECIMAL_PLACES
if let Ok(factor) = TEN.checked_pow(digits) {
Self(atomics.checked_div(factor).unwrap()) // Safe because factor cannot be zero
} else {
// In this case `factor` exceeds the Uint128 range.
// Any Uint128 `x` divided by `factor` with `factor > Uint128::MAX` is 0.
// Try e.g. Python3: `(2**128-1) // 2**128`
Self(Uint128::zero())
}
}
})
}

@archseer
Copy link
Author

archseer commented Nov 6, 2021

A raw constructor from u128 (maybe make it more explicit, const Decimal::raw) should still be available I think. It would partially address #1156 and it would be useful in const contexts.

@webmaster128
Copy link
Member

webmaster128 commented Jan 2, 2022

A raw constructor from u128 (maybe make it more explicit, const Decimal::raw) should still be available I think. It would partially address #1156 and it would be useful in const contexts.

Good point, added that to the list in #1186.

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

Successfully merging a pull request may close this issue.

4 participants