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!: Rename noir-execute to noir-executor #7605

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Mar 6, 2025

Description

Problem*

I want to add more commands related to the execution of artefacts without increasing the number of binaries.

Summary*

Renames noir-execute <args> to noir-executor execute <args>, so that we can add new commands related to artifact execution in the future. noir-execute is not installed yet with noirup so this shouldn't break anything at the moment.

Additional Context

AztecProtocol/aztec-packages#12148 shows what a "dump" from TypeScript will look like. Notably the inputs to the circuit are encoded similar to how the ACVM CLI takes them: as a witness map, as opposed to the human readable ABI encoded data structure we use in Prover.toml with nargo execute and noir-execute.

We can have another command to deal with this input format as it is, e.g. replay, which we could invoke as noir-replay, but I also thought we might want to take the witness map from the recording and write it into a Prover.toml file which we could use in integration tests.

This is something I did during debugging by logging the TypeScript DTOs to the console in JSON format, however that is not easy to capture in the dump due to where it happens: it's just a transient value in a conversion function.

We could call such a command e.g. noir-abi decode for example, with a potential noir-abi encode to do the opposite. So that's already two more executables to install along noir-execute, noir-profiler, noir-inspector, etc.

Originally I proposed to have a noir-artifact command to bundle artifact related commands. This PR is an alternative to that where we could group execution related commands: noir-executor execute, noir-executor replay, noir-executor abi decode for example.

An alternative is to introduce an umbrella noir command at some point, that would transparently forward noir <cmd> to noir-<cmd>, which what @asterite suggested git does.

Even then, I think having a separate execute and replay might be better than execute taking either a dump or separate args for inputs and the oracle transcript (which is in itself optional, it could come from RPC, or nowhere). We can keep them as separate binaries, this is just an idea, feel free to close if having separate ones is considered better.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

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

@aakoshh aakoshh requested a review from a team March 6, 2025 10:18
@aakoshh aakoshh changed the title feat: Rename noir-execute to noir-executor feat!: Rename noir-execute to noir-executor Mar 6, 2025
@TomAFrench
Copy link
Member

An alternative is to introduce an umbrella noir command at some point could would transparently forward noir to noir-, which what @asterite suggested git does.

I would be in support of doing this at some point.

@TomAFrench
Copy link
Member

I'll have a proper think about the knock on changes you mention in the OP today.

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.

Short block while we discuss the CLI structure as mentioned.

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.

3 participants