feat: Migrate accounts to auth witness #2281

spalladino · 2023-09-13T16:24:56Z

Changes all account contracts to use auth witnesses for authentication

Noir

Auth witnesses now have variable length.
Introduces an AccountActions module in Noir that abstracts most of the account contract requirements, leaving only is_valid to be implemented by the developer.
Removes the AccountInterface helper in Noir in favor of two assert_valid functions.

Accounts

Introduces a BaseAccountContract in ts, such that an account dev only needs to provide the abi, deploy args, and an AuthWitnessProvider that is basically a signer. Removes all the different entrypoint implementations, since we now have just a single one.
Adds a set of AuthWitnesses to a TxExecutionRequest, which is used by the simulator during that tx run, so it doesn't need to be persisted in the local db.
Renames Account to AccountManager.
Renames Entrypoint to AccountInterface.

Wallet

Adds a single createAuthWitness method to the Wallet interface that creates and registers the witness.
Adds a convenience setPublicAuth to the AccountWallet implementation for crafting the tx that sets a public auth (I think we can do better here though, it should be easy for the dev to create an action where they also include several public auths as part of a batch, but we can push that for later).
Kills EntrypointWallet and AuthWitnessEntrypointWallet in favor of a single AccountWallet.

Docs

Yes.

Fixes #2043

yarn-project/noir-contracts/src/contracts/schnorr_auth_witness_account_contract/src/main.nr

LHerskind

Looks amazing. Minor nits added.

yarn-project/acir-simulator/src/client/client_execution_context.ts

LHerskind · 2023-09-15T09:27:38Z

yarn-project/acir-simulator/src/client/private_execution.ts

@@ -73,7 +73,9 @@ export class PrivateFunctionExecution {
        return toACVMField(await this.context.packedArgsCache.pack(args.map(fromACVMField)));
      },
      getAuthWitness: async ([messageHash]) => {
-        return (await this.context.db.getAuthWitness(fromACVMField(messageHash))).map(toACVMField);
+        const witness = await this.context.getAuthWitness(fromACVMField(messageHash));


LHerskind · 2023-09-15T09:38:27Z

yarn-project/aztec-nr/aztec/src/account.nr

+  }
+
+  // TODO: See if we can remove this function. Maybe we can have the entrypoint call directly into internal_set_is_valid_storage..?
+  fn set_is_valid_storage(self, message_hash: Field, value: Field) {


Let value be a bool instead.

If the function is internal this should be fine for us to do. Should also make it pretty straightforward for us to make multiple insertions and sandwich our own tx with a set and unset (unsure if this actually save us gas, because we went from "nothing -> true -> false".

actions = [ set_is_valid_storage(A, true), execute_action(A), set_is_valid_storage(A, false), ]

nothing -> true -> false

I guess it'll depend on whether we differentiate empty from zero in public state? Are we making that distinction now?

initially 0, but I think it might get a little weird because of the larger storage tree that we are using 🤔 essentially inserting into it, and a removal would normally be to set the value to zero, but that value is still in the tree and it is different from nothing. I'm not fully sure at what point in public execution the database will actually be updated, @iAmMichaelConnor or @dbanks12 might know a bit here? Ideally the squashing should mean that there is nothing in the tx but I'm not completely sure when the first change is insertion.

LHerskind · 2023-09-15T09:38:27Z

yarn-project/aztec-nr/aztec/src/account.nr

+    let _void = private_context.call_public_function(private_context.this_address(), selector, [message_hash, value]);
+  }
+
+  fn internal_set_is_valid_storage(self, message_hash: Field, value: Field) {


Let value be a bool instead.

LHerskind · 2023-09-15T10:00:15Z

yarn-project/noir-contracts/src/contracts/ecdsa_account_contract/src/main.nr

+    }
+
+    #[aztec(private)]
+    fn set_is_valid_storage(message_hash: Field, value: Field) {


Following the note from earlier. I think we should be able to just have the internal and then use the entry-point to enter it. Would be pretty clean I think 👀. But let us look into that in a separate pr to not delay this one to run into merge hell.

LHerskind · 2023-09-15T10:24:38Z

yarn-project/aztec.js/src/account/index.ts

@@ -26,8 +26,8 @@ export function getEcdsaAccount(
  encryptionPrivateKey: GrumpkinPrivateKey,
  signingPrivateKey: Buffer,
  saltOrAddress?: Salt | CompleteAddress,
-): Account {
-  return new Account(rpc, encryptionPrivateKey, new EcdsaAccountContract(signingPrivateKey), saltOrAddress);
+): AccountManager {


Should the function name convey that it is an account manager. I think it is fine as it is, just wanted to point it out.

I had tried that change and reverted it. I agree it should be a manager, but I also like it more as it is.

LHerskind · 2023-09-15T10:26:14Z

yarn-project/aztec.js/src/account/interface.ts

+
+// docs:start:account-interface
+/** Creates authorisation witnesses. */
+export interface AuthWitnessProvider {


The name of the file pains me a little since it is unclear that there are multiple interfaces in here etc 😬 I kinda like the IName but this is just my preference really.

I wanted to avoid IName for consistency, since we're not using it across the codebase, even though it reminds me of my good old C# days. As for the name, agree, it's nasty calling something "Interface" when it's not referring to a language interface. I asked ChatGPT for ideas and it suggested Handler as an alternative, but I don't find it very clear either.

Lets leave it be for now then.

LHerskind · 2023-09-15T10:33:14Z

yarn-project/aztec.js/src/wallet/account_wallet.ts

+
+  /**
+   * Returns a function interaction to set a message hash as authorised in this account.
+   * Public calls can then consume this authorisation.


Being a pain here. But, should we not use authorize for the American spelling similar to changes made to serialize and deserialize? Not sure I did that myself actually, but for consistency 😅

LHerskind · 2023-09-15T10:36:32Z

yarn-project/noir-contracts/src/contracts/ecdsa_account_contract/src/main.nr

+    }
+
+    #[aztec(public)]
+    internal fn internal_set_is_valid_storage(message_hash: Field, value: Field) {


As mentioned earlier, with newest aztec noir version should be good to use bools for the value

LHerskind · 2023-09-15T12:49:42Z

yarn-project/aztec.js/src/account/interface.ts

+
+// docs:start:account-interface
+/** Creates authorisation witnesses. */
+export interface AuthWitnessProvider {


Lets leave it be for now then.

LHerskind · 2023-09-15T13:05:19Z

yarn-project/aztec-nr/aztec/src/account.nr

@@ -62,16 +62,6 @@ impl AccountActions {
    }
  }

-  // TODO: See if we can remove this function. Maybe we can have the entrypoint call directly into internal_set_is_valid_storage..?


🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.7.4</summary> ## [0.7.4](aztec-packages-v0.7.3...aztec-packages-v0.7.4) (2023-09-15) ### Features * Elliptic Curve Virtual Machine Circuit ([#1268](#1268)) ([f85ecd9](f85ecd9)) * Exposing nargo version via `NodeInfo` ([#2333](#2333)) ([1c2669c](1c2669c)), closes [#2332](#2332) * Migrate accounts to auth witness ([#2281](#2281)) ([91152af](91152af)), closes [#2043](#2043) ### Bug Fixes * Aztec-nr mirror url ([#2321](#2321)) ([aaf7f67](aaf7f67)) * **build:** Fixed paths on s3 deployments ([#2335](#2335)) ([38c7979](38c7979)) ### Miscellaneous * Do not format boxes with global format ([#2326](#2326)) ([2fe845f](2fe845f)) * Remove native token ([#2280](#2280)) ([4032d01](4032d01)) * Rename getAccounts to getRegisteredAccounts ([#2330](#2330)) ([c7f3776](c7f3776)) </details> <details><summary>barretenberg.js: 0.7.4</summary> ## [0.7.4](barretenberg.js-v0.7.3...barretenberg.js-v0.7.4) (2023-09-15) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.7.4</summary> ## [0.7.4](barretenberg-v0.7.3...barretenberg-v0.7.4) (2023-09-15) ### Features * Elliptic Curve Virtual Machine Circuit ([#1268](#1268)) ([f85ecd9](f85ecd9)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

spalladino commented Sep 13, 2023

View reviewed changes

yarn-project/noir-contracts/src/contracts/schnorr_auth_witness_account_contract/src/main.nr Outdated Show resolved Hide resolved

spalladino added 4 commits September 14, 2023 15:10

Extract account actions into helper struct

60d7af9

Fix build issue

3064ea6

Adapt ts side of things

f5809f4

Fix signature size

54e42ff

spalladino force-pushed the palla/auth-wits-everywhere branch from 5db62dc to 54e42ff Compare September 14, 2023 18:30

Update docs

cf3a745

spalladino force-pushed the palla/auth-wits-everywhere branch from 8814c9e to cf3a745 Compare September 14, 2023 19:41

spalladino added 4 commits September 14, 2023 17:04

Use contract_library_method for is_valid_impl

e073e79

Encode booleans in function calls

37e7d0c

Use auth helpers from lib instead of account interface in token contract

d0582e1

Fix signature size in account contract tutorial

84487ab

spalladino requested a review from LHerskind September 14, 2023 20:28

spalladino marked this pull request as ready for review September 14, 2023 20:28

spalladino added 2 commits September 14, 2023 17:57

Remove mentions to deleted contract

02352a9

Fix assertion messages in token contract test

30cc71d

LHerskind reviewed Sep 15, 2023

View reviewed changes

spalladino added 4 commits September 15, 2023 09:27

Use bools instead of fields for public auth value in nr

14f24f1

Americans won

b865da2

Americans won in docs too

4e802c6

Remove intermediate set_is_valid_storage fn

7d859bd

LHerskind approved these changes Sep 15, 2023

View reviewed changes

spalladino merged commit 91152af into master Sep 15, 2023
2 checks passed

spalladino deleted the palla/auth-wits-everywhere branch September 15, 2023 13:20

AztecBot mentioned this pull request Sep 15, 2023

chore(master): Release 0.7.4 #2320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Migrate accounts to auth witness #2281

feat: Migrate accounts to auth witness #2281

spalladino commented Sep 13, 2023 •

edited

Loading

LHerskind left a comment

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

spalladino Sep 15, 2023

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

spalladino Sep 15, 2023

LHerskind Sep 15, 2023

spalladino Sep 15, 2023 •

edited

Loading

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

LHerskind Sep 15, 2023

feat: Migrate accounts to auth witness #2281

feat: Migrate accounts to auth witness #2281

Conversation

spalladino commented Sep 13, 2023 • edited Loading

LHerskind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Sep 13, 2023 •

edited

Loading

spalladino Sep 15, 2023 •

edited

Loading