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

Fix and update dependencies to 0.3 #88

Open
wants to merge 4 commits into
base: constraints
Choose a base branch
from

Conversation

vlopes11
Copy link
Member

@vlopes11 vlopes11 commented Mar 23, 2022

Description

The branch constraints isn't building with latest dependencies.

Open questions

  • We declare DensePolynomial<F> concretely in some places, and expect the generic PC::BatchProof in others
  • fiat_shamir::AlgebraicSponge looks duplicate to ark_sponge::CryptographicSponge and should possibly be replaced

closes: #87


  • Targeted PR against correct branch (master) - N/A, this aims to fix constraints so the latter can be merged to master
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@vlopes11 vlopes11 changed the base branch from master to constraints March 23, 2022 15:27
@Pratyush
Copy link
Member

Pratyush commented Mar 23, 2022

We should indeed deprecate fiat_shamir::AlgebraicSponge entirely

src/lib.rs Outdated
Comment on lines 10 to 15
//#![deny(unused_import_braces, unused_qualifications, trivial_casts)]
//#![deny(trivial_numeric_casts, private_in_public)]
//#![deny(stable_features, unreachable_pub, non_shorthand_field_patterns)]
//#![deny(unused_attributes, unused_imports, unused_mut)]
//#![deny(renamed_and_removed_lints, stable_features, unused_allocation)]
//#![deny(unused_comparisons, bare_trait_objects, unused_must_use, const_err)]
Copy link
Member

Choose a reason for hiding this comment

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

We should change these to warns, like the main branch has

Copy link
Member Author

Choose a reason for hiding this comment

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

Ops, forgot these

Copy link
Member Author

Choose a reason for hiding this comment

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

@weikengchen
Copy link
Member

ideas to fix the test?

@vlopes11 vlopes11 requested a review from deevashwer May 20, 2022 16:18
/// The interface for a cryptographic sponge constraints on field `F`.
/// A sponge can `absorb` or take in inputs and later `squeeze` or output bytes or field elements.
/// The outputs are dependent on previous `absorb` and `squeeze` calls.
pub trait CryptographicSpongeVarNonNative<F: PrimeField, CF: PrimeField, S: CryptographicSponge>:

Choose a reason for hiding this comment

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

It does not seem this trait has been implemented in this PR.

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.

Fix and update dependencies to 0.3
4 participants