-
Notifications
You must be signed in to change notification settings - Fork 270
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: aztec nr lib constraining nullifier key is fresh #5939
Conversation
Benchmark resultsNo metrics with a significant change found. Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method
Transaction processing duration by data writes.
|
let derived_slot = pedersen_hash( | ||
[storage_slot_of_nullifier_public_key, address.to_field()], | ||
0 | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having hardcoded the inner logic of a Map here would result in hard to debug bug if something got changed there. I would introduce a derive_map_storage_slot function and use it from both Map and here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍. Addressed in 0ca8578
nullifier_public_key_to_test: GrumpkinPoint, | ||
) { | ||
// This is the storage slot of the nullifier_public_key inside the key registry contract | ||
let storage_slot_of_nullifier_public_key = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment to canonical registry that we have the slot hardcoded here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this, could we not expose "global" values on the struct directly? Seems like it could be super sleek it we could do something like
registry::SOME_STORAGE_SLOT_CONSTANT
@Thunkar do you have any thoughts here? I have actually not checked if this is already possible with the code you made 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the original comment in 0ca8578
|
||
// It's a bit wonky because we need to know the delay for get_current_value_in_private to work correctly | ||
// We read from the canonical Key Registry | ||
let registry_private_getter: SharedMutablePrivateGetter<Field, 5> = SharedMutablePrivateGetter::new(*context, AztecAddress::from_field(CANONICAL_KEY_REGISTRY_ADDRESS), derived_slot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also add comment to registry that we use the value here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 0ca8578
assert(keys[key_type].eq(key)); | ||
} | ||
|
||
fn get_public_keys_internal(address: AztecAddress) -> [GrumpkinPoint; 4] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like having this function just makes it harder to read then if you just had line 64 on line 58 but maybe it's just a matter of taste
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it is mostly taste, but I agree, if not using more than once would just insert it directly in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also gree with your matter of taste, I thought it was convention to do it this way but I guess I was wrong. Addressed in 0ca8578
get_public_keys_and_partial_address_oracle(address) | ||
} | ||
|
||
pub fn get_public_keys_and_partial_address(address: AztecAddress) -> (PartialAddress, [GrumpkinPoint; 4]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you aware of us needing the partial address somewhere?
As far as I know the whole purpose of it is to be able to check the preimage of aztec address which we only do internally in the function so I don't think it makes sense to return here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 0ca8578
describe('key rotation flows', () => { | ||
const firstNewMasterNullifierPublicKey = Point.random(); | ||
|
||
describe('normal key rotation flow', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that all the 3 cases in this describe block are interdependent I think it would make sense to just have them as one test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, addressed this and the other obvious cases in 0ca8578
}); | ||
|
||
it('checks our registry contract from test contract and finds the new nullifier public key that has been rotated', async () => { | ||
it('in the case where the key exists both in the pxe and our registry, we know nothing weird will happen', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"nothing weird will happen" is not descriptive enough. Also I feel all the test names are not concise enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I've tried to make a pass on the test names, please let me know if unsatisfactory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a few comments beyond and as extension to the ones Jan added.
NULLIFIER: 0, | ||
}; | ||
|
||
pub fn assert_nullifier_public_key_is_fresh( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed yesterday, think it would be better if we just have the getter here that is doing the assertions. In my mental model, you want to retrieve it for a user when you are populating the note, so having just one function do it instead of doing a retrieval and then a check that is doing the same internally seems nicer in my mind.
A similar change was made with the public storage some time back as an application can then easily add an explicit check if it want to see if it is a specific value etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, addressed as part of a stack, but popped into this pr for ease of review.
Addressed in 0ca8578
@@ -0,0 +1,67 @@ | |||
use dep::protocol_types::{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there different number of spaces per tab between users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will stick to four from now on 🙃
nullifier_public_key_to_test: GrumpkinPoint, | ||
) { | ||
// This is the storage slot of the nullifier_public_key inside the key registry contract | ||
let storage_slot_of_nullifier_public_key = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this, could we not expose "global" values on the struct directly? Seems like it could be super sleek it we could do something like
registry::SOME_STORAGE_SLOT_CONSTANT
@Thunkar do you have any thoughts here? I have actually not checked if this is already possible with the code you made 👀
assert(keys[key_type].eq(key)); | ||
} | ||
|
||
fn get_public_keys_internal(address: AztecAddress) -> [GrumpkinPoint; 4] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it is mostly taste, but I agree, if not using more than once would just insert it directly in there
} | ||
} | ||
|
||
fn check_public_key_validity(address: AztecAddress, key_type: u8, key: GrumpkinPoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the comment below, think this could be done inline, seems a bit unnecessary to have these two separate functions when it is as small as they are. Seems like it would just be two lines instead of two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 0ca8578
(partial_address, [nullifier_pub_key, incoming_pub_key, outgoing_pub_key, tagging_pub_key]) | ||
} | ||
|
||
fn _check_public_key_validity_constrain_oracle( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would likely be easier to follow, if you have that the function is returning the address instead, and then constrain it in the caller. Would then have function that is essentially take the preimage and compue, which should be similar to what we have other places (as jan mentioned)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, addresssed in 0ca8578
noir-projects/noir-contracts/contracts/test_contract/src/main.nr
Outdated
Show resolved
Hide resolved
@@ -6,7 +6,7 @@ | |||
"moduleNameMapper": { | |||
"^(\\.{1,2}/.*)\\.js$": "$1" | |||
}, | |||
"reporters": [["default", {"summaryThreshold": 9999}]], | |||
"reporters": [["default", { "summaryThreshold": 9999 }]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like just a good ol formatting
@@ -171,6 +172,27 @@ export class Oracle { | |||
return capsule.map(toACVMField); | |||
} | |||
|
|||
async getPublicKeysAndPartialAddress([address]: ACVMField[]): Promise<ACVMField[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name and the ordering of the return values messes with my brain. If we have the the key first in the name I would also assume it is also first in the return values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Addressed in 0ca8578
Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments
noir-projects/noir-contracts/contracts/key_registry_contract/src/main.nr
Show resolved
Hide resolved
noir-projects/noir-contracts/contracts/test_contract/src/main.nr
Outdated
Show resolved
Hide resolved
noir-projects/noir-protocol-circuits/crates/types/src/address/public_keys_hash.nr
Outdated
Show resolved
Hide resolved
…/aztec-packages#5939) resolves #5688 --------- Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
…/aztec-packages#5939) resolves #5688 --------- Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
…/aztec-packages#5939) resolves #5688 --------- Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
…/aztec-packages#5939) resolves #5688 --------- Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.37.0</summary> ## [0.37.0](aztec-package-v0.36.0...aztec-package-v0.37.0) (2024-05-02) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.37.0</summary> ## [0.37.0](barretenberg.js-v0.36.0...barretenberg.js-v0.37.0) (2024-05-02) ### Features * Devbox ([#5772](#5772)) ([72321f9](72321f9)) </details> <details><summary>aztec-packages: 0.37.0</summary> ## [0.37.0](aztec-packages-v0.36.0...aztec-packages-v0.37.0) (2024-05-02) ### ⚠ BREAKING CHANGES * use `distinct` return value witnesses by default (noir-lang/noir#4951) * Bit shift is restricted to u8 right operand (noir-lang/noir#4907) ### Features * Abort ongoing proving jobs ([#6049](#6049)) ([0aa352d](0aa352d)) * Add aztecprotocol/aztec-builder ([#6116](#6116)) ([30899d0](30899d0)) * Add de-sugaring for `impl Trait` in function parameters (noir-lang/noir#4919) ([f060fa6](f060fa6)) * Aztec nr lib constraining nullifier key is fresh ([#5939](#5939)) ([f95de6b](f95de6b)) * Bit shift is restricted to u8 right operand (noir-lang/noir#4907) ([f060fa6](f060fa6)) * Count Bb lines weighted by complexity ([#6090](#6090)) ([705177f](705177f)) * Devbox ([#5772](#5772)) ([72321f9](72321f9)) * Enforce gas limits from private kernels ([#6105](#6105)) ([4395855](4395855)) * **experimental:** `comptime` globals (noir-lang/noir#4918) ([f060fa6](f060fa6)) * Handle `no_predicates` attribute (noir-lang/noir#4942) ([4dc5efb](4dc5efb)) * Migrate boxes to GA and Earthly ([#6076](#6076)) ([4a49f9d](4a49f9d)) * Pippenger benchmarks compatible with wasmtime ([#6095](#6095)) ([5297b5b](5297b5b)) * Private da gas metering ([#6103](#6103)) ([1a8f372](1a8f372)) * Prover metrics ([#6050](#6050)) ([5b133f2](5b133f2)) * Use `distinct` return value witnesses by default (noir-lang/noir#4951) ([4dc5efb](4dc5efb)) ### Bug Fixes * Ban self-referential structs (noir-lang/noir#4883) ([f060fa6](f060fa6)) * **ci:** Build-key hotfix ([#6123](#6123)) ([5791004](5791004)) * **ci:** Ssh'ing into instances ([#6136](#6136)) ([af3192d](af3192d)) * Discard ref counts during unrolling (noir-lang/noir#4923) ([f060fa6](f060fa6)) * **docs:** Add codegen to `aztec-builder` command ([#6098](#6098)) ([4839ed9](4839ed9)) * Ensure where clauses propagated to trait default definitions (noir-lang/noir#4894) ([4dc5efb](4dc5efb)) * Require for all foldable functions to use distinct return (noir-lang/noir#4949) ([4dc5efb](4dc5efb)) ### Miscellaneous * Add regression test for [#3051](#3051) (noir-lang/noir#4815) ([f060fa6](f060fa6)) * Add test for recursing a foldable function (noir-lang/noir#4948) ([4dc5efb](4dc5efb)) * Adding devcontainer with create aztec app ([#5960](#5960)) ([ae5cb21](ae5cb21)) * Build docs in earthly ([#6038](#6038)) ([784d542](784d542)) * Bump bench-tx-size timeout ([#6109](#6109)) ([aa3eefa](aa3eefa)) * **ci:** Fix spot runner build key ([#6119](#6119)) ([f332bc9](f332bc9)) * **ci:** Hotfix ([#6124](#6124)) ([f60dfcd](f60dfcd)) * **ci:** Run benchmarks on Earthly ([#6089](#6089)) ([c985c73](c985c73)) * **ci:** Turn off ARM build for now ([#6135](#6135)) ([853913f](853913f)) * Disable bench-summary for now ([67485f1](67485f1)) * Disable doc builds ([#6107](#6107)) ([7933f0f](7933f0f)) * **docs:** Adding matomo tracking (noir-lang/noir#4898) ([4dc5efb](4dc5efb)) * Ebs attach robustness ([#6108](#6108)) ([c702688](c702688)) * Fix typo in `ResolverError::AbiAttributeOutsideContract` (noir-lang/noir#4933) ([4dc5efb](4dc5efb)) * Migrate the prover client test to earthly ([#6118](#6118)) ([a59a6c0](a59a6c0)) * Redo typo PR by stayweek ([#6080](#6080)) ([0869452](0869452)) * Redo typo PR by vitalmotif ([#6081](#6081)) ([1a89d1a](1a89d1a)) * Refactor nested contract test for speed ([#6117](#6117)) ([b346a2f](b346a2f)) * Remove unnecessary `pub(super)` in interpreter (noir-lang/noir#4939) ([4dc5efb](4dc5efb)) * Replace relative paths to noir-protocol-circuits ([47592a2](47592a2)) * Replace relative paths to noir-protocol-circuits ([f0d95f5](f0d95f5)) * Update error conversion traits to act on references (noir-lang/noir#4936) ([f060fa6](f060fa6)) ### Documentation * Tweaks for release ([#6129](#6129)) ([77b45b9](77b45b9)) * Update @aztec/builder readme ([#6115](#6115)) ([248761e](248761e)) * Yellow paper updates for the parity circuits ([#6048](#6048)) ([cfe1b05](cfe1b05)) </details> <details><summary>barretenberg: 0.37.0</summary> ## [0.37.0](barretenberg-v0.36.0...barretenberg-v0.37.0) (2024-05-02) ### ⚠ BREAKING CHANGES * use `distinct` return value witnesses by default (noir-lang/noir#4951) ### Features * Count Bb lines weighted by complexity ([#6090](#6090)) ([705177f](705177f)) * Devbox ([#5772](#5772)) ([72321f9](72321f9)) * Handle `no_predicates` attribute (noir-lang/noir#4942) ([4dc5efb](4dc5efb)) * Pippenger benchmarks compatible with wasmtime ([#6095](#6095)) ([5297b5b](5297b5b)) * Use `distinct` return value witnesses by default (noir-lang/noir#4951) ([4dc5efb](4dc5efb)) ### Bug Fixes * Ensure where clauses propagated to trait default definitions (noir-lang/noir#4894) ([4dc5efb](4dc5efb)) * Require for all foldable functions to use distinct return (noir-lang/noir#4949) ([4dc5efb](4dc5efb)) ### Miscellaneous * Add test for recursing a foldable function (noir-lang/noir#4948) ([4dc5efb](4dc5efb)) * **docs:** Adding matomo tracking (noir-lang/noir#4898) ([4dc5efb](4dc5efb)) * Fix typo in `ResolverError::AbiAttributeOutsideContract` (noir-lang/noir#4933) ([4dc5efb](4dc5efb)) * Redo typo PR by stayweek ([#6080](#6080)) ([0869452](0869452)) * Remove unnecessary `pub(super)` in interpreter (noir-lang/noir#4939) ([4dc5efb](4dc5efb)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.37.0</summary> ## [0.37.0](AztecProtocol/aztec-packages@aztec-package-v0.36.0...aztec-package-v0.37.0) (2024-05-02) ### Miscellaneous * **aztec-package:** Synchronize aztec-packages versions </details> <details><summary>barretenberg.js: 0.37.0</summary> ## [0.37.0](AztecProtocol/aztec-packages@barretenberg.js-v0.36.0...barretenberg.js-v0.37.0) (2024-05-02) ### Features * Devbox ([#5772](AztecProtocol/aztec-packages#5772)) ([72321f9](AztecProtocol/aztec-packages@72321f9)) </details> <details><summary>aztec-packages: 0.37.0</summary> ## [0.37.0](AztecProtocol/aztec-packages@aztec-packages-v0.36.0...aztec-packages-v0.37.0) (2024-05-02) ### ⚠ BREAKING CHANGES * use `distinct` return value witnesses by default (noir-lang/noir#4951) * Bit shift is restricted to u8 right operand (noir-lang/noir#4907) ### Features * Abort ongoing proving jobs ([#6049](AztecProtocol/aztec-packages#6049)) ([0aa352d](AztecProtocol/aztec-packages@0aa352d)) * Add aztecprotocol/aztec-builder ([#6116](AztecProtocol/aztec-packages#6116)) ([30899d0](AztecProtocol/aztec-packages@30899d0)) * Add de-sugaring for `impl Trait` in function parameters (noir-lang/noir#4919) ([f060fa6](AztecProtocol/aztec-packages@f060fa6)) * Aztec nr lib constraining nullifier key is fresh ([#5939](AztecProtocol/aztec-packages#5939)) ([f95de6b](AztecProtocol/aztec-packages@f95de6b)) * Bit shift is restricted to u8 right operand (noir-lang/noir#4907) ([f060fa6](AztecProtocol/aztec-packages@f060fa6)) * Count Bb lines weighted by complexity ([#6090](AztecProtocol/aztec-packages#6090)) ([705177f](AztecProtocol/aztec-packages@705177f)) * Devbox ([#5772](AztecProtocol/aztec-packages#5772)) ([72321f9](AztecProtocol/aztec-packages@72321f9)) * Enforce gas limits from private kernels ([#6105](AztecProtocol/aztec-packages#6105)) ([4395855](AztecProtocol/aztec-packages@4395855)) * **experimental:** `comptime` globals (noir-lang/noir#4918) ([f060fa6](AztecProtocol/aztec-packages@f060fa6)) * Handle `no_predicates` attribute (noir-lang/noir#4942) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Migrate boxes to GA and Earthly ([#6076](AztecProtocol/aztec-packages#6076)) ([4a49f9d](AztecProtocol/aztec-packages@4a49f9d)) * Pippenger benchmarks compatible with wasmtime ([#6095](AztecProtocol/aztec-packages#6095)) ([5297b5b](AztecProtocol/aztec-packages@5297b5b)) * Private da gas metering ([#6103](AztecProtocol/aztec-packages#6103)) ([1a8f372](AztecProtocol/aztec-packages@1a8f372)) * Prover metrics ([#6050](AztecProtocol/aztec-packages#6050)) ([5b133f2](AztecProtocol/aztec-packages@5b133f2)) * Use `distinct` return value witnesses by default (noir-lang/noir#4951) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) ### Bug Fixes * Ban self-referential structs (noir-lang/noir#4883) ([f060fa6](AztecProtocol/aztec-packages@f060fa6)) * **ci:** Build-key hotfix ([#6123](AztecProtocol/aztec-packages#6123)) ([5791004](AztecProtocol/aztec-packages@5791004)) * **ci:** Ssh'ing into instances ([#6136](AztecProtocol/aztec-packages#6136)) ([af3192d](AztecProtocol/aztec-packages@af3192d)) * Discard ref counts during unrolling (noir-lang/noir#4923) ([f060fa6](AztecProtocol/aztec-packages@f060fa6)) * **docs:** Add codegen to `aztec-builder` command ([#6098](AztecProtocol/aztec-packages#6098)) ([4839ed9](AztecProtocol/aztec-packages@4839ed9)) * Ensure where clauses propagated to trait default definitions (noir-lang/noir#4894) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Require for all foldable functions to use distinct return (noir-lang/noir#4949) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) ### Miscellaneous * Add regression test for [#3051](AztecProtocol/aztec-packages#3051) (noir-lang/noir#4815) ([f060fa6](AztecProtocol/aztec-packages@f060fa6)) * Add test for recursing a foldable function (noir-lang/noir#4948) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Adding devcontainer with create aztec app ([#5960](AztecProtocol/aztec-packages#5960)) ([ae5cb21](AztecProtocol/aztec-packages@ae5cb21)) * Build docs in earthly ([#6038](AztecProtocol/aztec-packages#6038)) ([784d542](AztecProtocol/aztec-packages@784d542)) * Bump bench-tx-size timeout ([#6109](AztecProtocol/aztec-packages#6109)) ([aa3eefa](AztecProtocol/aztec-packages@aa3eefa)) * **ci:** Fix spot runner build key ([#6119](AztecProtocol/aztec-packages#6119)) ([f332bc9](AztecProtocol/aztec-packages@f332bc9)) * **ci:** Hotfix ([#6124](AztecProtocol/aztec-packages#6124)) ([f60dfcd](AztecProtocol/aztec-packages@f60dfcd)) * **ci:** Run benchmarks on Earthly ([#6089](AztecProtocol/aztec-packages#6089)) ([c985c73](AztecProtocol/aztec-packages@c985c73)) * **ci:** Turn off ARM build for now ([#6135](AztecProtocol/aztec-packages#6135)) ([853913f](AztecProtocol/aztec-packages@853913f)) * Disable bench-summary for now ([67485f1](AztecProtocol/aztec-packages@67485f1)) * Disable doc builds ([#6107](AztecProtocol/aztec-packages#6107)) ([7933f0f](AztecProtocol/aztec-packages@7933f0f)) * **docs:** Adding matomo tracking (noir-lang/noir#4898) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Ebs attach robustness ([#6108](AztecProtocol/aztec-packages#6108)) ([c702688](AztecProtocol/aztec-packages@c702688)) * Fix typo in `ResolverError::AbiAttributeOutsideContract` (noir-lang/noir#4933) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Migrate the prover client test to earthly ([#6118](AztecProtocol/aztec-packages#6118)) ([a59a6c0](AztecProtocol/aztec-packages@a59a6c0)) * Redo typo PR by stayweek ([#6080](AztecProtocol/aztec-packages#6080)) ([0869452](AztecProtocol/aztec-packages@0869452)) * Redo typo PR by vitalmotif ([#6081](AztecProtocol/aztec-packages#6081)) ([1a89d1a](AztecProtocol/aztec-packages@1a89d1a)) * Refactor nested contract test for speed ([#6117](AztecProtocol/aztec-packages#6117)) ([b346a2f](AztecProtocol/aztec-packages@b346a2f)) * Remove unnecessary `pub(super)` in interpreter (noir-lang/noir#4939) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Replace relative paths to noir-protocol-circuits ([47592a2](AztecProtocol/aztec-packages@47592a2)) * Replace relative paths to noir-protocol-circuits ([f0d95f5](AztecProtocol/aztec-packages@f0d95f5)) * Update error conversion traits to act on references (noir-lang/noir#4936) ([f060fa6](AztecProtocol/aztec-packages@f060fa6)) ### Documentation * Tweaks for release ([#6129](AztecProtocol/aztec-packages#6129)) ([77b45b9](AztecProtocol/aztec-packages@77b45b9)) * Update @aztec/builder readme ([#6115](AztecProtocol/aztec-packages#6115)) ([248761e](AztecProtocol/aztec-packages@248761e)) * Yellow paper updates for the parity circuits ([#6048](AztecProtocol/aztec-packages#6048)) ([cfe1b05](AztecProtocol/aztec-packages@cfe1b05)) </details> <details><summary>barretenberg: 0.37.0</summary> ## [0.37.0](AztecProtocol/aztec-packages@barretenberg-v0.36.0...barretenberg-v0.37.0) (2024-05-02) ### ⚠ BREAKING CHANGES * use `distinct` return value witnesses by default (noir-lang/noir#4951) ### Features * Count Bb lines weighted by complexity ([#6090](AztecProtocol/aztec-packages#6090)) ([705177f](AztecProtocol/aztec-packages@705177f)) * Devbox ([#5772](AztecProtocol/aztec-packages#5772)) ([72321f9](AztecProtocol/aztec-packages@72321f9)) * Handle `no_predicates` attribute (noir-lang/noir#4942) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Pippenger benchmarks compatible with wasmtime ([#6095](AztecProtocol/aztec-packages#6095)) ([5297b5b](AztecProtocol/aztec-packages@5297b5b)) * Use `distinct` return value witnesses by default (noir-lang/noir#4951) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) ### Bug Fixes * Ensure where clauses propagated to trait default definitions (noir-lang/noir#4894) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Require for all foldable functions to use distinct return (noir-lang/noir#4949) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) ### Miscellaneous * Add test for recursing a foldable function (noir-lang/noir#4948) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * **docs:** Adding matomo tracking (noir-lang/noir#4898) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Fix typo in `ResolverError::AbiAttributeOutsideContract` (noir-lang/noir#4933) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) * Redo typo PR by stayweek ([#6080](AztecProtocol/aztec-packages#6080)) ([0869452](AztecProtocol/aztec-packages@0869452)) * Remove unnecessary `pub(super)` in interpreter (noir-lang/noir#4939) ([4dc5efb](AztecProtocol/aztec-packages@4dc5efb)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
try { | ||
publicKeys = await this.typedOracle.getPublicKeysForAddress(AztecAddress.fromField(fromACVMField(address))); | ||
} catch (err) { | ||
publicKeys = Array(4).fill(Point.ZERO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you caught here the error and populated the keys with invalid values here ^? This just makes the code hard to debug. Had to refactor this in my PR after debugging a failed constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, my bad, thanks for the heads up. I think initially there was a case of wanting to support grabbing either pub keys or partial address from this but yeah, it was stupid and I had changed stuff back while forgetting to change this back as well.
resolves #5688