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: Update authwit computation #2651

Merged
merged 7 commits into from
Oct 4, 2023
Merged

refactor: Update authwit computation #2651

merged 7 commits into from
Oct 4, 2023

Conversation

LHerskind
Copy link
Contributor

@LHerskind LHerskind commented Oct 3, 2023

Fixes #2448

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

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.

Love this. Some nitpicks, which may be pending because this is a draft, but sharing just in case:

  • computeAuthWitHash should be in aztec.js, it's super useful for devs
  • assert_current_call_is_valid_for should probably be assert_valid_current_call_for if we want to follow the same naming as we had, or we should rename assert_valid_message_for to assert_message_is_valid_for
  • compute_message_hash could be used within the assert_* function in that same module

@LHerskind
Copy link
Contributor Author

  • compute_message_hash could be used within the assert_* function in that same module

Not sure you are right here. compute_message_hash is computing the hash of the args passed and then the other hash, so h(...,h(...)) where the assert_* ones are doing h(...). The intention is that you can use it when you don't already got the args_hash, such as when uniswap one is compute message_hsah for a different call than itself.

@rahul-kothari
Copy link
Contributor

Had a high level look - looks very very clean!

  • Agree that computeAuthWitness should be in aztec.js.
  • I don't think assert_current_call_is_valid_for is the best name as it doesn't really reveal what exactly it does. The current function name makes me think "oh so it is checking if the current call is valid?" but what does "is valid" even mean? What exactly "is valid" - the function simulation? May I suggest the name assert_auth_witness_is_valid? Something that explicitly says what we are doing here?
  • One of my other problems with the name is - on the JS side we call it "computeAuthWitness" and expose it to devs. But on noir side, we hide it away and don't use that term. Either we use "authWitness" externally in both places or don't.

@LHerskind LHerskind force-pushed the lh/authwit-refactor branch from 82c0385 to d3afb5c Compare October 4, 2023 10:47
@LHerskind LHerskind marked this pull request as ready for review October 4, 2023 11:33
Copy link
Contributor

@rahul-kothari rahul-kothari left a comment

Choose a reason for hiding this comment

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

minor nits on naming but otherwise
image

// Compute the message hash to be used by an authentication witness
fn compute_authwit_message_hash<N>(caller: AztecAddress, target: AztecAddress, selector: Field, args: [Field; N]) -> Field {
let args_hash = hash_args(args);
pedersen_with_separator([caller.address, target.address, selector, args_hash], GENERATOR_INDEX__SIGNATURE_PAYLOAD)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

previously we had a TODO here reminding us to reconsider if this is the best pederson generator. Is that not the case anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a todo in the tracking issue, have created #2676 now, and will ref that.


/**
* Compute an authentication witness message hash from a caller and a request
* H(caller, target, selector, args_hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename target -> target_contract_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.

Copy link
Contributor

Choose a reason for hiding this comment

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

hahaha why? Cos these can be than just contract addresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

caller and target are both addresses, and requests can only really be targeted to addresses so seemed unnecessary.

await ownerWallet.setPublicAuth(swapMessageHash, true).send().wait();

// 4.2 Call swap_public from user2 on behalf of owner
const withdrawReceipt = await action.send().wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is so neat!


// 4.2 Call swap_public from user2 on behalf of owner
const withdrawReceipt = await uniswapL2Contract
const action = uniswapL2Contract
Copy link
Contributor

Choose a reason for hiding this comment

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

rename action to functionCall?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have something that is a FunctionCall, the return value from the request xD so think that might be more confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You guys discuss the names of local variables in tests..? Wow, that's a thorough review indeed.

@LHerskind LHerskind force-pushed the lh/authwit-refactor branch from fc1849c to 7f66228 Compare October 4, 2023 13:31
@LHerskind LHerskind enabled auto-merge (squash) October 4, 2023 13:31
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.

The one thing I'm not sold on is moving entrypoint and account to the new authwit library. It feels odd having the entire account "base" contract within an authwit library.

I'm assuming you did this because of the dependency on IS_VALID_SELECTOR for implementing the account actions, but that seems to be the only contact point between account and authwit.

I think I'd prefer:

  • Keep everything mashed up together in aztec (granular dependencies are overrated, especially when you have a compiler that removes everything that's unused)
  • Keep account and entrypoint in aztec, and consider moving IS_VALID_SELECTOR there as well.
  • Rename the authwith package to auth

But I won't cry too much if you just leave it as it is now in the PR, so feel free to ignore this and merge.

Comment on lines -75 to -98
// if someone else is calling on swap on sender's behalf, they need to have authorisation to do so:
let selector = compute_selector(
"swap_public((Field),(Field),Field,(Field),Field,Field,Field,(Field),Field,Field,(Field),(Field),Field)"
);
let message_field = compute_message_hash([
context.msg_sender(),
context.this_address(),
selector,
sender.address,
input_asset_bridge.address,
input_amount,
output_asset_bridge.address,
nonce_for_transfer_approval,
uniswap_fee_tier,
minimum_output_amount,
recipient.address,
secret_hash_for_L1_to_l2_message,
deadline_for_L1_to_l2_message,
canceller_for_L1_to_L2_message.address,
caller_on_L1.address,
nonce_for_swap_approval,
]);
// this also emits a nullifier for the message
assert_valid_public_message_for(&mut context,sender.address,message_field);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fuck yeah


// 4.2 Call swap_public from user2 on behalf of owner
const withdrawReceipt = await uniswapL2Contract
const action = uniswapL2Contract
Copy link
Collaborator

Choose a reason for hiding this comment

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

You guys discuss the names of local variables in tests..? Wow, that's a thorough review indeed.

@LHerskind LHerskind merged commit fdbe2b2 into master Oct 4, 2023
@LHerskind LHerskind deleted the lh/authwit-refactor branch October 4, 2023 13:48
spalladino added a commit that referenced this pull request Oct 4, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.3</summary>

##
[0.8.3](aztec-packages-v0.8.2...aztec-packages-v0.8.3)
(2023-10-04)


### Bug Fixes

* Do not depend on npx for check rebuild script
([#2681](#2681))
([20ffbbc](20ffbbc))
* Remove package json properties whitelist
([#2680](#2680))
([ef499a0](ef499a0))


### Miscellaneous

* Update authwit computation
([#2651](#2651))
([fdbe2b2](fdbe2b2)),
closes
[#2448](#2448)
</details>

<details><summary>barretenberg.js: 0.8.3</summary>

##
[0.8.3](barretenberg.js-v0.8.2...barretenberg.js-v0.8.3)
(2023-10-04)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.8.3</summary>

##
[0.8.3](barretenberg-v0.8.2...barretenberg-v0.8.3)
(2023-10-04)


### Miscellaneous

* **barretenberg:** Synchronize aztec-packages versions
</details>

---
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: Santiago Palladino <santiago@aztecprotocol.com>
Co-authored-by: Charlie Lye <karl.lye@gmail.com>
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Oct 5, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.3</summary>

##
[0.8.3](AztecProtocol/aztec-packages@aztec-packages-v0.8.2...aztec-packages-v0.8.3)
(2023-10-04)


### Bug Fixes

* Do not depend on npx for check rebuild script
([#2681](AztecProtocol/aztec-packages#2681))
([20ffbbc](AztecProtocol/aztec-packages@20ffbbc))
* Remove package json properties whitelist
([#2680](AztecProtocol/aztec-packages#2680))
([ef499a0](AztecProtocol/aztec-packages@ef499a0))


### Miscellaneous

* Update authwit computation
([#2651](AztecProtocol/aztec-packages#2651))
([fdbe2b2](AztecProtocol/aztec-packages@fdbe2b2)),
closes
[#2448](AztecProtocol/aztec-packages#2448)
</details>

<details><summary>barretenberg.js: 0.8.3</summary>

##
[0.8.3](AztecProtocol/aztec-packages@barretenberg.js-v0.8.2...barretenberg.js-v0.8.3)
(2023-10-04)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.8.3</summary>

##
[0.8.3](AztecProtocol/aztec-packages@barretenberg-v0.8.2...barretenberg-v0.8.3)
(2023-10-04)


### Miscellaneous

* **barretenberg:** Synchronize aztec-packages versions
</details>

---
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: Santiago Palladino <santiago@aztecprotocol.com>
Co-authored-by: Charlie Lye <karl.lye@gmail.com>
Maddiaa0 pushed a commit that referenced this pull request Oct 6, 2023
Maddiaa0 pushed a commit that referenced this pull request Oct 6, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.8.3</summary>

##
[0.8.3](aztec-packages-v0.8.2...aztec-packages-v0.8.3)
(2023-10-04)


### Bug Fixes

* Do not depend on npx for check rebuild script
([#2681](#2681))
([20ffbbc](20ffbbc))
* Remove package json properties whitelist
([#2680](#2680))
([ef499a0](ef499a0))


### Miscellaneous

* Update authwit computation
([#2651](#2651))
([fdbe2b2](fdbe2b2)),
closes
[#2448](#2448)
</details>

<details><summary>barretenberg.js: 0.8.3</summary>

##
[0.8.3](barretenberg.js-v0.8.2...barretenberg.js-v0.8.3)
(2023-10-04)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.8.3</summary>

##
[0.8.3](barretenberg-v0.8.2...barretenberg-v0.8.3)
(2023-10-04)


### Miscellaneous

* **barretenberg:** Synchronize aztec-packages versions
</details>

---
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: Santiago Palladino <santiago@aztecprotocol.com>
Co-authored-by: Charlie Lye <karl.lye@gmail.com>
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.

Make the auth witness flow on aztec-nr less clunky
3 participants