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

feature: ark version differentiation #241

Merged
merged 8 commits into from
Jul 24, 2023
Merged

feature: ark version differentiation #241

merged 8 commits into from
Jul 24, 2023

Conversation

prestwich
Copy link
Collaborator

@prestwich prestwich commented May 12, 2023

Motivation

closes #240

Solution

  • Move ark-ff@0.4 functionality to ark_ff_04.rs.
  • Restore 0.3 to its original home in ark_ff.rs
  • Explicit feature flag for ark-ff
  • Explicit feature flag for ark-ff-04

Considerations:

To avoid a backwards-incompatible interface change, the newer ark functionality has the qualified feature-name ark-ff-04. This preserves the current released behavior of --feature ark-ff, but users may not expect new functionality to be behind the more-involved feature flag

Don't love the name ark-ff-04

This PR makes the ark changes semver minor. Previous changes were semver major

PR Checklist

  • [n/a] Added Tests
  • Added Documentation
  • Updated the changelog

@prestwich prestwich added the enhancement New feature or request label May 12, 2023
@prestwich prestwich requested a review from recmo May 12, 2023 00:02
@prestwich prestwich self-assigned this May 12, 2023
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Patch coverage: 70.73% and project coverage change: -0.06 ⚠️

Comparison is base (223c97e) 83.56% compared to head (fb59103) 83.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
- Coverage   83.56%   83.51%   -0.06%     
==========================================
  Files          51       52       +1     
  Lines        5483     5538      +55     
==========================================
+ Hits         4582     4625      +43     
- Misses        901      913      +12     
Impacted Files Coverage Δ
src/support/ark_ff.rs 78.18% <53.84%> (-0.39%) ⬇️
src/support/ark_ff_04.rs 78.57% <78.57%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@prestwich prestwich enabled auto-merge May 30, 2023 17:13
@prestwich prestwich disabled auto-merge July 17, 2023 16:14
@gakonst
Copy link
Collaborator

gakonst commented Jul 21, 2023

Don't love the name ark-ff-04

Me neither - I chatted w/ Remco and he was also OK with it.

This PR makes the ark changes semver minor. Previous changes were semver major

This changes the feature flag name, does that warrant semver major maybe? Remco said ideally we could do this for all support features, i.e. rename them all to be pinned to their version (maybe unnecessary tbh) and then do a major release. Is that a big deal?

Copy link
Collaborator

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Good w/ me.

@@ -47,7 +47,8 @@ thiserror = "1.0"
# support
alloy-rlp = { version = "0.3", optional = true }
arbitrary = { version = "1", optional = true }
ark-ff = { version = "0.4", optional = true }
ark-ff-04 = { version = "0.4.0", package = "ark-ff", optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

gg

Comment on lines +21 to +27
macro_rules! impl_from_ark {
($ark:ty, $bits:expr, $limbs:expr) => {
impl From<$ark> for Uint<$bits, $limbs> {
fn from(value: $ark) -> Self {
Self::from_limbs(value.0)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@DaniPopes DaniPopes mentioned this pull request Jul 22, 2023
3 tasks
@prestwich prestwich merged commit fdf65e6 into main Jul 24, 2023
@prestwich prestwich deleted the prestwich/ark branch July 24, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for ark-ff@0.3 and ark-ff@0.4
2 participants