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.

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

refactor: nuking pay_refund_with_shielded_rebate flow #9639

Merged

Conversation

benesjan
Copy link
Contributor

The pay_refund_with_shielded_rebate flow is going away as it relies on the old token shielding flow which is to be removed soon. With that it makes sense to merge the old private fpc with FPC.

@benesjan benesjan mentioned this pull request Oct 31, 2024
Copy link
Contributor Author

benesjan commented Oct 31, 2024

@benesjan benesjan added the e2e-all CI: Enables this CI job. label Oct 31, 2024
Base automatically changed from 10-31-test_cleaning_up_token_test_utils to master November 1, 2024 01:26
@benesjan benesjan force-pushed the 10-31-refactor_nuking_pay_refund_with_shielded_rebate_flow branch 2 times, most recently from a716dbc to 28e562a Compare November 7, 2024 22:25
@benesjan benesjan marked this pull request as ready for review November 7, 2024 22:26
Copy link
Contributor

github-actions bot commented Nov 7, 2024

Changes to public function bytecode sizes

Generated at commit: 2e73842957a6159c398ff00a29d79cc8a5dcba9a, compared to commit: ddba505bf4f875d2370a4555064aec6b09783818

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
FPC::constructor +96 ❌ +3.14%
FPC::public_dispatch -1,034 ✅ -11.00%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
FPC::constructor 3,152 (+96) +3.14%
FPC::public_dispatch 8,362 (-1,034) -11.00%

@benesjan benesjan force-pushed the 10-31-refactor_nuking_pay_refund_with_shielded_rebate_flow branch from 28e562a to ff23639 Compare November 8, 2024 01:59
@benesjan benesjan marked this pull request as draft November 8, 2024 02:11
@benesjan benesjan force-pushed the 10-31-refactor_nuking_pay_refund_with_shielded_rebate_flow branch from cf9e8d3 to 7c5718b Compare November 8, 2024 02:30
@benesjan benesjan marked this pull request as ready for review November 8, 2024 02:30
@@ -29,7 +29,6 @@ members = [
"contracts/parent_contract",
"contracts/pending_note_hashes_contract",
"contracts/price_feed_contract",
"contracts/private_fpc_contract",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

merged private fpc with FPC as it made sense

}

#[private]
fn fee_entrypoint_private(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this flow now uses the partial notes refund flow

@@ -675,6 +675,7 @@ contract Token {
fee_payer: AztecAddress, // Address of the entity which will receive the fee note.
user: AztecAddress, // A user for which we are setting up the fee refund.
funded_amount: Field, // The amount the user funded the fee payer with (represents fee limit).
nonce: Field, // A nonce to make authwitness unique.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this as I was getting authwit nullifier collisions (the args were the same across multiple tests).

*/
private rebateSecret = Fr.random(),
private feeRecipient: AztecAddress,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is code is from PrivateRefundPaymentMethod which was in refunds test before.

const OutrageousPublicAmountAliceDoesNotHave = BigInt(1e8);
const PrivateMintedAlicePrivateBananas = BigInt(1e15);
const outrageousPublicAmountAliceDoesNotHave = BigInt(1e8);
const privateMintedAlicePrivateBananas = BigInt(1e15);
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 was not a fan of not following camel case so I changed this.

@@ -138,6 +127,7 @@ export class FeesTest {
}

/** Adds a pending shield transparent node for the banana coin token contract to the pxe. */
// TODO(benesjan): nuke this
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will tackle this in a followup PR

@@ -244,45 +238,6 @@ export class FeesTest {
);
}

async applyTokenAndFPC() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now there is just the banana ting and not a separate unnamed token

@@ -36,39 +35,37 @@ describe('e2e_fees private_payment', () => {
await t.teardown();
});

let InitialSequencerL1Gas: bigint;
let initialSequencerL1Gas: bigint;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

once again fixed camelcase issue.

),
},
})
.wait(),
).rejects.toThrow('Tx dropped by P2P node.');
});

// TODO(#7694): Remove this test once the lacking feature in TXE is implemented.
it('insufficient funded amount is correctly handled', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was moved over from private_refunds.test.ts as that test file was nuked.

@@ -1,177 +0,0 @@
import {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nuked this as private_payments.test.ts captures it all sufficiently now.

Copy link
Contributor

@sklppy88 sklppy88 left a comment

Choose a reason for hiding this comment

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

beautiful !

@benesjan benesjan merged commit 2e13938 into master Nov 8, 2024
141 of 142 checks passed
@benesjan benesjan deleted the 10-31-refactor_nuking_pay_refund_with_shielded_rebate_flow branch November 8, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants