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.

Sign up for GitHub

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

fix(noir): Add workaround for latest noir in account contracts #1781

Merged
merged 7 commits into from
Aug 24, 2023
Merged

fix(noir): Add workaround for latest noir in account contracts #1781

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8839fd5
fix: workaround for noir issue with slices
sirasistant Aug 24, 2023
38f388a
test: update account contract test
sirasistant Aug 24, 2023
16269f1
Merge branch 'master' into arv/account_contracts_fix
sirasistant Aug 24, 2023
0f409f4
Merge branch 'master' into arv/account_contracts_fix
sirasistant Aug 24, 2023
3f70986
chore: update acvm js version
sirasistant Aug 24, 2023
29c56c4
Merge branch 'arv/account_contracts_fix' of github.com:AztecProtocol/…
sirasistant Aug 24, 2023
d3d9608
Merge branch 'master' into arv/account_contracts_fix
sirasistant Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 3 additions & 68 deletions yarn-project/aztec.js/src/abis/ecdsa_account_contract.json
View file
Open in desktop

Large diffs are not rendered by default.

69 changes: 2 additions & 67 deletions yarn-project/aztec.js/src/abis/schnorr_account_contract.json
View file
Open in desktop

Large diffs are not rendered by default.

40 changes: 2 additions & 38 deletions yarn-project/aztec.js/src/abis/schnorr_single_key_account_contract.json
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 6 additions & 1 deletion yarn-project/noir-contracts/src/contracts/ecdsa_account_contract/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@ contract EcdsaAccount {
// Note that noir expects the hash of the message/challenge as input to the ECDSA verification.
let payload_fields: [Field; entrypoint::ENTRYPOINT_PAYLOAD_SIZE] = payload.serialize();
let message_field: Field = std::hash::pedersen_with_separator(payload_fields, GENERATOR_INDEX__SIGNATURE_PAYLOAD)[0];
let message_bytes = message_field.to_be_bytes(32);
// TODO workaround for https://github.com/noir-lang/noir/issues/2421
let message_bytes_slice = message_field.to_be_bytes(32);
let mut message_bytes: [u8; 32] = [0; 32];
for i in 0..32 {
message_bytes[i] = message_bytes_slice[i];
}
let hashed_message: [u8; 32] = std::hash::sha256(message_bytes);
let verification = std::ecdsa_secp256k1::verify_signature(public_key.x, public_key.y, signature, hashed_message);
assert(verification == true);
Expand Down
7 changes: 6 additions & 1 deletion yarn-project/noir-contracts/src/contracts/schnorr_account_contract/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,12 @@ contract SchnorrAccount {
// Verify payload signature
let payload_fields: [Field; entrypoint::ENTRYPOINT_PAYLOAD_SIZE] = payload.serialize();
let message_field: Field = std::hash::pedersen_with_separator(payload_fields, GENERATOR_INDEX__SIGNATURE_PAYLOAD)[0];
let message_bytes = message_field.to_be_bytes(32);
// TODO workaround for https://github.com/noir-lang/noir/issues/2421
let message_bytes_slice = message_field.to_be_bytes(32);
let mut message_bytes: [u8; 32] = [0; 32];
for i in 0..32 {
message_bytes[i] = message_bytes_slice[i];
}

// Verify signature of the payload bytes
let verification = std::schnorr::verify_signature(public_key.x, public_key.y, signature, message_bytes);
Expand Down
8 changes: 6 additions & 2 deletions yarn-project/noir-contracts/src/contracts/schnorr_single_key_account_contract/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ contract SchnorrSingleKeyAccount {
// Verify payload signature
let payload_fields: [Field; entrypoint::ENTRYPOINT_PAYLOAD_SIZE] = payload.serialize();
let message_field: Field = std::hash::pedersen_with_separator(payload_fields, GENERATOR_INDEX__SIGNATURE_PAYLOAD)[0];
let message_bytes = message_field.to_be_bytes(32);
// TODO workaround for https://github.com/noir-lang/noir/issues/2421
let message_bytes_slice = message_field.to_be_bytes(32);
let mut message_bytes: [u8; 32] = [0; 32];
for i in 0..32 {
message_bytes[i] = message_bytes_slice[i];
}

// Convert owner pubkey into fields
let mut x: Field = 0;
Expand All @@ -50,7 +55,6 @@ contract SchnorrSingleKeyAccount {
}

// Verify signature of the payload hash
// TODO: Find out why this signature verification never fails
let verification = std::schnorr::verify_signature(x, y, signature, message_bytes);
assert(verification == true);

Expand Down
2 changes: 2 additions & 0 deletions yarn-project/noir-contracts/src/scripts/copy_output.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ function writeToProject(abi: any) {
const toWrite = {
...abi,
functions: abi.functions.map((f: any) => omit(f, projectContract.exclude)),
// If we maintain debug symbols they will get commited to git.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried untracking the account contracts but it generates a chicken and egg issue, where noir-contracts depends on aztec.js but it writes assets to aztec.js so if the account contracts abis are untracked noir-contracts will fail to build and thus to generate the account contract abis 😢

Copy link
Member

Choose a reason for hiding this comment

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

Can we move around the deps to avoid this? some packages can depend on the noir-contracts package and copy the built artifacts over

Copy link
Collaborator Author

@sirasistant sirasistant Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure we can move the deps around easily, because if aztec.js depends on compilation results then compilation cannot depend on aztec.js. I think the best solution would be for noir-contracts to not depend on aztec.js, but for that I think we'd need to remove types generation out of noir-contracts

debug: undefined,
};
const targetFilename = pathJoin(projectContract.target, `${snakeCase(abi.name)}_contract.json`);
writeFileSync(targetFilename, JSON.stringify(toWrite, null, 2) + '\n');
Expand Down