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

Updated ff crate to 0.13 #1

Merged
merged 6 commits into from
May 9, 2024
Merged

Updated ff crate to 0.13 #1

merged 6 commits into from
May 9, 2024

Conversation

TrevorGKann
Copy link
Contributor

ff 0.13 adds nice functionality like from_u128 to field element to avoid having to convert ints to strings and then field elements.

All tests are passing locally.

Copy link
Owner

@kwantam kwantam left a comment

Choose a reason for hiding this comment

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

See note below about semver. I think this has to be a new minor version number...

Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "fffft"
version = "0.4.1"
version = "0.4.2"
Copy link
Owner

Choose a reason for hiding this comment

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

I know I messed this up in the past, but I think this should be 0.5.0 rather than 0.4.2 because bumping the ff version breaks semver compatibility with other crates.

(For example, wouldn't this bump break lcpc-ligero-pc?)

README.md Outdated
@@ -25,6 +25,8 @@ This crate contains a blanket trait impl for [ff::PrimeField].

- 0.4.1: Update deps.

- 0.4.2: Update `ff` to 0.13, no API changes.
Copy link
Owner

Choose a reason for hiding this comment

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

If it's 0.5.0 instead, need to change this, too.

src/lib.rs Outdated
@@ -303,7 +303,7 @@ fn roots_of_unity<T: Field>(mut root: T, log_len: u32, rdeg: u32) -> Vec<T> {

// early exit for short inputs
if log_len - 1 <= LOG_PAR_LIMIT {
return iterate(T::one(), |&v| v * root)
return iterate(T::ONE.clone(), |&v| v * root)
Copy link
Owner

Choose a reason for hiding this comment

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

Field is Copy, so shouldn't the clone() be unnecessary?

src/lib.rs Outdated
@@ -325,7 +325,7 @@ fn rou_rec<T: Field>(out: &mut [T], log_roots: &[T]) {

// base case: just compute the roots sequentially
if log_roots.len() <= LOG_PAR_LIMIT as usize {
out[0] = T::one();
out[0] = T::ONE.clone();
Copy link
Owner

Choose a reason for hiding this comment

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

ditto re: clone

src/lib.rs Outdated
@@ -358,7 +358,7 @@ fn roots_of_unity_ser<T: Field>(mut root: T, log_len: u32, rdeg: u32) -> Vec<T>
root *= root;
}

iterate(T::one(), |&v| v * root)
iterate(T::ONE.clone(), |&v| v * root)
Copy link
Owner

Choose a reason for hiding this comment

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

ditto re: clone

@kwantam
Copy link
Owner

kwantam commented Oct 17, 2023

By the way, once this is merged we can update the version of fffft in https://github.com/conroi/lcpc/tree/rsw/v0.2-prep

@TrevorGKann
Copy link
Contributor Author

My fork also has a branch with the update semver for the new 0.5.0 (previously 0.4.1).

New changes bring this up to 0.6.0 now with the new ff versioning.

@kwantam
Copy link
Owner

kwantam commented Oct 25, 2023

Since version 0.5.0 doesn't exist yet, why not just fold both updates into the same update? I don't think there's any need to bump the version twice in the same PR! :)

Copy link
Owner

@kwantam kwantam left a comment

Choose a reason for hiding this comment

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

👍

@kwantam kwantam merged commit be8702c into kwantam:main May 9, 2024
1 check passed
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