Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat(acir)!: Add a public outputs field #56

Merged
merged 8 commits into from
Mar 20, 2023
Merged

Conversation

kevaundray
Copy link
Contributor

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves (link to issue)

Description

Summary of changes

(Describe the changes in this PR. Point out breaking changes if any.)

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

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.

Additional context

(If applicable.)

@kevaundray kevaundray changed the title Add a public outputs field feat!: Add a public outputs field Feb 3, 2023
@TomAFrench TomAFrench linked an issue Mar 1, 2023 that may be closed by this pull request
@TomAFrench
Copy link
Member

Now we have an explicit return type on the ABI this should be able to be handled seamlessly by the evaluator. Would you like me to fix up any conflicts so we can get this merged?

@kevaundray
Copy link
Contributor Author

Just merged the conflicts -- One thing to double check is that the definition of public outputs here also matches the definition being used in Noir

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.

Based on https://aztecprotocol.slack.com/archives/C0183F0V42V/p1676636370362909?thread_ts=1676636143.866669&cid=C0183F0V42V, we may want to take the opportunity to rename these fields for more clarity.

acir/src/circuit/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

LGTM - one small comment re docs

@kevaundray
Copy link
Contributor Author

This LGTM -- @TomAFrench you'll need to approve since the PR was originally made by me

@TomAFrench
Copy link
Member

Ah, that's true. Thanks. Approving now.

@TomAFrench TomAFrench added this pull request to the merge queue Mar 20, 2023
@TomAFrench TomAFrench changed the title feat!: Add a public outputs field feat(acir)!: Add a public outputs field Mar 20, 2023
@TomAFrench TomAFrench merged commit 5f358a9 into master Mar 20, 2023
@github-actions github-actions bot mentioned this pull request Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce the concept of a public output
2 participants