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

feat(AuthWit): Query if Authwit was valid in Aztec.nr #4822

Closed
Tracked by #2199
rahul-kothari opened this issue Feb 28, 2024 · 4 comments · Fixed by #5316
Closed
Tracked by #2199

feat(AuthWit): Query if Authwit was valid in Aztec.nr #4822

rahul-kothari opened this issue Feb 28, 2024 · 4 comments · Fixed by #5316
Assignees

Comments

@rahul-kothari
Copy link
Contributor

rahul-kothari commented Feb 28, 2024

From the offsite notes:

Difficult to “query” if something was valid

  • We wanted to query the is_valid method in an account contract from a private function
  • is_valid should return a boolean (based on the name) but it asserts it’s true
  • Related, document why is_valid returns IS_VALID_SELECTOR
  • Even after changing it to return true, we cannot query it, because it depends on get_auth_witness, which fails if there is no auth wit registered.
    • We’d need a try_get_auth_witness oracle method, and in general be able to try to do things and let the caller decide whether to assert or do something else.
    • Alternatively, we could have a try_call to a function called in an unconstrained way, so we can try calling a function and see what would happen.
    • It’s also not possible to call an unconstrained function in another contract from a private function.
@LHerskind
Copy link
Contributor

As part of #4799 cancellations was added. So the functions were renamed and a now emitting nullifiers to actually "spend" the approvals.

  • is_valid explicitly did not return a boolean to ensure that it is not by mistake executable. It is also explained in the documentation why it is returning IS_VALID selector.
  • We explicitly CANNOT rely on the caller doing something if you want to have the cancellable. If we contract must do it the nullifier is in the contract, and contracts need to support cancellations instead of the account needing it.
  • We could add a is_valid that is essentially the check but without spending it, but it seems dangerous from the POV that someone will guaranteed be using it when they really need to spend it. This is the "read without nullifying" debate all over.
  • We could have on unconstrained though. Then it can essentially return two values, valid_public and valid_private. Seems less dangerous.

For public, there is the option to lookup the value, but if you really want there will now be plenty of functions essentially doing the same but for unconstrained and public.

If the main issue was reading when it was entirely off-chain then it seems like something we could address at in js? Was it all for your own account, or for someone elses? Essentially to figure out whether we need to call a function on it to check.

If we can simulate properly, then it seems like it would be fairly easy to do? Simulate the call as if you had a different msg_sender and pass along the values.

@rahul-kothari
Copy link
Contributor Author

Also gonna add @spalladino since it was his suggestion from the dogfooding?

@spalladino
Copy link
Collaborator

@LHerskind our use case was for implementing multisig: we loop over the signers, and for each of them ask them if the current action is_valid. If at least threshold of them answer true, then we can carry on.

We ended up solving it by adding a very hacky try_call_function_unconstrained oracle call that would execute a private or unconstrained function in an unconstrained way and returned a Result to allow for catching failed assertions, so we'd first call is_valid in this way, and if we unconstrainedly knew it worked, then call it properly.

So, going back to your points, it seems that adding an unconstrained variant for is_valid should do the trick, and this way we don't risk users accidentally using the wrong function (the "read without nullifying" debate).

Also, if we're going to keep the assertion within is_valid for security reasons (which I'm fine with), then I'd push for renaming it to assert_valid instead. We should also change the is_valid_impl helper we're using in account contracts, so it just returns a boolean, and the wrapper function in account.nr takes care of the assertion.

@spalladino
Copy link
Collaborator

It is also explained in the documentation why it is returning IS_VALID selector.

As expected, no one read the docs during the hacking at the offsite. We need rustdoc for noir asap so we can have proper inline docs.

@LHerskind LHerskind changed the title Query if Authwit was valid in Aztec.nr feat(AuthWit): Query if Authwit was valid in Aztec.nr Mar 8, 2024
@LHerskind LHerskind added this to the Developer Facing Aztec.nr milestone Mar 8, 2024
@LHerskind LHerskind self-assigned this Mar 11, 2024
LHerskind added a commit that referenced this issue Mar 21, 2024
Fixes #4822.

The current solution is pretty ugly (I think).

The issues encountered is mainly that:
- There are no context so we need to pass stuff that seems quite
redundant (address of the contract itself and block number)
- Fetching witness data with oracles explodes if you try to fetch
something that does not exists.
- The implementation here is a lookup from "outside" a transaction.

The latter is the main trouble. It can be somewhat mitigated by having a
function on the typescript side of things that try doing the lookup in
its local storage first to not encounter that case, but then we need to
be able to split the check so seems a bit meh to fix it that way.

This means that the unconstrained cannot really be used nicely as what
@spalladino did at the offsite, but mainly is an off-chain thing for the
user who is unsure if something was approved and want to check it.

I'm not really sure if I like to have the implementation this way where
we are doing things in both ts and noir to get it somewhat working, but
seems like the thing that is making it the easiest to get a query going
fast.
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 a pull request may close this issue.

3 participants