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: fpc optimization, cleanup + docs #10555

Merged
merged 35 commits into from
Dec 18, 2024

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Dec 9, 2024

FPC code docs, overall cleanup and refactor, improved naming, ContractFunctionInteraction fix.

Note 1: Based on Nico's feedback the fee is paid in public and not in private (only the refund is paid in private as we need to conceal the identity of the user --> identity of the FPC is public so no need to waste resources on the private note).

Note 2: there is no longer a fee_recipient passed to the setup_refund function as it's no longer required --> before it was required because we needed someone to receive the fee note and the FPC could not receive notes. With the change to public we just send the fee directly to FPC.

Copy link
Contributor Author

benesjan commented Dec 9, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benesjan benesjan force-pushed the 12-09-refactor_fpc_cleanup_docs branch 4 times, most recently from 35a2868 to bc6c54b Compare December 11, 2024 00:59
@benesjan benesjan added the e2e-all CI: Enables this CI job. label Dec 11, 2024
@benesjan benesjan marked this pull request as ready for review December 11, 2024 01:18
@benesjan benesjan marked this pull request as draft December 11, 2024 01:18
Copy link
Contributor

github-actions bot commented Dec 11, 2024

Changes to public function bytecode sizes

Generated at commit: 08c6b6647bb7f2b024023f319e54711f3d28bf45, compared to commit: 970ad77966a17fd5c8071a7c3c3a405f83630c5d

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
Token::complete_refund +1,438 ❌ +26.10%
FPC::public_dispatch +404 ❌ +5.39%
Token::public_dispatch +212 ❌ +0.65%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
Token::complete_refund 6,948 (+1,438) +26.10%
FPC::public_dispatch 7,905 (+404) +5.39%
Token::public_dispatch 32,739 (+212) +0.65%

@benesjan benesjan marked this pull request as ready for review December 11, 2024 02:13
#[public]
#[internal]
fn prepare_fee(from: AztecAddress, amount: Field, asset: AztecAddress, nonce: Field) {
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 function as it was superfluous --> we could just enqueue the call to the token directly from the private function (which saves 1 kernel iteration).

#[private]
fn fee_entrypoint_public(amount: Field, asset: AztecAddress, nonce: Field) {
FPC::at(context.this_address())
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 the prepare_fee function as it was superfluous --> we could just enqueue the call to the token directly from the private function (which saves 1 kernel iteration).

&mut context,
);
#[view]
fn get_accepted_asset() -> 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.

Exposed this so that we can load this info directly from contract instead of having to pass that value in our TS API.

@@ -615,9 +615,9 @@ contract Token {

/// We need to use different randomness for the user and for the fee payer notes because if the randomness values
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unified the naming in this contract with FPC.

@@ -42,8 +41,40 @@ export class PrivateFeePaymentMethod implements FeePaymentMethod {
* The asset used to pay the fee.
* @returns The asset used to pay the fee.
*/
getAsset() {
return this.asset;
getAsset(): Promise<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.

Now obtaining the value directly from the contract --> this declutters the API.

@benesjan benesjan requested review from nventuro and Thunkar December 11, 2024 13:26
@benesjan benesjan force-pushed the 12-09-refactor_fpc_cleanup_docs branch from 7338f52 to ae343c7 Compare December 11, 2024 13:29
@benesjan benesjan marked this pull request as draft December 11, 2024 16:00
@benesjan benesjan force-pushed the 12-09-refactor_fpc_cleanup_docs branch from ae343c7 to ceabdbb Compare December 11, 2024 16:01
@benesjan benesjan marked this pull request as ready for review December 11, 2024 17:42
// since we're interested in the first set of values AFTER the account entrypoint
// For public functions we retrieve the first values directly from the public output.
const rawReturnValues =
this.functionDao.functionType == FunctionType.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 assumed that we always simulate via an account contract and if we did not it would fail with undefined. Since I did exactly that in this PR I've stumbled upon this. Note that there is a new issue that needs to be tackled.

Comment on lines 36 to 43
/// 1. `setup_refund` function subtracts the `max_fee` from user's balance of AA, prepares partial notes
/// for the `private_fee_recipient` (to obtain the payment in AA for the fee) and for the msg_sender (for refund
/// note of the AA) and sets a public teardown function (in which the partial notes will be finalized),
/// 2. then the private and public functions of a tx get executed,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does private_fee_recipient need to receive the payment privately? The amount is public, and the address is public. Can't we just send them public balance? That'd be much cheaper since there are no logs etc., and also less gates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes total sense. Will do the change 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not as part of this PR, but I do think we need an example for how someone can pay a fee privately with the following kept private:

  • The user
  • The FPC contract address
  • The asset used to pay the fee.

If there are to be many FPCs on Aztec, then this information will drastically reduce Aztec's global privacy set.

Ideally we'd have an example of a way to pay fees without leaking any of this info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue for this.

If there are to be many FPCs on Aztec, then this information will drastically reduce Aztec's global privacy set.

As we discussed in a scrum, this could be alleviated by us integrating the FPC with the token contract and allowing only for a few of those (e.g. 2 stablecoins and ETH).

@benesjan benesjan marked this pull request as draft December 16, 2024 16:27
@benesjan benesjan force-pushed the 12-09-refactor_fpc_cleanup_docs branch 2 times, most recently from 6949ccf to bd10e66 Compare December 16, 2024 18:28
@benesjan benesjan marked this pull request as ready for review December 16, 2024 18:35
@benesjan benesjan force-pushed the 12-09-refactor_fpc_cleanup_docs branch from 41c3df4 to 03946d4 Compare December 17, 2024 19:14
@benesjan benesjan merged commit e23fd0d into master Dec 18, 2024
72 checks passed
@benesjan benesjan deleted the 12-09-refactor_fpc_cleanup_docs branch December 18, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants