Upgrade passport circuits to latest Noir version#172
Conversation
CodSpeed WallTime Performance ReportMerging #172 will not alter performanceComparing
|
0xVikasRushi
changed the title
0xVikasRushi
force-pushed
the
vr/noir-latest
branch
from
5a32127 to
95190db
Compare
0xVikasRushi
force-pushed
the
vr/noir-latest
branch
from
074912d to
3fcadcc
Compare
|
|
||
| [dependencies] | ||
| sha256 = { tag = "v0.1.1", git = "https://github.com/noir-lang/sha256" } | ||
| sha256 = { tag = "main", git = "https://github.com/noir-lang/sha256" } |
There was a problem hiding this comment.
there are a few changes in v0.2.0 that are causing bugs. Brillig function call isn’t properly covered by a manual constraint in version 0.2.0. However, latest main branch doesn’t produce these bugs. Both cases works, 0.2.0 but produces brillig bug.
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the Noir passport circuits to the latest Noir version (v1.0.0-beta.11), adds a Rust crate for generating US dummy passport data, and includes the noir-rsa dependency for RSA signature verification functionality.
Key changes include:
- Upgrading Noir compiler version requirements from v0.22.0/v0.35.0 to v1.0.0
- Updating function signatures to use u32 instead of Field for array indices
- Adding comprehensive type definitions and test utilities for passport data processing
Reviewed Changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| noir-examples/noir-r1cs-test-programs/*/src/main.nr | Updated array indexing parameters from Field to u32 for memory operations |
| noir-examples/noir-passport-examples/zkpassport_libs/utils/src/types.nr | Added comprehensive type definitions for DG1, MRZ, and passport data structures |
| noir-examples/noir-passport-examples/zkpassport_libs/utils/src/tests.nr | Added extensive test suite with DG1Builder utility and validation functions |
| noir-examples/noir-passport-examples/zkpassport_libs/utils/src/lib.nr | Enhanced utility functions with new data extraction methods and improved type safety |
| noir-examples/noir-passport-examples/zkpassport_libs/sig-check/*/Nargo.toml | Updated dependency versions and compiler requirements |
| noir-examples/noir-passport-examples/zkpassport_libs/commitment/*/src/lib.nr | Updated commitment functions with improved error messages and parameter handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| /// Returns total TLV length (tag + length field + content) for any | ||
| /// ASN.1 element using DER/BER **definite-length** encoding with a | ||
| /// single-byte tag (tag number field < 31) |
There was a problem hiding this comment.
The function name includes 'unsafe' but lacks documentation explaining why it's unsafe and what safety precautions callers should take. Add a doc comment explaining the safety implications and proper usage.
| /// Safety: This is safe because the offset is only used as a starting point | ||
| /// to verify the substring exists |
There was a problem hiding this comment.
This function is marked as 'unsafe' but lacks documentation explaining the safety implications. The safety comment on line 654-655 should be moved to a proper doc comment on this function.
| /// Safety: This is safe because the offset is only used as a starting point | |
| /// to verify the substring exists | |
| /** | |
| * Safety: This is safe because the offset is only used as a starting point | |
| * to verify the substring exists | |
| */ |
| data_to_sign: [u8; DATA_TO_SIGN_LEN], | ||
| data_to_sign_len: u64, | ||
| data_to_sign: [u8; DATA_TO_SIGN_MAX_LEN], | ||
| data_to_sign_len: u32, |
There was a problem hiding this comment.
The parameter type changed from u64 to u32 for data_to_sign_len. This could be a breaking change if the original u64 was intentional for handling very large data sizes. Verify this change is intentional and doesn't limit functionality.
| data_to_sign_len: u32, | |
| data_to_sign_len: u64, |
| if issuing_country == ZKR_COUNTRY_CODE_BYTES { | ||
| // Set the scoped nullifier to 1 to indicate the issuing country is not a real one | ||
| // and prevent the use of these proofs in production | ||
| // Note: ZKPassport's registries on mainnet blockchains will not include | ||
| // the ZKR certificates but still this distinction can be useful for testnets/devnets | ||
| scoped_nullifier = 1; | ||
| } |
There was a problem hiding this comment.
Using the magic number 1 as a special value for test nullifiers is unclear. Consider defining a named constant like TEST_NULLIFIER_VALUE to make the intent more explicit.
Upgrade passport circuits to latest Noir version
complete_age_checkcircuits to the latest Noir version (v1.0.0-beta.11)