From 21bb2e4659648973484ea13ec55f9491de0fe598 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 9 Jul 2024 13:49:49 +0000 Subject: [PATCH 1/6] refactor: private refund cleanup --- .../contracts/private_token_contract/src/main.nr | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr index 688b578eb648..1d84837425f1 100644 --- a/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr @@ -190,22 +190,15 @@ contract PrivateToken { // function has access to the final transaction fee, which is needed to compute the actual refund amount. context.set_public_teardown_function( context.this_address(), - FunctionSelector::from_signature("complete_refund(Field,Field,Field,Field)"), + FunctionSelector::from_signature("complete_refund((Field,Field,bool),(Field,Field,bool))"), [fee_payer_point.x, fee_payer_point.y, user_point.x, user_point.y] ); } #[aztec(public)] #[aztec(internal)] - fn complete_refund( - fee_payer_point_x: Field, - fee_payer_point_y: Field, - user_point_x: Field, - user_point_y: Field - ) { + fn complete_refund(fee_payer_point: Point, user_point: Point) { // 1. We get the final note content hashes by calling the `complete_refund` on the note. - let fee_payer_point = Point { x: fee_payer_point_x, y: fee_payer_point_y, is_infinite: false }; - let user_point = Point { x: user_point_x, y: user_point_y, is_infinite: false }; let tx_fee = context.transaction_fee(); let (fee_payer_note_content_hash, user_note_content_hash) = TokenNote::complete_refund(fee_payer_point, user_point, tx_fee); From c3ce5487e451162846057839209f9d8df4c022fb Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 9 Jul 2024 14:16:02 +0000 Subject: [PATCH 2/6] fix --- .../contracts/private_token_contract/src/main.nr | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr index 1d84837425f1..695da575cde9 100644 --- a/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr @@ -191,7 +191,9 @@ contract PrivateToken { context.set_public_teardown_function( context.this_address(), FunctionSelector::from_signature("complete_refund((Field,Field,bool),(Field,Field,bool))"), - [fee_payer_point.x, fee_payer_point.y, user_point.x, user_point.y] + [ + fee_payer_point.x, fee_payer_point.y, fee_payer_point.is_infinite as Field, user_point.x, user_point.y, user_point.is_infinite as Field + ] ); } From d6ec79568733245ed3c312fb0544e164f01f7f57 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 9 Jul 2024 14:19:01 +0000 Subject: [PATCH 3/6] comment --- yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts b/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts index bbbae0727de7..d3ff883290e8 100644 --- a/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts @@ -78,7 +78,7 @@ describe('e2e_fees/private_refunds', () => { expect(tx.transactionFee).toBeGreaterThan(0); // 3. Now we compute the contents of the note containing the refund for Alice. The refund note value is simply - // the fee limit less the final transaction fee. The other 2 fields in the note are Alice's npk_m_hash and + // the fee limit minus the final transaction fee. The other 2 fields in the note are Alice's npk_m_hash and // the randomness. const refundNoteValue = t.gasSettings.getFeeLimit().sub(new Fr(tx.transactionFee!)); // TODO(#7324): The values in complete address are currently not updated after the keys are rotated so this does From ca848a4b0902af3e17e3385461a199f3b6e1bf73 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 9 Jul 2024 14:22:32 +0000 Subject: [PATCH 4/6] clarifying issues --- .../noir-contracts/contracts/private_token_contract/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr b/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr index 695da575cde9..789799211e34 100644 --- a/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/private_token_contract/src/main.nr @@ -174,7 +174,7 @@ contract PrivateToken { // 3. Deduct the funded amount from the user's balance - this is a maximum fee a user is willing to pay // (called fee limit in aztec spec). The difference between fee limit and the actual tx fee will be refunded // to the user in the `complete_refund(...)` function. - // TODO(#7324): using npk_m_hash here does not work with key rotation + // TODO(#7324), TODO(#7323): using npk_m_hash here is vulnerable in 2 ways described in the linked issues. storage.balances.sub(user_npk_m_hash, U128::from_integer(funded_amount)).emit(encode_and_encrypt_note_with_keys(&mut context, user_ovpk, user_ivpk)); // 4. We generate the refund points. From 05381a13f2d789fa0b2c9db7f0715d902f9c6c34 Mon Sep 17 00:00:00 2001 From: benesjan Date: Tue, 9 Jul 2024 14:39:12 +0000 Subject: [PATCH 5/6] checking logs --- .../src/e2e_fees/private_refunds.test.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts b/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts index d3ff883290e8..e6e46fadd17a 100644 --- a/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts @@ -75,9 +75,14 @@ describe('e2e_fees/private_refunds', () => { }) .wait(); + // 3. We check that randomness for Bob was correctly emitted as an unencrypted log (Bobs needs it to reconstruct his note). + const resp = await aliceWallet.getUnencryptedLogs({ txHash: tx.txHash }); + const bobRandomnessFromLog = Fr.fromBuffer(resp.logs[0].log.data); + expect(bobRandomnessFromLog).toEqual(bobRandomness); + expect(tx.transactionFee).toBeGreaterThan(0); - // 3. Now we compute the contents of the note containing the refund for Alice. The refund note value is simply + // 4. Now we compute the contents of the note containing the refund for Alice. The refund note value is simply // the fee limit minus the final transaction fee. The other 2 fields in the note are Alice's npk_m_hash and // the randomness. const refundNoteValue = t.gasSettings.getFeeLimit().sub(new Fr(tx.transactionFee!)); @@ -86,7 +91,7 @@ describe('e2e_fees/private_refunds', () => { const aliceNpkMHash = t.aliceWallet.getCompleteAddress().publicKeys.masterNullifierPublicKey.hash(); const aliceRefundNote = new Note([refundNoteValue, aliceNpkMHash, aliceRandomness]); - // 4. If the refund flow worked it should have added emitted a note hash of the note we constructed above and we + // 5. If the refund flow worked it should have added emitted a note hash of the note we constructed above and we // should be able to add the note to our PXE. Just calling `pxe.addNote(...)` is enough of a check that the note // hash was emitted because the endpoint will compute the hash and then it will try to find it in the note hash // tree. If the note hash is not found in the tree, an error is thrown. @@ -101,13 +106,13 @@ describe('e2e_fees/private_refunds', () => { ), ); - // 5. Now we reconstruct the note for the final fee payment. It should contain the transaction fee, Bob's + // 6. Now we reconstruct the note for the final fee payment. It should contain the transaction fee, Bob's // npk_m_hash (set in the paymentMethod above) and the randomness. // Note that FPC emits randomness as unencrypted log and the tx fee is publicly know so Bob is able to reconstruct // his note just from on-chain data. const bobFeeNote = new Note([new Fr(tx.transactionFee!), bobNpkMHash, bobRandomness]); - // 6. Once again we add the note to PXE which computes the note hash and checks that it is in the note hash tree. + // 7. Once again we add the note to PXE which computes the note hash and checks that it is in the note hash tree. await t.bobWallet.addNote( new ExtendedNote( bobFeeNote, @@ -119,7 +124,7 @@ describe('e2e_fees/private_refunds', () => { ), ); - // 7. At last we check that the gas balance of FPC has decreased exactly by the transaction fee ... + // 8. At last we check that the gas balance of FPC has decreased exactly by the transaction fee ... await expectMapping(t.getGasBalanceFn, [privateFPC.address], [initialFPCGasBalance - tx.transactionFee!]); // ... and that the transaction fee was correctly transferred from Alice to Bob. await expectMapping( From 25029373b9915feb319ec31d6370229f7993b38f Mon Sep 17 00:00:00 2001 From: benesjan Date: Wed, 10 Jul 2024 08:41:05 +0000 Subject: [PATCH 6/6] commit --- yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts b/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts index e6e46fadd17a..52076a9703cd 100644 --- a/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees/private_refunds.test.ts @@ -75,13 +75,13 @@ describe('e2e_fees/private_refunds', () => { }) .wait(); + expect(tx.transactionFee).toBeGreaterThan(0); + // 3. We check that randomness for Bob was correctly emitted as an unencrypted log (Bobs needs it to reconstruct his note). const resp = await aliceWallet.getUnencryptedLogs({ txHash: tx.txHash }); const bobRandomnessFromLog = Fr.fromBuffer(resp.logs[0].log.data); expect(bobRandomnessFromLog).toEqual(bobRandomness); - expect(tx.transactionFee).toBeGreaterThan(0); - // 4. Now we compute the contents of the note containing the refund for Alice. The refund note value is simply // the fee limit minus the final transaction fee. The other 2 fields in the note are Alice's npk_m_hash and // the randomness.