feat(e2e): shielding and unshielding by Maddiaa0 · Pull Request #915 · AztecProtocol/aztec-packages

Maddiaa0 · 2023-06-27T03:53:06Z

Description

part of #878

re-rolls noir sha256 stuff, as loops dont harm compile time anymore!
Moves public -> private stuff into the non native token as shielding and unshielding
Moves misc test stuff in public -> private into the test contract
Creates e2e test for shielding and un shielding again

Please provide a paragraph or two giving a summary of the change, including relevant motivation and context.

Checklist:

I have reviewed my diff in github, line by line.
Every change is related to the PR description.
I have linked this pull request to the issue(s) that it resolves.
There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
The branch has been merged or rebased against the head of its merge target.
I'm happy for the PR to be merged at the reviewer's next convenience.

rahul-kothari · 2023-06-27T13:20:37Z

Btw this might help reduce some copy pasta - #916

rahul-kothari · 2023-06-28T13:48:42Z

+import { NonNativeTokenContractAbi } from '@aztec/noir-contracts/examples';
 import { DebugLogger } from '@aztec/foundation/log';
-import { pointToPublicKey, setup } from './utils.js';
+import { expectStorageSlot, pointToPublicKey, setup } from './utils.js';


could we rename this function to expectL2StorageSlotValue to make it clearer?

rahul-kothari · 2023-06-28T13:50:16Z

@@ -244,15 +244,15 @@ export function delay(ms: number): Promise<void> {
 */
 export async function calculateStorageSlot(slot: bigint, key: Fr): Promise<Fr> {


can we rename this to calculateL2StorageSlotValue

rahul-kothari · 2023-06-28T13:55:05Z

+        secretHash: Field,
+        _padding: [Field; abi::MAX_ARGS - 2]
+    ) {
+        // Create a commitment to the amount


Shouldn't we ensure that amount <= public_balances[msg.sender]?

That would leak the sender, also the amount is being minted from private, the hope is that this function would only be callable by a private function where this has been constrained in private

Aggreing with rahul, its a public function, so you are already leaking and with the current balance being in public, how would it be constrained by the private function?

Seems like it is just a mint function right now as it effectively creates a commitment you can use for minting but it don't burn anything so just inflate supply.

rahul-kothari · 2023-06-28T13:56:02Z

+        initialContext.args = initialContext.args.push_array([amount, owner.x, owner.y]);
+
+        let sender_balance = balances.at(owner.x);
+        let (mut context, (note1, note2)) = sender_balance.get_2(initialContext);


for my understanding, why do we get 2 notes?

again this is arbitrary until we can have generics and slices

rahul-kothari · 2023-06-28T13:56:31Z

+        note2.validate(owner);
+
+        let sum = note1.value + note2.value;
+        assert(sum as u64 >= amount as u64);


I saw as u120 in other noir files fwiw

Its arbitrary however i can increase it

Maddiaa0 · 2023-06-30T09:04:37Z

-        hash_bytes[321] = callerOnL1_bytes[29];
-        hash_bytes[322] = callerOnL1_bytes[30];
-        hash_bytes[323] = callerOnL1_bytes[31];
+


@iAmMichaelConnor

As above, they don't need individual loops all of them as it is the same range 🤷

Maddiaa0 · 2023-07-03T08:05:17Z

+        context = sender_balance.insert(context, change_note);
+        assert(emit_encrypted_log(inputs.call_context.storage_contract_address, sender_balance.storage_slot, change_note.owner, change_note) == 0);
+
+        let thisAddress = inputs.call_context.storage_contract_address;


rahul-kothari · 2023-07-03T14:59:26Z

 WASM_EXPORT const char* abis__test_roundtrip_serialize_signature(uint8_t const* input, uint32_t* size)
 {
-    return as_string_output<NT::ecdsa_signature>(input, size);
+    return as_string_output<NT::schnorr_signature>(input, size);


This is just a merge commit i didnt do this (no idea why though)

LHerskind · 2023-07-03T15:22:02Z

-        hash_bytes[29 + 224] = fee_bytes[29];
-        hash_bytes[30 + 224] = fee_bytes[30];
-        hash_bytes[31 + 224] = fee_bytes[31];
+        for i in 0..32 {


But they don't need each their own loop, you could do it in the same one, same range for them all.

LHerskind · 2023-07-03T15:29:50Z

+        secretHash: Field,
+        _padding: [Field; abi::MAX_ARGS - 2]
+    ) {
+        // Create a commitment to the amount


Aggreing with rahul, its a public function, so you are already leaking and with the current balance being in public, how would it be constrained by the private function?

Seems like it is just a mint function right now as it effectively creates a commitment you can use for minting but it don't burn anything so just inflate supply.

LHerskind · 2023-07-03T15:39:34Z

    it('Should be able to create a commitment from the public context', async () => {
-      const publicToPrivateAbi = PublicToPrivateContractAbi.functions.find(f => f.name === 'mintFromPublicToPrivate')!;
-      const args = encodeArguments(publicToPrivateAbi, params);
+      const shieldAbi = NonNativeTokenContractAbi.functions.find(f => f.name === 'shield')!;


As noted later, this should handle the burning to not inflate.

LHerskind · 2023-07-03T15:42:25Z

    await expectBalance(owner, mintAmount);
-  }, 60_000);
+
+    // Unshield the tokens again, sending them to the same account, however this can be any account.


Before unshielding, can you add a test that check that the factual total supply is not inflated. Supply (private + public) should not increase.

await expectAztecStorageSlot(logger, aztecNode, contract, publicBalancesSlot, owner.toField(), 0n);

LHerskind · 2023-07-03T15:46:26Z

-  const balancesStorageSlot = new Fr(slot); // this value is manually set in the Noir contract
-  const mappingStorageSlot = new Fr(4n); // The pedersen domain separator for storage slot calculations.
+  const mappingStorageSlot = new Fr(slot); // this value is manually set in the Noir contract
+  const mappingStorageSlotSeparator = new Fr(4n); // The pedersen domain separator for storage slot calculations.


Do you know if this is a constant somewhere so we don't need a 4n to hold it together? 👀

LHerskind · 2023-07-03T16:12:37Z

+    const [owner] = accounts;

-    const deployedContract = await deployContract();
+    await deployContract();


Before shielding, can you add a check to see that the supply matches the expected. Also should probably mint something, it seems strange that you can shield something, when there is nothing to shield.

LHerskind · 2023-07-03T16:27:12Z

-        hash_bytes[321] = callerOnL1_bytes[29];
-        hash_bytes[322] = callerOnL1_bytes[30];
-        hash_bytes[323] = callerOnL1_bytes[31];
+


As above, they don't need individual loops all of them as it is the same range 🤷

LHerskind

For addUnshieldedBalance we need to think about access control. Right now, anyone can call it and mint tokens. We would need to have either be able to constrain that it could only be called by the contract itself, in which case msg.sender would need to be itself, but we don’t have that because it is a private function call. Think you need to insert a commitment and then spend that commitment as part of the public execution afterwards. Have similar issue in the lending for, access control is pain.

LHerskind · 2023-07-05T08:03:39Z

+        let sender_balance = public_balances.at(sender);
+        let current_sender_balance = sender_balance.read();
+
+        if (current_sender_balance as u120) > (amount as u120) {


When amount >= current_sender_balance seems like I would just create the commitment without spending any funds 😬

We should assert that current_sender_balance <= amount and then we can decrease afterwards.

ah good spot, this was lazy

LHerskind · 2023-07-05T12:21:19Z

    ASSERT_EQ(outputs.end_contract_tree_snapshot, expectedEndContractTreeSnapshot);
    ASSERT_EQ(outputs.start_contract_tree_snapshot, emptyInputs.start_contract_tree_snapshot);
-    ASSERT_FALSE(builder.failed());
+    ASSERT_FALSE(builder.failed()) << builder.failure_msgs;


@jeanmon is this your << builder.failure_msgs? There is a few of them that have sneaked into the codebase, is this on purpose?

Yes this is on purpose. This the way gtest works (or at least my understanding of it) to log additional information when a test is failing.

@LHerskind Do you see any issue with this?

Were just that it is extra print things that we don't expect to normally need, but if only when it fails, think is probably fine 👍. Thanks for clarifying

Maddiaa0 changed the title ~~feat: shielding and unshielding~~ feat(e2e): shielding and unshielding Jun 27, 2023

Maddiaa0 marked this pull request as ready for review June 27, 2023 23:54

ludamad force-pushed the master branch from 1df243e to a8da98f Compare June 28, 2023 02:39

rahul-kothari reviewed Jun 28, 2023

View reviewed changes

Maddiaa0 commented Jun 30, 2023

View reviewed changes

Maddiaa0 commented Jul 3, 2023

View reviewed changes

Maddiaa0 requested review from LHerskind and rahul-kothari July 3, 2023 14:28

rahul-kothari reviewed Jul 3, 2023

View reviewed changes

rahul-kothari approved these changes Jul 3, 2023

View reviewed changes

LHerskind requested changes Jul 3, 2023

View reviewed changes

Maddiaa0 requested a review from LHerskind July 4, 2023 17:34

LHerskind requested changes Jul 5, 2023

View reviewed changes

Maddiaa0 added 16 commits July 5, 2023 09:13

fix: reroll noir hash loops

ee8dc36

fix: use libraries refactor

80657bd

feat: add automatic formatting to build command

ec876ab

fix: merge noir refactors

3e1a7b3

fix: update test contract

6993900

fix: update zk_token

0d492f1

fix: update public private

0719269

fix: update non native token

8a936ec

refactor: move aztec-noir lib into libs folder

1ac7df5

fix: update compile all

d4882cd

feat: unshield tokens

27976ad

fix: formatting

3480ee0

fix: revert plookup change

43f4441

fix: increase timeout

6e1dcc0

merge

2b5dc7e

fix: merge conflict

cbdd11c

Maddiaa0 added 10 commits July 5, 2023 09:48

fix: review comments

023613c

fix: fmt

a76942a

fix: test contract recompile

20196b5

fix: hash code cleanup

da7fa27

fix: update public to private e2e to check key invariants

dabc818

chore(fmt): e2e

8ee2833

fmt

e2f7e17

fix: post rebase clean

daba48b

fix: fmt

629f9ea

fmt

7c7af5d

Maddiaa0 force-pushed the md/878-unshield branch from fbd03fb to 7c7af5d Compare July 5, 2023 09:57

Maddiaa0 mentioned this pull request Jul 5, 2023

feat(noir): use noir libs #968

Merged

6 tasks

Maddiaa0 changed the base branch from master to md/noir-libs July 5, 2023 10:00

Maddiaa0 added 6 commits July 5, 2023 11:07

fix

9770592

Merge branch 'master' into md/878-unshield

d49336a

fix: collapse l1 to l2 message loops

d680ef3

fix: more unrolling

47dc272

fix: note on sig

1c56fb1

Merge branch 'md/noir-libs' into md/878-unshield

772e3e0

LHerskind approved these changes Jul 5, 2023

View reviewed changes

Base automatically changed from md/noir-libs to master July 5, 2023 12:52

Maddiaa0 enabled auto-merge (squash) July 5, 2023 12:55

Maddiaa0 disabled auto-merge July 5, 2023 12:55

Merge branch 'master' into md/878-unshield

c6bdb67

Maddiaa0 enabled auto-merge (squash) July 5, 2023 13:02

Maddiaa0 merged commit c1439a3 into master Jul 5, 2023

Maddiaa0 deleted the md/878-unshield branch July 5, 2023 13:10

rahul-kothari mentioned this pull request Jul 10, 2023

feat: Shielding and unshielding token contract #878

Closed

This was referenced Jul 25, 2023

chore(aztec-sandbox): Release 0.2.0-alpha27 #1180

Closed

chore(aztec-sandbox): Release 0.2.0 #1181

Closed

		@@ -244,15 +244,15 @@ export function delay(ms: number): Promise<void> {
		*/
		export async function calculateStorageSlot(slot: bigint, key: Fr): Promise<Fr> {

Conversation

Maddiaa0 commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

rahul-kothari commented Jun 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maddiaa0 Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Maddiaa0 commented Jun 27, 2023 •

edited

Loading

Maddiaa0 Jul 3, 2023 •

edited

Loading