-
Notifications
You must be signed in to change notification settings - Fork 172
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
feat!: replace U128::from_integer
with From
impls and U128::from_field
#4944
base: master
Are you sure you want to change the base?
Conversation
🚀 Deployed on https://663b84cbd7d2d92a129bb36a--noir-docs.netlify.app |
FYI @noir-lang/developerrelations on Noir doc changes. |
aa4a6f6
to
4fb65ee
Compare
This is blocked by #4944 |
noir_stdlib/src/uint128.nr
Outdated
impl From<u64> for U128 { | ||
fn from(value: u64) -> U128 { | ||
U128::from_u64s_le(value, 0) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a from_u64
in addition to from_field
? I think it'd make this easier to find for users
U128 { lo, hi } | ||
} | ||
|
||
pub fn to_integer<T>(self) -> T { | ||
crate::from_field(self.lo + self.hi * pow64) | ||
crate::from_field(self.lo + self.hi * POW_64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have plans to remove from_field
and as_field
eventually? It seems they'd be better replaced by the From
trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept from_field
and not implemented From<Field>
as this conversion can fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to have something along the lines of an AssertFrom
trait which is essentially From
but will throw a constraint error if the conversion isn't valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was just thinking of TryFrom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would TryFrom
work here? I'm thinking of AssertFrom
as we have a number of conversions where it's much easier to assert that the conversion is valid rather than checking whether it is valid or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TryFrom
would always be similar to AssertFrom
, it just gives users the option to recover from an error if possible but would be less efficient than the AssertFrom in cases where users do just want to panic on failure.
Yep, just noticed I copy-pasted incorrectly there. I'll add |
Conversion between unsigned integer types and U128 are done through the use of the `From` trait and `to_integer` method. There also exists a `from_field` method which accepts a `Field` type as input (which is asserted to fit within a `U128`). | ||
|
||
```rust | ||
fn main() { | ||
let x = U128::from_integer(23); | ||
let x = U128::from(23); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs updating to use from_u64
as the From
trait is broken
This performance regression on |
From the CI itself it looks like although there are more ACIR instructions the overall circuit size is still smaller for u128 at least. |
Description
Problem*
Resolves
Summary*
This PR replaces the overly wide generic function
U128::from_integer
withFrom
impls foru1
,u8
,u32
, andu64
. I've also addedU128::from_field
to maintain the ability to create aU128
from fields.Additional Context
Opened #4943 based off of issues when making this PR.
Documentation*
Check one:
PR Checklist*
cargo fmt
on default settings.