From f3a0eae3e46160c0596cd2bb9aff2bba0b183a1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 23 Apr 2024 01:10:11 +0000 Subject: [PATCH 1/3] Fix formatting --- .../end-to-end/src/e2e_blacklist_token_contract.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts b/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts index 8f1d8735bc6..4b8e9fe3633 100644 --- a/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts +++ b/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts @@ -69,6 +69,8 @@ describe('e2e_blacklist_token_contract', () => { } toCallValue() { + // We need to use lowercase identifiers as those are what the noir interface expects + // eslint-disable-next-line camelcase return { is_admin: this.isAdmin, is_minter: this.isMinter, is_blacklisted: this.isBlacklisted }; } From 78fbdfd373f367a022b341b92fcdfc3516322801 Mon Sep 17 00:00:00 2001 From: LHerskind Date: Tue, 23 Apr 2024 07:44:41 +0000 Subject: [PATCH 2/3] fix: serialization issue with roles --- .../src/types/roles.nr | 16 ++--- .../src/e2e_blacklist_token_contract.test.ts | 71 +++++++------------ 2 files changed, 33 insertions(+), 54 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr index a2fe3bd8727..5982bab9dae 100644 --- a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr +++ b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr @@ -41,14 +41,14 @@ impl ToField for UserFlags { } } -// We don't actually need to implement this trait, but the current macros system requires that all types that are -// contained by a state variable are serializable in order to determine their length. -// Once https://github.com/AztecProtocol/aztec-packages/issues/5736 is closed we'll be able to remove this (unless -// https://github.com/AztecProtocol/aztec-packages/issues/5491 is addressed first, in which case we'd remove the to/from -// field traits instead). -impl Serialize<1> for UserFlags { - fn serialize(self) -> [Field; 1] { - [self.to_field()] +// We implement this as it is used when serializing the state variable into return values +// This is very inefficient if used to store the state variable. +// We are currently "abusing" that the `to_field` is called in the `scheduled_value_change` +// where we are using this value. +impl Serialize<3> for UserFlags { + fn serialize(self) -> [Field; 3] { + [self.is_admin.to_field(), self.is_minter.to_field(), self.is_blacklisted.to_field()] + // [self.to_field()] } } diff --git a/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts b/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts index 4b8e9fe3633..a10290d717c 100644 --- a/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts +++ b/yarn-project/end-to-end/src/e2e_blacklist_token_contract.test.ts @@ -68,30 +68,11 @@ describe('e2e_blacklist_token_contract', () => { return this; } - toCallValue() { + toNoirStruct() { // We need to use lowercase identifiers as those are what the noir interface expects // eslint-disable-next-line camelcase return { is_admin: this.isAdmin, is_minter: this.isMinter, is_blacklisted: this.isBlacklisted }; } - - toReturnValue() { - // This matches the result of the serialize() function in the Noir struct - - let value = 0; - if (this.isAdmin) { - value += 1; - } - - if (this.isMinter) { - value += 2; - } - - if (this.isBlacklisted) { - value += 4; - } - - return [BigInt(value)]; - } } const addPendingShieldNoteToPXE = async (accountIndex: number, amount: bigint, secretHash: Fr, txHash: TxHash) => { @@ -121,9 +102,7 @@ describe('e2e_blacklist_token_contract', () => { await mineBlocks(DELAY); // This gets us past the block of change - expect(await asset.methods.get_roles(admin.getAddress()).simulate()).toEqual( - new Role().withAdmin().toReturnValue(), - ); + expect(await asset.methods.get_roles(admin.getAddress()).simulate()).toEqual(new Role().withAdmin().toNoirStruct()); logger.info(`Token deployed to ${asset.address}`); tokenSim = new TokenSimulator( @@ -153,59 +132,59 @@ describe('e2e_blacklist_token_contract', () => { const adminMinterRole = new Role().withAdmin().withMinter(); await asset .withWallet(admin) - .methods.update_roles(admin.getAddress(), adminMinterRole.toCallValue()) + .methods.update_roles(admin.getAddress(), adminMinterRole.toNoirStruct()) .send() .wait(); await mineBlocks(DELAY); // This gets us past the block of change - expect(await asset.methods.get_roles(admin.getAddress()).simulate()).toEqual(adminMinterRole.toReturnValue()); + expect(await asset.methods.get_roles(admin.getAddress()).simulate()).toEqual(adminMinterRole.toNoirStruct()); }); it('create a new admin', async () => { const adminRole = new Role().withAdmin(); - await asset.withWallet(admin).methods.update_roles(other.getAddress(), adminRole.toCallValue()).send().wait(); + await asset.withWallet(admin).methods.update_roles(other.getAddress(), adminRole.toNoirStruct()).send().wait(); await mineBlocks(DELAY); // This gets us past the block of change - expect(await asset.methods.get_roles(other.getAddress()).simulate()).toEqual(adminRole.toReturnValue()); + expect(await asset.methods.get_roles(other.getAddress()).simulate()).toEqual(adminRole.toNoirStruct()); }); it('revoke the new admin', async () => { const noRole = new Role(); - await asset.withWallet(admin).methods.update_roles(other.getAddress(), noRole.toCallValue()).send().wait(); + await asset.withWallet(admin).methods.update_roles(other.getAddress(), noRole.toNoirStruct()).send().wait(); await mineBlocks(DELAY); // This gets us past the block of change - expect(await asset.methods.get_roles(other.getAddress()).simulate()).toEqual(noRole.toReturnValue()); + expect(await asset.methods.get_roles(other.getAddress()).simulate()).toEqual(noRole.toNoirStruct()); }); it('blacklist account', async () => { const blacklistRole = new Role().withBlacklisted(); await asset .withWallet(admin) - .methods.update_roles(blacklisted.getAddress(), blacklistRole.toCallValue()) + .methods.update_roles(blacklisted.getAddress(), blacklistRole.toNoirStruct()) .send() .wait(); await mineBlocks(DELAY); // This gets us past the block of change - expect(await asset.methods.get_roles(blacklisted.getAddress()).simulate()).toEqual(blacklistRole.toReturnValue()); + expect(await asset.methods.get_roles(blacklisted.getAddress()).simulate()).toEqual(blacklistRole.toNoirStruct()); }); describe('failure cases', () => { it('set roles from non admin', async () => { const newRole = new Role().withAdmin().withAdmin(); await expect( - asset.withWallet(other).methods.update_roles(AztecAddress.random(), newRole.toCallValue()).prove(), - ).rejects.toThrow("Assertion failed: caller is not admin 'caller_roles.isAdmin'"); + asset.withWallet(other).methods.update_roles(AztecAddress.random(), newRole.toNoirStruct()).prove(), + ).rejects.toThrow("Assertion failed: caller is not admin 'caller_roles.is_admin'"); }); it('revoke minter from non admin', async () => { const noRole = new Role(); await expect( - asset.withWallet(other).methods.update_roles(admin.getAddress(), noRole.toCallValue()).prove(), - ).rejects.toThrow("Assertion failed: caller is not admin 'caller_roles.isAdmin'"); + asset.withWallet(other).methods.update_roles(admin.getAddress(), noRole.toNoirStruct()).prove(), + ).rejects.toThrow("Assertion failed: caller is not admin 'caller_roles.is_admin'"); }); }); }); @@ -255,7 +234,7 @@ describe('e2e_blacklist_token_contract', () => { it('mint to blacklisted entity', async () => { await expect( asset.withWallet(wallets[1]).methods.mint_public(blacklisted.getAddress(), 1n).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.is_blacklisted'"); }); }); }); @@ -327,7 +306,7 @@ describe('e2e_blacklist_token_contract', () => { it('mint and try to redeem at blacklist', async () => { await expect(asset.methods.redeem_shield(blacklisted.getAddress(), amount, secret).prove()).rejects.toThrow( - "Assertion failed: Blacklisted: Recipient '!to_roles.isBlacklisted'", + "Assertion failed: Blacklisted: Recipient '!to_roles.is_blacklisted'", ); }); }); @@ -484,13 +463,13 @@ describe('e2e_blacklist_token_contract', () => { it('transfer from a blacklisted account', async () => { await expect( asset.methods.transfer_public(blacklisted.getAddress(), wallets[0].getAddress(), 1n, 0n).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.is_blacklisted'"); }); it('transfer to a blacklisted account', async () => { await expect( asset.methods.transfer_public(wallets[0].getAddress(), blacklisted.getAddress(), 1n, 0n).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.is_blacklisted'"); }); }); }); @@ -645,13 +624,13 @@ describe('e2e_blacklist_token_contract', () => { it('transfer from a blacklisted account', async () => { await expect( asset.methods.transfer(blacklisted.getAddress(), wallets[0].getAddress(), 1n, 0).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.is_blacklisted'"); }); it('transfer to a blacklisted account', async () => { await expect( asset.methods.transfer(wallets[0].getAddress(), blacklisted.getAddress(), 1n, 0).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.is_blacklisted'"); }); }); }); @@ -772,7 +751,7 @@ describe('e2e_blacklist_token_contract', () => { it('shielding from blacklisted account', async () => { await expect( asset.withWallet(blacklisted).methods.shield(blacklisted.getAddress(), 1n, secretHash, 0).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.is_blacklisted'"); }); }); }); @@ -886,13 +865,13 @@ describe('e2e_blacklist_token_contract', () => { it('unshield from blacklisted account', async () => { await expect( asset.methods.unshield(blacklisted.getAddress(), wallets[0].getAddress(), 1n, 0).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Sender '!from_roles.is_blacklisted'"); }); it('unshield to blacklisted account', async () => { await expect( asset.methods.unshield(wallets[0].getAddress(), blacklisted.getAddress(), 1n, 0).prove(), - ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.isBlacklisted'"); + ).rejects.toThrow("Assertion failed: Blacklisted: Recipient '!to_roles.is_blacklisted'"); }); }); }); @@ -989,7 +968,7 @@ describe('e2e_blacklist_token_contract', () => { it('burn from blacklisted account', async () => { await expect(asset.methods.burn_public(blacklisted.getAddress(), 1n, 0).prove()).rejects.toThrow( - "Assertion failed: Blacklisted: Sender '!from_roles.isBlacklisted'", + "Assertion failed: Blacklisted: Sender '!from_roles.is_blacklisted'", ); }); }); @@ -1107,7 +1086,7 @@ describe('e2e_blacklist_token_contract', () => { it('burn from blacklisted account', async () => { await expect(asset.methods.burn(blacklisted.getAddress(), 1n, 0).prove()).rejects.toThrow( - "Assertion failed: Blacklisted: Sender '!from_roles.isBlacklisted'", + "Assertion failed: Blacklisted: Sender '!from_roles.is_blacklisted'", ); }); }); From 66e379e641470ce6f30b916af24fef94212c480f Mon Sep 17 00:00:00 2001 From: LHerskind Date: Tue, 23 Apr 2024 08:00:19 +0000 Subject: [PATCH 3/3] fix: deserialization issue with roles if imported --- .../token_blacklist_contract/src/types/roles.nr | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr index 5982bab9dae..4321a2a7f28 100644 --- a/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr +++ b/noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/roles.nr @@ -1,4 +1,4 @@ -use dep::aztec::protocol_types::traits::{FromField, ToField, Serialize}; +use dep::aztec::protocol_types::traits::{FromField, ToField, Serialize, Deserialize}; global ADMIN_FLAG: u64 = 1; global MINTER_FLAG: u64 = 2; @@ -52,6 +52,18 @@ impl Serialize<3> for UserFlags { } } +// We implement this as it is required for other contracts to be able to "rebuild" the variable +// after having received the serialized value as a return-value +impl Deserialize<3> for UserFlags { + fn deserialize(fields: [Field; 3]) -> Self { + Self { + is_admin: fields[0] as bool, + is_minter: fields[1] as bool, + is_blacklisted: fields[2] as bool, + } + } +} + mod test { use crate::types::roles::UserFlags;