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

Jump to bottom

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 all commits
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
2 changes: 1 addition & 1 deletion yarn-project/acir-simulator/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"@aztec/circuits.js": "workspace:^",
"@aztec/foundation": "workspace:^",
"@aztec/types": "workspace:^",
"acvm_js": "github:noir-lang/acvm-js-wasm#arv/0.22+init-pedersen",
"acvm_js": "github:noir-lang/acvm-js-wasm#arv/0.23.0_prerelease",
"levelup": "^5.1.1",
"memdown": "^6.1.1",
"tslib": "^2.4.0"
Expand Down
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.

4 changes: 3 additions & 1 deletion yarn-project/end-to-end/src/e2e_account_contracts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ function itShouldBehaveLikeAnAccountContract(getAccountContract: (encryptionKey:
accountAddress,
).getWallet();
const childWithInvalidWallet = await ChildContract.at(child.address, invalidWallet);
await expect(childWithInvalidWallet.methods.value(42).simulate()).rejects.toThrowError(/Assertion failed: '.*'/);
await expect(childWithInvalidWallet.methods.value(42).simulate()).rejects.toThrowError(
/Cannot satisfy constraint.*/,
);
});
});
}
Expand Down
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
Contributor 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
Contributor 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
10 changes: 5 additions & 5 deletions yarn-project/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ __metadata:
"@rushstack/eslint-patch": ^1.1.4
"@types/jest": ^29.5.0
"@types/node": ^18.7.23
acvm_js: "github:noir-lang/acvm-js-wasm#arv/0.22+init-pedersen"
acvm_js: "github:noir-lang/acvm-js-wasm#arv/0.23.0_prerelease"
jest: ^29.5.0
jest-mock-extended: ^3.0.4
levelup: ^5.1.1
Expand Down Expand Up @@ -4021,10 +4021,10 @@ __metadata:
languageName: node
linkType: hard

"acvm_js@github:noir-lang/acvm-js-wasm#arv/0.22+init-pedersen":
version: 0.22.0
resolution: "acvm_js@https://github.com/noir-lang/acvm-js-wasm.git#commit=4ccaee804a9bbb5c8e3bb2adc74382a8fcff0d42"
checksum: 8f4802a4af520957a512266e3c7fb106af893209feb807d68991d374bc30d9899cdf09e3d9967d152a78e6bc8eb58e4ad397bff94f2e4b99d324b7fbfdcfbeab
"acvm_js@github:noir-lang/acvm-js-wasm#arv/0.23.0_prerelease":
version: 0.0.0-be7fafd
resolution: "acvm_js@https://github.com/noir-lang/acvm-js-wasm.git#commit=89b4ddef96ec4098d53be9d532952043502f53b9"
checksum: 469cfb1654781f928e203ebdde52e06e4633d5b319c5301a4f29a07961ef763059dd539ca37d179cbf5e786bdf114fa9767eea27d5f0c1af6bdb802360538297
languageName: node
linkType: hard

Expand Down