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

Move Fp and R1CS to manta-crypto #248

Closed
wants to merge 12 commits into from
Closed

Move Fp and R1CS to manta-crypto #248

wants to merge 12 commits into from

Conversation

GhostOfGauss
Copy link
Contributor

@GhostOfGauss GhostOfGauss commented Sep 8, 2022

Moves Fp and partially moves R1CS to manta-crypto as part of #40

This copies manta_pay::crypto::constraint::arkworks::codec into manta-crypto without removing it from manta-pay because it is still referenced by too many other parts of manta-pay. It will be removed in a future PR.

This removes scale entirely from manta-pay, its functionality is to be replaced in a way that @bhgomes can explain.

This finishes copying some R1CS impl's that weren't included in #180 but does not remove R1CS from manta-pay yet due to this being referenced by too much groth16 related code. R1CS will be removed from manta-pay together with groth16 in a future PR.


Before we can merge this PR, please make sure that all the following items have been checked off:

  • Linked to an issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Added one line describing your change in CHANGELOG.md and added the appropriate changelog label to the PR.
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Checked that changes and commits conform to the standards outlined in CONTRIBUTING.md.

@GhostOfGauss GhostOfGauss added A-cryptography Area: Issues and PRs related to Cryptography A-openzl Area: Development Related to OpenZL and ECLAIR labels Sep 8, 2022
@GhostOfGauss GhostOfGauss self-assigned this Sep 8, 2022
@GhostOfGauss GhostOfGauss marked this pull request as ready for review September 12, 2022 19:08
SupremoUGH
SupremoUGH previously approved these changes Sep 12, 2022
Copy link
Contributor

@SupremoUGH SupremoUGH left a comment

Choose a reason for hiding this comment

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

Everything seems fine, even though I don't fully understand (de)serialization, so please double check the details of that with Brandon.


/// Updates the internal reader state by performing the `f` computation.
#[inline]
fn update<T, F>(&mut self, f: F) -> Option<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we want this fn to be private?

// You should have received a copy of the GNU General Public License
// along with manta-rs. If not, see <http://www.gnu.org/licenses/>.

//! Codec Utilities
Copy link
Contributor

Choose a reason for hiding this comment

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

The stuff in this file can go in arkworks::serialize instead, we don't need to split this out.

@@ -34,6 +34,7 @@ pub use ark_ed_on_bls12_381 as ed_on_bls12_381;
pub use ark_ed_on_bn254 as ed_on_bn254;

pub mod algebra;
pub mod codec;
Copy link
Contributor

Choose a reason for hiding this comment

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

Like I mentioned before, we can use serialize here instead and do an extension of ark-serialize in that module.

@@ -17,7 +17,7 @@
//! Manta-Pay Configuration

use crate::crypto::{
constraint::arkworks::{field_element_as_bytes, groth16, Boolean, Fp, FpVar, R1CS},
constraint::arkworks::{groth16, Boolean, R1CS}, // TODO: Move R1CS with Groth16
Copy link
Contributor

Choose a reason for hiding this comment

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

fix todo item or leave it out of here (use GH issue to track this)

@@ -19,7 +19,7 @@
use crate::crypto::constraint::arkworks::{
self,
codec::{HasDeserialization, HasSerialization, SerializationError},
R1CS,
R1CS, // TODO: Move with Groth16
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above TODO

@@ -757,12 +451,13 @@ where
/// Testing Suite
#[cfg(test)]
mod tests {
// TODO: move to constraint with R1CS
Copy link
Contributor

Choose a reason for hiding this comment

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

same as other TODO

empty,
full,
Boolean,
R1CS, // TODO: Move R1CS with Groth16
Copy link
Contributor

Choose a reason for hiding this comment

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

same as other TODO

@@ -32,6 +32,7 @@ arkworks = [
"ark-r1cs-std",
"ark-relations",
"ark-serialize",
"ark-std",
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, cause we need it, lets re-export it too

@bhgomes bhgomes added the P-medium Priority: Medium label Sep 20, 2022
Signed-off-by: Brandon H. Gomes <bhgomes@pm.me>
@bhgomes
Copy link
Contributor

bhgomes commented Nov 18, 2022

Replaced by #284

@bhgomes bhgomes closed this Nov 18, 2022
@bhgomes bhgomes deleted the feat/move_fp branch November 18, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Issues and PRs related to Cryptography A-openzl Area: Development Related to OpenZL and ECLAIR P-medium Priority: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants