feat: initial `is_valid` eip1271 style wallet + minimal test changes #1935

LHerskind · 2023-09-01T16:22:22Z

Fixes #1913. For more information see the issue.

Creates a new account contract that:
- Adds support for the is_valid function
- Uses oracle to get signatures for entrypoint and is_valid auth data

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).

PhilWindle

Should we remove the code references to Eip1271? I realise that's the inspiration behind it but it's an Ethereum reference.

iAmMichaelConnor · 2023-09-04T10:01:28Z

Should we remove the code references to Eip1271? I realise that's the inspiration behind it but it's an Ethereum reference.

Removing the EIP references would be my preference too. Is there a short, descriptive name that can replace "EIP1271"?

LHerskind · 2023-09-04T10:17:08Z

Should we remove the code references to Eip1271? I realise that's the inspiration behind it but it's an Ethereum reference.

Removing the EIP references would be my preference too. Is there a short, descriptive name that can replace "EIP1271"?

Ye, we can do that. So we have https://github.com/AztecProtocol/AZIPs repo for stuff like this. I can make an actual azip for it and then simply name it based on that instead.

spalladino · 2023-09-04T11:57:22Z

I'd vote for not using an EIP nor an AZIP identifier. We're early enough (and this would be core enough) that we should refer to it by name, and not by a made up number that makes understanding more difficult to an incoming dev (which is something that really sucks about Ethereum).

Maybe just s/Eip1271Witness/AuthWitness/g..?

LHerskind · 2023-09-04T12:30:48Z

I'd vote for not using an EIP nor an AZIP identifier. We're early enough (and this would be core enough) that we should refer to it by name, and not by a made up number that makes understanding more difficult to an incoming dev (which is something that really sucks about Ethereum).

I'm fine with using the auth witness. Will change the naming.

spalladino

Awesome work. Left a few comments to discuss.

spalladino · 2023-09-04T12:20:24Z

yarn-project/aztec-rpc/src/database/database.ts

+  /**
+   * Add a eip1271 witness to the database.
+   * @param messageHash - The message hash.
+   * @param witness - An array of field elements representing the eip1271 witness.
+   */
+  addEip1271Witness(messageHash: Fr, witness: Fr[]): Promise<void>;


I was going to make a sarcastic comment about how important it is that we have mandatory comments, but I've also been guilty of doing this kind of stuff :-P

spalladino · 2023-09-04T12:22:14Z

yarn-project/aztec-rpc/src/database/memory_db.ts

+  public getEip1271Witness(messageHash: Fr): Promise<Fr[]> {
+    return Promise.resolve(this.eip1271Witnesses[messageHash.toString()]);


What should we return if there's no value for this key? If it's undefined (what I'd go with) we need to adjust the return type here.

spalladino · 2023-09-04T12:28:50Z

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

+  async signAndAddEip1271Witness(messageHash: Buffer): Promise<void> {
+    const witness = await this.accountImpl.createEip1271Witness(messageHash);
+    await this.rpc.addEip1271Witness(Fr.fromBuffer(messageHash), witness);
+    return Promise.resolve();
+  }


Should we return the signed message as well? I'm thinking this could be used for "exporting" the 12721 auth message and sharing it with another user who embeds it in their tx.

You could use the sign for that, get the sig, then on the other side he recreate it 🤷 but I follow what you are saying, might be fine to add to retrieve directly.

spalladino · 2023-09-04T12:31:06Z

yarn-project/end-to-end/src/e2e_account_contracts.test.ts

+      const { account: a, wallet: w } = await walletSetup(
+        context.aztecRpcServer,
+        encryptionPrivateKey,
+        getAccountContract(encryptionPrivateKey),
+      );
+      account = a;
+      wallet = w;


Suggested change

const { account: a, wallet: w } = await walletSetup(

context.aztecRpcServer,

encryptionPrivateKey,

getAccountContract(encryptionPrivateKey),

);

account = a;

wallet = w;

({ account, wallet } = await walletSetup(

context.aztecRpcServer,

encryptionPrivateKey,

getAccountContract(encryptionPrivateKey),

));

So you don't need the intermediate vars

spalladino · 2023-09-04T12:32:33Z

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

+                GENERATOR_INDEX__SIGNATURE_PAYLOAD
+            )[0];
+
+            let _callStackItem0 = context.call_private_function(from, 0x29d25ca9, [message_field]);


Add a comment on what 0x29d25ca9 is

spalladino · 2023-09-04T12:33:44Z

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

+    #[aztec(private)]
+    fn entrypoint(
+        payload: pub EntrypointPayload,
+    ) {
+        let message_hash: Field = pedersen_with_separator(
+          payload.serialize(), 
+          GENERATOR_INDEX__SIGNATURE_PAYLOAD
+        )[0];
+        let eip_witness = get_eip_1271_witness(message_hash);
+        assert(recover_address(message_hash, eip_witness) == context.this_address());
+        payload.execute_calls(&mut context);
+    }


Absolutely love how this turned out

spalladino · 2023-09-04T12:36:14Z

yarn-project/aztec.js/src/account/contract/eip_1271_account_contract.ts

+ * the note encryption key, relying on a single private key for both encryption and authentication.
+ * Extended to pull verification data from the oracle instead of passed as arguments.
+ */
+export class Eip1271AccountContract implements AccountContract {


I'm curious how you found this abstraction to implement your own account contract..? Was it easy, confusing..? Anything you'd change?

Getting the abis in there was initially a pain. The copy script apparently adds _ before numbers, so that was a little funky.

It felt a bit clunky with the AuthWitnessAccountContract and then the entrypoints without the access to rpc and wallet without access to the keys, so the AuthWitnessAccountEntrypoint ended up looking a little odd as I needed the keys to sign and then passing that sig back such that the wallet could insert it into the RPC. Were thinking that throwing the rpc in there seemed the nicest way to keep it with same interface, but then the wallet bleeds into entrypoints, ended up with the separate wallet to handle the flow which don't seem like the cleanest, but needed a way to also insert into the db without a payload for the approvals etc.

spalladino · 2023-09-04T12:41:00Z

yarn-project/aztec.js/src/account/entrypoint/eip_1271_account_entrypoint.ts

+  createTxExecutionRequest(executions: FunctionCall[], _opts: CreateTxRequestOpts = {}): Promise<TxExecutionRequest> {
+    throw new Error(`Not implemented, use createTxExecutionRequestWithWitness instead`);
+  }


Not a fan of this since it breaks composability (though I'm fine with it until we further refine all this 1271 story). I'd extend the TxExecutionRequest to also contain a set of MessageWithAuthWitness (where a MessageWithAuthWitness is just key+value, ie message+witness), so that when it's run through the simulator, it picks witnesses from there and only uses the db as fallback.

This should allow us to remove the new wallet type also.

Maddiaa0

Overall looks good to me, small comment

Maddiaa0 · 2023-09-04T16:18:52Z

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

+                    to, 
+                    amount
+                ], 
+                GENERATOR_INDEX__SIGNATURE_PAYLOAD


Im not sure I like that generator indexes are exposed directly to the programmer. I feel like there should be a helper function that takes in an array of inputs with a selector or something to abstract this from them.

It feels like a certain footgun

Ideally it should not be this generator at all. Think it is better with a separate generator such that payloads and other signatures don't sit in the same domain.

spalladino · 2023-09-05T19:34:33Z

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

+        let witness = get_auth_witness(message_hash);
+        assert(recover_address(message_hash, witness) == context.this_address());


I'm guessing you cannot call is_valid directly to be inlined because it's already an external function, so it is initialising its own PrivateContext, right?

Will create an inner function that they are both calling, a little clunky, but should work fine for adding a public is valid later on as well.

spalladino · 2023-09-05T19:35:00Z

yarn-project/aztec.js/src/account/entrypoint/auth_witness_account_entrypoint.ts

+  createTxExecutionRequest(executions: FunctionCall[], _opts: CreateTxRequestOpts = {}): Promise<TxExecutionRequest> {
+    throw new Error(`Not implemented, use createTxExecutionRequestWithWitness instead`);
+  }


Can you create an issue to track this, so we remove it eventually..?

Created #2043

🤖 I have created a new Aztec Packages release --- ## [0.1.0-alpha60](v0.1.0-alpha59...v0.1.0-alpha60) (2023-09-06) ### Features * Goblin recursive verifier ([#1822](#1822)) ([f962cb6](f962cb6)) * initial `is_valid` eip1271 style wallet + minimal test changes ([#1935](#1935)) ([f264c54](f264c54)) ### Bug Fixes * benchmark git repo ([#2041](#2041)) ([3c696bb](3c696bb)) * cli canary & deployment ([#2053](#2053)) ([1ddd24a](1ddd24a)) * **rpc:** Fixes getNodeInfo serialisation ([#1991](#1991)) ([0a29fa8](0a29fa8)) ### Miscellaneous * **circuits:** - use msgpack for cbind routines of native private kernel circuits ([#1938](#1938)) ([3dc5c07](3dc5c07)) * **docs:** API docs stucture ([#2014](#2014)) ([9aab9dd](9aab9dd)) * Update function selector computation ([#2001](#2001)) ([e07ea1a](e07ea1a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Transport #1935 from monorepo. Respond go Luke's PR review: * Remove patch artifacts.; Delete unneeded sumcheck_types folder.; Note memory inefficiency of folding.; intebers ~> integers; Rename BarycentricData tests.; Fix bad find&replace in test names.; Removed unneeded include.; Git rid of UnivariateClass hack.; Move BarycentricData out of relations.; Restore health space between blocks.; Add all univariates to transcript.; Fix typo.; Revised comment around polynomial_cache.; Initialize target_total_sum.; edge_extensions ~> extended_edges; Fr ~> FF; Add comments to two sumcheck_round functions.; Fixed typo.

LHerskind force-pushed the lh/1913-eip1271 branch from ce23746 to f4e3715 Compare September 2, 2023 13:35

ludamad force-pushed the master branch from e2c543c to 0036e07 Compare September 3, 2023 22:02

LHerskind marked this pull request as ready for review September 4, 2023 09:15

LHerskind requested review from Maddiaa0, spalladino and iAmMichaelConnor September 4, 2023 09:16

PhilWindle reviewed Sep 4, 2023

View reviewed changes

spalladino reviewed Sep 4, 2023

View reviewed changes

LHerskind force-pushed the lh/1913-eip1271 branch from ace2841 to 685a5ca Compare September 4, 2023 14:30

Maddiaa0 reviewed Sep 4, 2023

View reviewed changes

LHerskind force-pushed the lh/1913-eip1271 branch from 685a5ca to 84c7a0e Compare September 5, 2023 14:48

LHerskind requested a review from spalladino September 5, 2023 15:21

spalladino approved these changes Sep 5, 2023

View reviewed changes

LHerskind added 10 commits September 6, 2023 07:23

feat: initial is_valid eip1271 style wallet + minimal test changes

2ea84f8

chore: formatting 🧹

e0776a0

chore: rebase + minor fix in e2e_account_contracts

73befb9

feat: use eip-witness for entrypoint

f59ceed

test: fix issue in e2e_account_contracts

935d63e

chore: comments cleanup

3d743d7

chore: USe auth witness instead of eip1271

cd52ab8

chore: exterminate eip-1271 survivor

c3097b0

chore: recompile noir

cec9292

fix: add inner function for re-use

a4ebc27

LHerskind force-pushed the lh/1913-eip1271 branch from 5d9dda5 to a4ebc27 Compare September 6, 2023 07:37

chore: bootstrapping

6764008

LHerskind enabled auto-merge (squash) September 6, 2023 08:22

chore: remove error message to align with other entry-points

5311040

LHerskind merged commit f264c54 into master Sep 6, 2023
2 checks passed

LHerskind deleted the lh/1913-eip1271 branch September 6, 2023 09:16

github-actions bot mentioned this pull request Sep 6, 2023

chore(master): release 0.1.0-alpha60 #2040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial `is_valid` eip1271 style wallet + minimal test changes #1935

feat: initial `is_valid` eip1271 style wallet + minimal test changes #1935

LHerskind commented Sep 1, 2023 •

edited

Loading

PhilWindle left a comment •

edited

Loading

iAmMichaelConnor commented Sep 4, 2023

LHerskind commented Sep 4, 2023

spalladino commented Sep 4, 2023 •

edited

Loading

LHerskind commented Sep 4, 2023

spalladino left a comment

spalladino Sep 4, 2023

spalladino Sep 4, 2023

spalladino Sep 4, 2023

LHerskind Sep 4, 2023

spalladino Sep 4, 2023

spalladino Sep 4, 2023

spalladino Sep 4, 2023

spalladino Sep 4, 2023

LHerskind Sep 4, 2023

spalladino Sep 4, 2023

Maddiaa0 left a comment

Maddiaa0 Sep 4, 2023

LHerskind Sep 5, 2023

spalladino Sep 5, 2023

LHerskind Sep 6, 2023

spalladino Sep 5, 2023

LHerskind Sep 6, 2023

		public getEip1271Witness(messageHash: Fr): Promise<Fr[]> {
		return Promise.resolve(this.eip1271Witnesses[messageHash.toString()]);

		let witness = get_auth_witness(message_hash);
		assert(recover_address(message_hash, witness) == context.this_address());

feat: initial is_valid eip1271 style wallet + minimal test changes #1935

feat: initial is_valid eip1271 style wallet + minimal test changes #1935

Conversation

LHerskind commented Sep 1, 2023 • edited Loading

Checklist:

PhilWindle left a comment • edited Loading

Choose a reason for hiding this comment

iAmMichaelConnor commented Sep 4, 2023

LHerskind commented Sep 4, 2023

spalladino commented Sep 4, 2023 • edited Loading

LHerskind commented Sep 4, 2023

spalladino 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

Maddiaa0 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

feat: initial `is_valid` eip1271 style wallet + minimal test changes #1935

feat: initial `is_valid` eip1271 style wallet + minimal test changes #1935

LHerskind commented Sep 1, 2023 •

edited

Loading

PhilWindle left a comment •

edited

Loading

spalladino commented Sep 4, 2023 •

edited

Loading