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 compute_note_hash_and_nullifier compiler check #3816

Closed

Conversation

benesjan
Copy link
Contributor

Fixes AztecProtocol/aztec-packages#3669

It's desirable to have nicely typed args in our noir contracts and for this reason I want to change signature of compute_note_hash_and_nullifier from:
compute_note_hash_and_nullifier(Field,Field,Field,[Field; N]) -> [Field; 4]

to:
compute_note_hash_and_nullifier(AztecAddress,Field,Field,[Field; N]) -> [Field; 4]

This PR updates the corresponding compiler check so that this can be achieved.

Copy link
Contributor

Changes to circuit sizes

Generated at commit: dd02795cc40dc2f4916c338aa03f2c4dce72fe37, compared to commit: fc00722cf9df4183f116411eddfa59834a02aa07

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
pred_eq +4 ❌ +400.00% +3 ❌ +50.00%
tuple_inputs +1 ❌ +1.85% +3 ❌ +0.08%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
pred_eq 5 (+4) +400.00% 9 (+3) +50.00%
tuple_inputs 55 (+1) +1.85% 3,689 (+3) +0.08%
conditional_2 21 (+1) +5.00% 2,765 (+1) +0.04%

@TomAFrench
Copy link
Member

Why not make this change inside aztec-packages?

@benesjan
Copy link
Contributor Author

benesjan commented Dec 15, 2023

@TomAFrench TBH didn't know that you can send a PR to a submodule (noir is a sumbodule in aztec-packages). Is that what you meant? Will give it a try if yes.

@TomAFrench
Copy link
Member

TomAFrench commented Dec 15, 2023

Yeah, you can make PRs in aztec-packages with changes in the noir directory and those get mirrored across. If you're making changes to just Noir it's best to do it here but seeing as this change is only relevant to Aztec then I think it makes sense to do over there (so Aztec CI runs on the PR)

@benesjan
Copy link
Contributor Author

Was not aware for that. Thanks for the tip. Will re-open the PR there.

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.

2 participants