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.

Sign up for GitHub

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

Jump to bottom

feat: introduce codegen-verifier section to Nargo.toml #1138

Closed
wants to merge 2 commits into from
Closed

feat: introduce codegen-verifier section to Nargo.toml #1138

wants to merge 2 commits into from

Conversation

longfin
Copy link

@longfin longfin commented Apr 11, 2023 •
edited
Loading

Related issue(s)

Resolves #1009

Description

Summary of changes

Added codegen-verifier section to Nargo.toml, to enable to config the output directory for nargo codegen-verifier command.

Dependency additions / changes

N/A (maybe 😅 )

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@longfin longfin changed the title Adjust directory name for nargo codegen-verifier feat: Adjust directory name for nargo codegen-verifier Apr 11, 2023
@longfin longfin marked this pull request as ready for review April 11, 2023 19:57
@kevaundray
Copy link
Contributor

Hi,

Thank you for contributing to the Noir repository :)

The issue which you are aiming to solve has additional context here given by @TomAFrench here. Its a bit more than just a folder rename!

@longfin
Copy link
Author

longfin commented Apr 12, 2023

Hi,

Thank you for contributing to the Noir repository :)

The issue which you are aiming to solve has additional context here given by @TomAFrench here. Its a bit more than just a folder rename!

Thanks for letting me know! I misread that the configurable folder name might be a different PR. 😅 I'll be back after implementation.

@longfin longfin marked this pull request as draft April 12, 2023 14:40
@longfin longfin changed the title feat: Adjust directory name for nargo codegen-verifier feat: Introduce codegen-verifier section to Nargo.toml Apr 12, 2023
@longfin longfin marked this pull request as ready for review April 12, 2023 17:00
@longfin
Copy link
Author

longfin commented Apr 12, 2023

@kevaundray @TomAFrench PTAL 😄

TomAFrench
TomAFrench reviewed Apr 12, 2023
View reviewed changes
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.

Thanks for your help with this issue @longfin

The PR's looking pretty good, I've made a couple of small notes.

crates/nargo/src/manifest/mod.rs Outdated Show resolved Hide resolved
crates/nargo/src/manifest/mod.rs Outdated Show resolved Hide resolved
crates/nargo_cli/src/cli/codegen_verifier_cmd.rs Outdated Show resolved Hide resolved
crates/nargo_cli/src/cli/codegen_verifier_cmd.rs Outdated Show resolved Hide resolved
crates/nargo_cli/src/cli/codegen_verifier_cmd.rs Outdated Show resolved Hide resolved
@TomAFrench TomAFrench changed the title feat: Introduce codegen-verifier section to Nargo.toml feat: introduce codegen-verifier section to Nargo.toml Apr 12, 2023
@longfin longfin marked this pull request as draft April 13, 2023 06:58
@longfin longfin force-pushed the feature/1009 branch 2 times, most recently from c5ecd2d to 31577ac Compare April 13, 2023 14:26
@longfin
Copy link
Author

longfin commented Apr 13, 2023

@TomAFrench Thanks a lot for the detailed review! I guess that most of them are addressed on 31577ac.

Please take a look and feel free to let me know about remaining or other comments. 😄

@longfin longfin requested a review from TomAFrench April 13, 2023 14:32
@longfin longfin marked this pull request as ready for review April 13, 2023 14:32
@longfin
Copy link
Author

longfin commented Apr 14, 2023

I'll be back after fixing it 😓

TomAFrench
TomAFrench reviewed Apr 17, 2023
View reviewed changes
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.

Thanks @longfin, it's looking really good now. I'm going to merge this soon but I need to add an issue for the feature I mentioned so we can link to that.

crates/nargo_cli/src/cli/codegen_verifier_cmd.rs Outdated Show resolved Hide resolved
crates/nargo_cli/src/cli/codegen_verifier_cmd.rs
Comment on lines +38 to +53
/// TODO: Move this function to `PackageManifest` after tracking the root directory.
/// See also: https://github.com/noir-lang/noir/pull/1138#discussion_r1165351237
Copy link
Member

Choose a reason for hiding this comment

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

I'll make a quick issue detailing this and then we can update this to point at that issue.

@kevaundray
Copy link
Contributor

@TomAFrench barring conflicts, can we merge this?

@longfin
Copy link
Author

longfin commented Apr 27, 2023

@TomAFrench barring conflicts, can we merge this?

I'll take a look today. 💪

@longfin longfin marked this pull request as draft April 27, 2023 10:02
@longfin
Copy link
Author

longfin commented Apr 27, 2023

@kevaundray @TomAFrench In general cases, I prefer to rebase instead of merging since I don't want to leave a merge commit unnecessarily. but my branch already has been exposed and is containing @TomAFrench's commits.

In this case, is there any existing way that the team prefers between merge or rebase?

@TomAFrench
Copy link
Member

TomAFrench commented Apr 27, 2023 •
edited
Loading

Hey, sorry for the delay on this PR.

@longfin A merge commit into this branch is fine as we'll be squashing all the commits when we merge into master. Rebasing causes some issues when it comes to reviewing (we need to check the entire PR again rather than just the new commit) so we try not to do them once others have started looking at the PR.

@longfin
Copy link
Author

longfin commented Apr 27, 2023

Conflicts resolved.

@TomAFrench is there a new issue for this comment?

@longfin longfin requested a review from TomAFrench April 27, 2023 15:38
@longfin longfin marked this pull request as ready for review April 27, 2023 15:38
@longfin
Copy link
Author

longfin commented Apr 28, 2023

Oops, some tests seem to fail. I'll check again.

@longfin longfin marked this pull request as draft April 28, 2023 07:23
@TomAFrench TomAFrench mentioned this pull request May 16, 2024
Merged
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request May 17, 2024
@TomAFrench @ludamad @vezenovm
chore!: remove backend interactions from nargo (#6320)
7a31896 
This PR removes the `nargo codegen-verifier` command and replaces it
with an example bash script showing how to use `nargo` in conjunction
with `bb` to generate a smart contract verifier.

cc @AztecProtocol/devrel 



Closes noir-lang/noir#4447
Closes noir-lang/noir#1138
Closes noir-lang/noir#2222
Closes noir-lang/noir#1159
Closes noir-lang/noir#1009
Closes noir-lang/noir#3837
Closes noir-lang/noir#2667

---------

Co-authored-by: ludamad <[email protected]>
Co-authored-by: Maxim Vezenov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomAFrench TomAFrench Awaiting requested review from TomAFrench

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Place verifier contract to contracts folder instead of contract
4 participants
@longfin @kevaundray @TomAFrench @signorecello