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

feat(stdlib)!: Adding Mimc Sponge to stdlib #2297

Closed
wants to merge 14 commits into from

Conversation

seugu
Copy link

@seugu seugu commented Aug 12, 2023

Description

Problem*

Resolves

Summary*

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@seugu seugu changed the title Mimc sponge feat: Adding Mimc Sponge to stdlib Aug 12, 2023
@kevaundray
Copy link
Contributor

Hi,

Thank you for your contribution!

Can you update the description to accurately describe the changes being made in this PR, ie why files were moved, why we need the sponge variation etc?

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

This is currently a breaking change. Can you please undo moving the existing mimc functions?

@kevaundray
Copy link
Contributor

This is currently a breaking change. Can you please undo moving the existing mimc functions?

Perhaps its better to just change the PR title because it makes sense to move it to its own file

@vezenovm vezenovm changed the title feat: Adding Mimc Sponge to stdlib feat(stdlib)!: Adding Mimc Sponge to stdlib Aug 15, 2023
@vezenovm
Copy link
Contributor

vezenovm commented Aug 15, 2023

@TomAFrench I changed the title to a breaking change as I do think moving the mimc functions to their own file makes sense. Especially when the impls require long lists of constants

Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

@seugu you need to update the import in a couple tests that you can see failing in the CI.

Could you also add a test for MiMCSponge that checks the output of some simple outputs. There are similar tests for hash functions like sha256 under crates/nargo_cli/tests/execution_success which you can follow. The test will just be a simple Noir program.

noir_stdlib/src/hash/mimc.nr Outdated Show resolved Hide resolved
noir_stdlib/src/hash/mimc.nr Outdated Show resolved Hide resolved
noir_stdlib/src/hash/mimc.nr Outdated Show resolved Hide resolved
noir_stdlib/src/hash/mimc.nr Outdated Show resolved Hide resolved
@vezenovm vezenovm requested a review from TomAFrench September 5, 2023 19:32
@TomAFrench
Copy link
Member

I can't really validate the correctness of the implementation so I'll approve if someone on the team vouches for it.

That said, I think this goes against our aims in #1258. This would be best as a mimc library.

@vezenovm
Copy link
Contributor

vezenovm commented Sep 5, 2023

That said, I think this goes against our aims in #1258. This would be best as a mimc library.

I recommended for this to be put in the standard lib as MiMC is common hash function that should not be changing much and we already have MiMC7 inside of the stdlib. I forgot about the issue and aims listed in #1258, but perhaps this can just be moved out once we have some type of hashes library under noir-lang. If we feel strongly this should remain outside of Noir, we can ask @seugu to turn his Noir-MiMCSponge repo into a library.

The implementation is correct from my review and checks against the circom implementations.

@vezenovm
Copy link
Contributor

vezenovm commented Sep 5, 2023

@seugu It looks like you are using an old version of nargo for the new tests you included under execution_success. The projects are missing the name and type fields in the Nargo.toml.

@TomAFrench
Copy link
Member

I'm closing this PR as the mimc implementation has been moved to an external library: https://github.com/noir-lang/mimc

@TomAFrench TomAFrench closed this Sep 26, 2024
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.

4 participants