Support fraction multiply floor/ceil

[grod220]

PR to implement thoughts from: #1485 (Uint * Fraction arithmetic)

[webmaster128]

💪

[webmaster128]

You can probably avoid the need for the second macro argument

[grod220]

At the moment, there are two blockers for implementing this macro for Uint512's:

1 - It does not implement .full_mul()
2 - In packages/std/src/math/fraction.rs line 42, the Ok(res.try_into()?) causes an error:

the trait `From<Infallible>` is not implemented for `CheckedMultiplyFractionError`

Any thoughts on the Uint512 case?

[webmaster128]

Any thoughts on the Uint512 case?

Yes, sorry I forgot to mention explicitely. Forget about that one. The only purpose for Uint512 is being the full_mul result of Uint256. If we don't stop there, the chain goes on endlessly. The implementations that matter are Uint64. Uint128 and Uint256.

[grod220]

Any thoughts on extending this to also support dividing?

pub fn div_floor<F: Fraction<T>, T: Into<$Uint>>(self, rhs: F) -> Self
pub fn div_ceil<F: Fraction<T>, T: Into<$Uint>>(self, rhs: F) -> Self

The implementation would simply be self.checked_mul_floor(rhs.inv()). Well actually, .inv() returns an optional, so that would need an ..ok_or(). Side note: I think it makes more sense for .inv() to return a result rather an optional.

[webmaster128]

Side note: I think it makes more sense for .inv() to return a result rather an optional.

Why do you think so? None here represents the only possible error case (div by zero). This is what the Rust standard library math does as well. However, in our case we often use Results as well because there are multiple error cases. It's a bit inconsistent right now.

[webmaster128]

Any thoughts on extending this to also support dividing?

Sounds straight forward. This has not been requested yet but why not.

[grod220]

Dividing support added. Turns out we cannot just do self.checked_mul_floor(rhs.inv()) as .inv() can't be trusted for Decimals (https://twitter.com/grod220/status/1613168904452603906).

Why do you think so? None here represents the only possible error case (div by zero). This is what the Rust standard library math does as well. However, in our case we often use Results as well because there are multiple error cases. It's a bit inconsistent right now.

I'd say that's treating an Option as if it were a Result. The Result struct is quite good and has built in support for the kinds of handling you'd need to do if it were to raise. None should not represent an error in general, Err should. If I were using this in a contract, I'd likely do a .ok_or(Err... on that option.

On tests:
In general, I'm not really a fan of co-locating the tests with the implementation. If tested well, the tests will be like 70% of the lines and make it harder to navigate the implementation. It also makes it harder to find the tests that are relevant to the feature you are testing. Ideally these tests would live in: packages/std/tests/math/uint128/test_fraction.rs

Also, it is bad that mul_floor/mul_ceil/div_floor/div_ceil is reserved for fraction math? Could this pose a problem later? Should it be instead something like frac_mul_floor?

[webmaster128]

Dividing support added. Turns out we cannot just do self.checked_mul_floor(rhs.inv()) as .inv() can't be trusted for Decimals (https://twitter.com/grod220/status/1613168904452603906).

Good point

I'd say that's treating an Option as if it were a Result. The Result struct is quite good and has built in support for the kinds of handling you'd need to do if it were to raise. None should not represent an error in general, Err should.

I like that take. It's a good way to do things differently than what the standard library does. It also comes with the following advantages:

• Custom errors can implement From for StdError converters, which we cannot achieve for None
• We get consistency between APIs with 1 possible error case and multiple cases

If tested well, the tests will be like 70% of the lines and make it harder to navigate the implementation. It also makes it harder to find the tests that are relevant to the feature you are testing.

Yeah, right. I got used to it. I think a big advantage of having them in the same file is being able to test private function. I'd keep it that way for now.

Also, it is bad that mul_floor/mul_ceil/div_floor/div_ceil is reserved for fraction math? Could this pose a problem later? Should it be instead something like frac_mul_floor?

I think this is fine. All same type arithmetic can go into operator overloading. And if we really need something else later, we can reconsider.

[grod220]

Think we are feature complete with this PR. Let me know if there's anything else or if we need a second reviewer or can merge.

[webmaster128]

Great stuff, thanks a lot. Great test coverage ❤️

What I see missing is:

• A basic rust doc for each new function in the macro
• A changelog entry to the Unreleased section
• The implementation suggestion mentioned inline

If we can finalize this PR this week, it can be shipped as part of CosmWasm 1.2. I'll prioritize it accordingly.

[webmaster128]

🎖

[grod220]

Oh shoot, tried to sync the fork to fix the merge conflict. Will fix now.