From ebec8ff48900b48be3fce6bc9a52bcb566c79a7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Bene=C5=A1?= Date: Wed, 10 Jul 2024 16:38:20 +0200 Subject: [PATCH] refactor: private refund cleanup (#7403) --- .../private_token_contract/src/main.nr | 17 ++++++----------- .../src/e2e_fees/private_refunds.test.ts | 17 +++++++++++------ 2 files changed, 17 insertions(+), 17 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 688b578eb64..789799211e3 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. @@ -190,22 +190,17 @@ 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)"), - [fee_payer_point.x, fee_payer_point.y, user_point.x, user_point.y] + FunctionSelector::from_signature("complete_refund((Field,Field,bool),(Field,Field,bool))"), + [ + 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 + ] ); } #[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); 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 bbbae0727de..52076a9703c 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 @@ -77,8 +77,13 @@ 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 + // 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); + + // 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!)); // TODO(#7324): The values in complete address are currently not updated after the keys are rotated so this does @@ -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(