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

docs: adding some authwit docs #2711

Merged
merged 4 commits into from
Oct 11, 2023
Merged

docs: adding some authwit docs #2711

merged 4 commits into from
Oct 11, 2023

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented Oct 5, 2023

Writing docs for AuthWit as part of #2710.

  • I'm not fully satisfied with the naming for messages/actions etc. Don't really like the message hash because we already have multiple types of these things but right not it is a little inconsistent.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@catmcgee
Copy link
Contributor

catmcgee commented Oct 6, 2023

@LHerskind hope you don't mind I pushed some grammar fixes

deactivate AC;
```

:::info **TODO**
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have the blogs now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its the ones @rahul-kothari have been working on, so partially.

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Looks good to me! I added a bunch of suggestions, but none that prevents from merging. I do think there's a pass needed for reviewing styling issues, but maybe devrel can own it?

docs/docs/concepts/foundation/accounts/authwit.md Outdated Show resolved Hide resolved

In the EVM world, this is often accomplished by having the user `approve` the protocol to transfer funds from their account, and then calling a `deposit` function on it afterwards.

```mermaid
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoa, fancy! Does our docusaurus support 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.

Yep, it is quite nice for making it easier to update the docs without leaving the markdown files.


To limit the annoyance for return-users, some front-ends will use the `approve` function with an infinite amount, which means that the user will only has to sign the `approve` transaction once, and every future `deposit` with then use some of that "allowance" to transfer funds from the user's account to the protocol's account.

This can lead to a series of issues though, eg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd argue that another issue is that you cannot know for sure how your approval will be used by the protocol. You just give them carte blanche to manage your funds, and hope that the protocol is properly designed so they are not lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you kinda still know how your funds are getting used, just where the transfer is going, but it could be lost or send it into the abyss afterwards still 🤷.


If you recall from [State model](./../state_model.md), private state is generally only known by its owner and those they have shared it with. Because it relies on secrets, private state might be "owned" by a contract, but it needs someone with knowledge of these secrets to actually spend it. You might see where this is going.

If we were to implement the `approve` with an allowance in private, you might know the allowance, but unless you also know about the individual notes that make up the user's balances, it would be of no use to you! It is private after all.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd clarify that "know about the individual notes that make up the user's balances" requires access to the user's decryption keys, to make it even more obvious

docs/docs/concepts/foundation/accounts/authwit.md Outdated Show resolved Hide resolved

// Example action that authenticates:
// defi to transfer 1000 tokens to itself on behalf of alice_account
action = H(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use the same name authentication_witness_action as above


Given the action, the developer can ask the `on_behalf_of` account contract if the action is authenticated or not.

```mermaid
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd personally remove the diagram from here to avoid duplication


### General utilities

The primary general utility is the `compute_authwit_message_hash` function which computes the action hash from its components. This is useful for when you need to generate a hash that is not for the current call, such as when you want to update a public approval state value that is later used for [authentication in public](#updating-approval-state-in-noir).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is useful for when you need to generate a hash that is not for the current call

I get the sense that generating a hash for the current call is the most common use case, right? If so, I'd introduce it first, and then present this utility later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the action is mainly one-step yes, but when doing more complex where the contract also need to approve we want to use it.


The first thing we see in the snippet above, is that if `from` is not the call we are calling the `assert_current_call_valid_authwit` function from [earlier](#private-functions). If the call is not throwing, we are all good and can continue with the transfer.

In the snippet we are constraining the `else` case such that only `nonce = 0` is supported. This is not strictly necessary, but because I can't stand dangling useless values. By making it constrained, we can limit what people guess it does, I hope.
Copy link
Collaborator

Choose a reason for hiding this comment

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

😆, but it feels odd for the "I" to suddenly pop up in the middle of the doc

docs/docs/concepts/foundation/accounts/authwit.md Outdated Show resolved Hide resolved
docs/docs/concepts/foundation/accounts/authwit.md Outdated Show resolved Hide resolved

To outline an example as mentioned earlier, let's say that we have a token that implements `AuthWit` such that transfer funds from A to B is valid if A is doing the transfer, or there is a witness that authenticates the caller to transfer funds from A's account. While this specifies the spending rules, one must also know of the notes to use them for anything. This means that a witness in itself is only half the information.

Creating the authentication action for the transfer of funds to the Defi contract would look like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Creating the authentication action for the transfer of funds to the Defi contract would look like this:
Creating the authentication action for the transfer of funds to a Defi contract would look like 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.

the was used as it was that specific defi and not just any


The main difference is that we are not setting up an allowance, but allowing the execution of a specific action. We decided on this option as the default since it is more explicit and the user can agree exactly what they are signing.

Also, most uses of the approvals are for contracts where the following interactions are called by the user themselves, so it is not a big issue that they are not as easily "transferrable" as the `permit`s.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another difference - in the public case, all "approvals" are stored in your contract as opposed to being spread out across all contracts


### Other use-cases

We don't need to limit ourselves to the `transfer` function, we can use the same scheme for any function that requires authentication. For example, for authenticating to burn or shield assets or to vote in a governance contract or perform an operation on a lending protocol.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 need next steps that link to the common patterns and the dev docs on how to actually use authwit.

Also in both the pages (dev docs, common patterns), lets link this too


Authentication Witness is a scheme for authentication actions on Aztec, so users can allow third-parties (eg protocols or other users) to execute an action on their behalf.

How it works logically is explained in the [foundational concepts](./../../../../concepts/foundation/accounts/authwit.md) but we will do a short recap here.
Copy link
Contributor

Choose a reason for hiding this comment

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

add prerequisite reading at the top in a separate section?


All of these issues have been discussed in the community for a while, and there are many proposals to solve them. However, none of them have been widely adopted - ERC20 is so commonly used and changing a standard is hard.

## In Aztec
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not want to talk about how partial addresses are used to generate this witness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on what you mean here? What would you want to be added?

LHerskind and others added 4 commits October 11, 2023 10:35
Co-authored-by: Santiago Palladino <santiago@aztecprotocol.com>
Co-authored-by: Rahul Kothari <rahul.kothari.201@gmail.com>
@LHerskind LHerskind enabled auto-merge (squash) October 11, 2023 09:35
@LHerskind LHerskind merged commit afc23f4 into master Oct 11, 2023
78 of 80 checks passed
@LHerskind LHerskind deleted the lh/authwit-docs branch October 11, 2023 10:01
@AztecBot
Copy link
Collaborator

Benchmark results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 8 txs 32 txs 128 txs
l1_rollup_calldata_size_in_bytes 45444 179588 716132
l1_rollup_calldata_gas 223020 868268 3449552
l1_rollup_execution_gas 842107 3595376 22204921
l2_block_processing_time_in_ms 1049 3998 15755
note_successful_decrypting_time_in_ms 322 1017 3755
note_trial_decrypting_time_in_ms 24 108 137
l2_block_building_time_in_ms 9183 36618 152605
l2_block_rollup_simulation_time_in_ms 6852 27343 107876
l2_block_public_tx_process_time_in_ms 2290 9140 44251

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric 10 blocks 20 blocks 30 blocks
node_history_sync_time_in_ms 31524 77086 137450
note_history_successful_decrypting_time_in_ms 4745 13061 20093
note_history_trial_decrypting_time_in_ms 146 240 268
node_database_size_in_bytes 1193949 1902581 2755204
pxe_database_size_in_bytes 54187 108338 162578

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 56.158742331288344 56577 14745
private-kernel-ordering 30.506134969325153 20137 8089
base-rollup 870 631604 810
root-rollup 38.09756097560975 4072 1097
private-kernel-inner 53.286265432098766 72288 14745
public-kernel-private-input 51.80941358024691 37359 14745
public-kernel-non-first-iteration 31.15354938271605 37401 14745
merge-rollup 1.0116279069767442 2592 873

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants