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: buggy e2e key registry test setup #6496

Merged
merged 1 commit into from
May 17, 2024
Merged

fix: buggy e2e key registry test setup #6496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion yarn-project/circuits.js/src/types/public_keys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { poseidon2Hash } from '@aztec/foundation/crypto';
import { type Fr, Point } from '@aztec/foundation/fields';
import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize';
import { BufferReader, FieldReader, serializeToBuffer } from '@aztec/foundation/serialize';

import { GeneratorIndex } from '../constants.gen.js';
import { type PublicKey } from './public_key.js';
Expand Down Expand Up @@ -107,4 +107,14 @@ export class PublicKeys {
...this.masterTaggingPublicKey.toFields(),
];
}

static fromFields(fields: Fr[] | FieldReader): PublicKeys {
const reader = FieldReader.asReader(fields);
return new PublicKeys(
reader.readObject(Point),
reader.readObject(Point),
reader.readObject(Point),
reader.readObject(Point),
);
}
}
20 changes: 12 additions & 8 deletions yarn-project/end-to-end/src/e2e_key_registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,21 +56,25 @@ describe('Key Registry', () => {

describe('failure cases', () => {
it('throws when address preimage check fails', async () => {
const publicKeysBuf = account.publicKeys.toBuffer();
// We randomly invalidate some of the keys by overwriting random byte
const byteIndex = Math.floor(Math.random() * publicKeysBuf.length);
publicKeysBuf[byteIndex] = (publicKeysBuf[byteIndex] + 2) % 256;
// First we get invalid keys by replacing any of the 8 fields of public keys with a random value
let invalidPublicKeys: PublicKeys;
{
// We call toBuffer and fromBuffer first to ensure that we get a deep copy
const publicKeysFields = PublicKeys.fromBuffer(account.publicKeys.toBuffer()).toFields();
const randomIndex = Math.floor(Math.random() * publicKeysFields.length);
publicKeysFields[randomIndex] = Fr.random();

const publicKeys = PublicKeys.fromBuffer(publicKeysBuf);
invalidPublicKeys = PublicKeys.fromFields(publicKeysFields);
}
Comment on lines +60 to +68
Copy link
Contributor

@sklppy88 sklppy88 May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just a taste thing but I would remove { } on line 61 and 68, and just have a const dec at the bottom like this. Seems to make it simpler. Either way it works.

 const publicKeysFields = PublicKeys.fromBuffer(account.publicKeys.toBuffer()).toFields();
 const randomIndex = Math.floor(Math.random() * publicKeysFields.length);
 publicKeysFields[randomIndex] = Fr.random();

 const invalidPublicKeys = PublicKeys.fromFields(publicKeysFields);

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 put there the parentheses as it informs you as a code reader that you can forget about publicKeysFields and randomIndex and only invalidPublicKeys is relevant from then on and I think it improves readability.

So thank you for your opinion ser, strongly disagree, and aping the PR to master. Have a nice day

Copy link
Contributor

Choose a reason for hiding this comment

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

🫡


await expect(
keyRegistry
.withWallet(wallets[0])
.methods.register(
account,
account.partialAddress,
// TODO(#6337): Directly dump account.publicKeys here
publicKeys.toNoirStruct(),
// TODO(#6337): Make calling `toNoirStruct()` unnecessary
invalidPublicKeys.toNoirStruct(),
)
.send()
.wait(),
Expand Down Expand Up @@ -120,7 +124,7 @@ describe('Key Registry', () => {
.methods.register(
account,
account.partialAddress,
// TODO(#6337): Directly dump account.publicKeys here
// TODO(#6337): Make calling `toNoirStruct()` unnecessary
account.publicKeys.toNoirStruct(),
)
.send()
Expand Down
Loading