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

impl Div<Decimal256> for Uint256 #1822

Closed
0xForerunner opened this issue Aug 18, 2023 · 6 comments
Closed

impl Div<Decimal256> for Uint256 #1822

0xForerunner opened this issue Aug 18, 2023 · 6 comments

Comments

@0xForerunner
Copy link

0xForerunner commented Aug 18, 2023

Currently this is not implemented, my guess as to why being that the Output type is not obvious.
The most obvious workaround being:

let res = my_uint * my_dec.inv();

This comes with the drawback that if my_dec is very large you begin to lose significant precision with .inv(), to the point that it could become 0. Another possible solution is perhaps:

let decimal_res = Decimal256::new(my_uint * DECIMAL_FRACTION) / my_dec;
// and if you want a Uint256
let uint_res = decimal_res.atomics() / DECIMAL_FRACTIONAL

My question is then, Do you plan to implement this? and if not, is this method the best way to go about it?

@0xForerunner
Copy link
Author

This also raises the question about why we don't have

impl From<Decimal256> for Uint256 // should floor by default

and

impl TryFrom<Uint256> for Decimal256

I imagine these would be very useful to many.

@0xForerunner
Copy link
Author

0xForerunner commented Aug 18, 2023

After playing around I suppose this is probably the cleanest solution:

pub fn div(numerator: Uint256, denominator: Decimal256) -> Uint256 {
    numerator * Decimal256::one().atomics() / denominator.atomics()
}

and perhaps to prevent overflow during the first mul then this would be the most robust version:

pub fn checked_div(numerator: Uint256, denominator: Decimal256) -> StdResult<Uint256> {
    Ok(numerator.full_mul(Decimal256::one().atomics()).checked_div(Uint512::from(denominator.atomics()))?.try_into()?)
}

@webmaster128
Copy link
Member

Mixed type arithmetic is discouraged and should not be implemented. I was a mistake to implement it in the first place. As a consequence, let res = my_uint * my_dec.inv(); (uint times decimal) will not be possible from 2.0 onwards. The better way is to do a clean conversion and then operate on decimals or integers, depending on the situation. Some of those conversions between ints and decimals are probably missing and we should add them.

However, #1566 added things like checked_div_ceil which you can use on the integer.

@0xForerunner
Copy link
Author

Mixed type arithmetic is discouraged and should not be implemented. I was a mistake to implement it in the first place. As a consequence, let res = my_uint * my_dec.inv(); (uint times decimal) will not be possible from 2.0 onwards. The better way is to do a clean conversion and then operate on decimals or integers, depending on the situation. Some of those conversions between ints and decimals are probably missing and we should add them.

However, #1566 added things like checked_div_ceil which you can use on the integer.

Ah yeah I suppose that makes sense. Adding those conversions would definitely be really helpful I think. I've personally found it a bit cumbersome! Thanks for the reply though. Feel free to close this issue, or change the title to implementing those conversions!

@webmaster128
Copy link
Member

Here you find an overview of the conversions between unsigned math types. Hope this helps. Is there anything missing that you need?

Untitled Diagram

@webmaster128
Copy link
Member

With #1825 being shipped in 1.4, I think we are good here. Closing for now.

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

2 participants