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

Uint256 implementation #1059

Merged
merged 14 commits into from
Aug 30, 2021
Merged

Uint256 implementation #1059

merged 14 commits into from
Aug 30, 2021

Conversation

uint
Copy link
Contributor

@uint uint commented Aug 27, 2021

Also includes Isqrt implementation for Uint64, Uint128 and Uint256.

Closes #1053
Closes #1054

@uint uint requested review from webmaster128 and maurolacy August 27, 2021 20:30
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.

Nice!

Now with this we can make Uint128::full_mul public and return an Uint256.

packages/std/src/math/isqrt.rs Show resolved Hide resolved
packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint256.rs Show resolved Hide resolved
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

lgtm.

packages/std/src/math/isqrt.rs Show resolved Hide resolved
@uint uint requested a review from webmaster128 August 30, 2021 11:27
@uint
Copy link
Contributor Author

uint commented Aug 30, 2021

All the commends have been addressed!

@uint uint requested a review from webmaster128 August 30, 2021 12:02
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.

💪

}

/// Multiplies two u128 values without overflow.
fn full_mul(self, rhs: impl Into<u128>) -> U256 {
U256::from(self.u128()) * U256::from(rhs.into())
pub fn full_mul(self, rhs: impl Into<u128>) -> Uint256 {
Copy link
Member

Choose a reason for hiding this comment

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

This new API is nice. Could you add a CHANGELOG entry and an example doc for it?

@@ -4,6 +4,9 @@ mod system_error;
mod verification_error;

pub use recover_pubkey_error::RecoverPubkeyError;
pub use std_error::{DivideByZeroError, OverflowError, OverflowOperation, StdError, StdResult};
pub use std_error::{
ConversionOverflowError, DivideByZeroError, OverflowError, OverflowOperation, StdError,
Copy link
Member

Choose a reason for hiding this comment

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

ConversionOverflowError also need to be exported in lib.rs because it is used in StdError now.

panic!(
"right shift error: {} is larger than the number of bits in Uint256",
"right shift error: {} is larger or equal than the number of bits in Uint256",
Copy link
Contributor

@maurolacy maurolacy Aug 30, 2021

Choose a reason for hiding this comment

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

👍🏼 This is being handled in the other impls by delegating to the checked variant and unwrapping. Nice to have consistent behaviour here.

What about a checked_shr, by the way? I guess it's not currently needed / used. But wouldn't be bad to have; particularly because it doesn't panic.

@uint uint requested a review from webmaster128 August 30, 2021 14:50
@webmaster128 webmaster128 merged commit d032dfc into main Aug 30, 2021
@webmaster128 webmaster128 deleted the isqrt-for-uint128-uint256 branch August 30, 2021 15:35
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.

Implement trait Isqrt for Uint128 and Uint256 Add Uint256
3 participants