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

chore(cli): Use noir_artifact_cli::fs to read artifacts #7391

Merged
merged 59 commits into from
Feb 26, 2025

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Feb 14, 2025

Description

Problem*

Resolves #7381
Builds on #7384

Summary*

Changes other CLI tools to use the noir_artifact_cli::fs functions to read artifacts.

  • inspector (now handles contracts as well)
  • profiler (still only expects programs)
  • nargo (only executes/infos programs)
  • criterion benches

Additional Context

The PR contains some fixes that came out trying to get a more uniform handling of programs vs contracts:

  • The ContractFunctionArtifact had brillig_names, but not names; that seems to be the only missing piece to be able to show inspector info about contract functions, so I added it.
  • Adds hash to ContractFunctionArtifact so that we can map it back into a CompiledProgram without the loss of information
  • Changes the serialisation of hash: u64 to be as String, so that the last digits don't get truncated in the TypeScript types
  • Changes the TypeScript representation of hash to number | string for backwards compatibility (although old TS would not read new format)
  • I got a test failure due to a mismatch between the hash calculated by nargo and noir-wasm for the contract functions. Unfortunately I didn't notice that for programs they are explicitly deleted by the test until after I debugged the issue to find out that it is down to the FileIds being different, which I was able to fix by changing HashMap to BTreeMap in the wasm compile module. This worked for the test contract, but not the test programs, those still needed the field deleted. Later I understood that nargo adds sources to the FileManager as it visits dependencies, whereas in TypeScript we resolve all dependencies, then add them to the source, and then feed them to the FileManager, so the order is going to be different. Nevertheless I think BTreeMap should at least ensure that TypeScript should agree between different browsers for example, or anything else that could affect the hashes in a HashMap.

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 changed the base branch from master to 7132-artifact-cli February 14, 2025 16:06
@aakoshh aakoshh changed the title 7381 use artifact fs chore(cli): Use noir_artifact_cli::fs to read artefacts Feb 14, 2025
@aakoshh aakoshh marked this pull request as ready for review February 14, 2025 18:12
@aakoshh aakoshh requested a review from a team February 14, 2025 18:12
@aakoshh aakoshh requested review from a team and removed request for a team February 17, 2025 12:40
Base automatically changed from 7132-artifact-cli to master February 19, 2025 18:21
@aakoshh aakoshh requested a review from TomAFrench February 19, 2025 18:38
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.

Some nits, generally looks good.

@aakoshh aakoshh requested a review from TomAFrench February 26, 2025 12:47
@TomAFrench TomAFrench added this pull request to the merge queue Feb 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 26, 2025
@aakoshh aakoshh enabled auto-merge February 26, 2025 19:05
@aakoshh aakoshh added this pull request to the merge queue Feb 26, 2025
Merged via the queue into master with commit 3869fe9 Feb 26, 2025
103 checks passed
@aakoshh aakoshh deleted the 7381-use-artifact-fs branch February 26, 2025 19:29
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 26, 2025
chore: revert bump of base64ct to `v1.6.0` after `v1.7.0` yanked (noir-lang/noir#7541)
feat(performance): Avoid extra Brillig array offsetting for constant indices  (noir-lang/noir#7522)
feat: test error spans and messages (noir-lang/noir#7526)
chore: bump wasm-bindgen (noir-lang/noir#7535)
chore: parallelize `test_transform_program_is_idempotent` (noir-lang/noir#7539)
chore(cli): Use `noir_artifact_cli::fs` to read artifacts (noir-lang/noir#7391)
fix(experimental): Replace most remaining match panics with errors (noir-lang/noir#7536)
chore: bump external pinned commits (noir-lang/noir#7537)
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.

Use the artifact CLI file system libraries in other CLI tools
2 participants