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

Support fraction multiply floor/ceil #1566

Merged
merged 13 commits into from
Jan 23, 2023

Conversation

grod220
Copy link
Contributor

@grod220 grod220 commented Jan 3, 2023

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

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint128.rs Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably avoid the need for the second macro argument

packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
@grod220
Copy link
Contributor Author

grod220 commented Jan 4, 2023

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
Copy link
Member

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 grod220 marked this pull request as ready for review January 5, 2023 08:26
packages/std/src/math/fraction.rs Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
packages/std/src/math/fraction.rs Outdated Show resolved Hide resolved
@grod220
Copy link
Contributor Author

grod220 commented Jan 8, 2023

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
Copy link
Member

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
Copy link
Member

Any thoughts on extending this to also support dividing?

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

@grod220
Copy link
Contributor Author

grod220 commented Jan 12, 2023

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
Copy link
Member

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
Copy link
Contributor Author

grod220 commented Jan 17, 2023

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.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

packages/std/src/math/fraction.rs Show resolved Hide resolved
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎖

@grod220
Copy link
Contributor Author

grod220 commented Jan 18, 2023

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

@grod220 grod220 reopened this Jan 18, 2023
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 this pull request may close these issues.

2 participants