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!: update bberg ACIR parser to target ACVM 0.21.0 #1235

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

TomAFrench
Copy link
Member

Description

Updates the ACIR parser to be compatible with the format outputted by the new nargo nightlies.

bb-bin-tests has been disabled as it tests recursion which is not implemented in the new SSA code (and the existing artifacts are incompatible with the new serialisation logic)

Mirrors AztecProtocol/barretenberg#631

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@netlify
Copy link

netlify bot commented Jul 27, 2023

Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

Name Link
🔨 Latest commit 6c5dd13
🔍 Latest deploy log https://app.netlify.com/sites/preeminent-bienenstitch-606ad0/deploys/64c2580ba1765b00086cff12

@TomAFrench TomAFrench force-pushed the acvm-0.21.0-compat branch from a5f91c9 to 9a17faf Compare July 27, 2023 11:17
@Maddiaa0
Copy link
Member

Merging master to fix stale l1-contracts tag issue

@ludamad
Copy link
Collaborator

ludamad commented Jul 27, 2023

Thinking aloud: Should we create an issue to reinstate bb-bin-test or is it no longer relevant?

@Maddiaa0
Copy link
Member

Thinking aloud: Should we create an issue to reinstate bb-bin-test or is it no longer relevant?

I think so

@ludamad
Copy link
Collaborator

ludamad commented Jul 27, 2023

@TomAFrench mind leaving a bread crumb to this around in the commented out CI tasks? #1236

@@ -4494,6 +4458,7 @@ void serde::Serializable<Circuit::Circuit>::serialize(const Circuit::Circuit& ob
serializer.increase_container_depth();
serde::Serializable<decltype(obj.current_witness_index)>::serialize(obj.current_witness_index, serializer);
serde::Serializable<decltype(obj.opcodes)>::serialize(obj.opcodes, serializer);
serde::Serializable<decltype(obj.private_parameters)>::serialize(obj.private_parameters, serializer);
Copy link
Collaborator

@ludamad ludamad Jul 27, 2023

Choose a reason for hiding this comment

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

I'm surprised the decltype pattern is needed here vs deriving the type from the first arg somehow (I guess we'd need a helper function though)

Copy link
Collaborator

@ludamad ludamad left a comment

Choose a reason for hiding this comment

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

LGTM overall. We really need to come up with a representation of what Noir we target in the repo next (currently a little free-for-all with people bumping noir locally)

@TomAFrench TomAFrench merged commit dc9052e into master Jul 27, 2023
@TomAFrench TomAFrench deleted the acvm-0.21.0-compat branch July 27, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants