Execute the program to generate the witness#122
Conversation
Dzejkop
force-pushed
the
dzejkop/no-more-invoking-nargo
branch
from
9d852d6 to
d677ca4
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new compile_workspace helper to replace manual Nargo CLI invocations and extends the Noir proof API to directly read inputs and generate witnesses. Key changes include:
- Adding
compile_workspaceinnoir-toolsand integrating it across tests, CLI commands, and benchmarks. - Enhancing
NoirProofSchemewithread_witness,generate_witness, and updatingproveto accept anInputMap. - Updating workspace and dependency configurations to include the new
noir-toolscrate.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| noir-tools/src/lib.rs | Introduced compile_workspace function for program compilation. |
| noir-tools/Cargo.toml | Added noir-tools to workspace dependencies. |
| noir-r1cs/tests/compiler.rs | Replaced shell commands with compile_workspace and updated witness path. |
| noir-r1cs/src/noir_witness.rs | Added public abi() accessor and a serialization TODO. |
| noir-r1cs/src/noir_proof_scheme.rs | Added read_witness, generate_witness, updated prove signature. |
| noir-r1cs/src/cli/cmd/prove.rs | Updated CLI to use read_witness/prove API. |
| noir-r1cs/benches/bench.rs | Benchmarks now use compile_workspace and InputMap. |
| noir-r1cs/Cargo.toml | Added noir-tools dependency and new workspace crates. |
| Cargo.toml | Added noir-tools as workspace member and dependencies. |
| .github/workflows/ci.yml | Removed obsolete noirup step from CI. |
Comments suppressed due to low confidence (6)
noir-tools/src/lib.rs:10
- Add a doc comment for the public
compile_workspacefunction to describe its behavior, inputs, and expected outputs.
pub fn compile_workspace(workspace_path: impl AsRef<Path>) -> Result<Workspace> {
noir-tools/src/lib.rs:10
- Add unit tests for
compile_workspaceto verify it correctly resolves and compiles a workspace with and without theNargo.tomlsuffix.
pub fn compile_workspace(workspace_path: impl AsRef<Path>) -> Result<Workspace> {
noir-r1cs/src/noir_witness.rs:56
- Consider adding a doc comment to explain what
abi()returns and how it should be used by consumers.
pub fn abi(&self) -> &Abi {
noir-r1cs/src/noir_proof_scheme.rs:103
- Add documentation explaining the purpose of
read_witnessand the expected format of the input TOML file.
pub fn read_witness(&self, prover_toml: impl AsRef<Path>) -> Result<InputMap> {
noir-r1cs/src/noir_proof_scheme.rs:110
- Add a doc comment to describe
generate_witness, including how it handles missing inputs and error conditions.
pub fn generate_witness(&self, input_map: &InputMap) -> Result<WitnessMap<NoirFieldElement>> {
noir-r1cs/src/noir_proof_scheme.rs:110
- Add tests for
generate_witnessto verify witness generation logic under various input scenarios and edge cases.
pub fn generate_witness(&self, input_map: &InputMap) -> Result<WitnessMap<NoirFieldElement>> {
| noirc_driver = { git = "https://github.com/noir-lang/noir", rev = "nightly-2025-05-28" } | ||
| noirc_errors = { git = "https://github.com/noir-lang/noir", rev = "nightly-2025-05-28" } | ||
| fm = { git = "https://github.com/noir-lang/noir", rev = "nightly-2025-05-28" } | ||
| nargo_cli = { git = "https://github.com/noir-lang/noir", rev = "nightly-2025-05-28" } |
There was a problem hiding this comment.
I wasn't expecting the command line specific stuff to be linked in. Fine for now, but we probably want to trim this before audit.
There was a problem hiding this comment.
This is mostly for the compile_workspace funciton so we can avoid calling the external nargo binary in tests & benchmarks
| pub program: Program<NoirFieldElement>, | ||
| pub r1cs: R1CS, | ||
| pub witness_builders: Vec<WitnessBuilder>, | ||
| pub witness_generator: NoirWitnessGenerator, |
There was a problem hiding this comment.
witness_builders, witness_generator and now also a generate_witness fn entirely separate from these? This is needs to be cleaned up soon, but maybe not in this PR.
| // Note: Abi uses an [internally tagged] enum format in Serde, which is not compatible | ||
| // with some schemaless formats like Postcard. | ||
| // [internally-tagged]: https://serde.rs/enum-representations.html | ||
| // TODO: serializes the ABI as a json string. Something like CBOR might be better. |
There was a problem hiding this comment.
Agree. Now I remember it all again. This is a pretty ugly hack. Fortunately the Abi object is small so it has little impact.
There was a problem hiding this comment.
Yeah, I'm leaving the TODO - I don't think it's worth sinking time into it now
| let test_case_path = test_case_path.as_ref(); | ||
|
|
||
| run_nargo(test_case_path); | ||
| compile_workspace(test_case_path).expect("Compiling workspace"); |
There was a problem hiding this comment.
To clarify: It's of course fine to run nargo to do some stuff for end-to-end tests. And might even be preferred if it reduces dependency bloat or complexity.
There was a problem hiding this comment.
We can make the nargo cli deps dev-dependencies. This approach is a little easier to setup for tests (no external dependency) and ensures that there's no version mismatch between the nargo binary and the dependencies.
It also gives us a little more flexibility as it's e.g. possible to make tests independent by overriding the workspace compilation target directory
There was a problem hiding this comment.
Fully agree with this! Dev dependency are excluded from bloat/audit concerns.
Allows the noir-r1cs program to read the Prover.toml directly and generates the witness using the embedded ACVM program & Abi
No description provided.