Reassessing function types and visibility

After the latest changes in removing the internal and open keywords from Noir, we now have the following situation when it comes to function types:

  • #[aztec(private)]: Private constrained functions
  • #[aztec(public)]: Public constrained functions
  • #[aztec(internal)]: Functions only callable via an aztec function call from the same contract (used with either of the two above)
  • #[contract-library-method]: Constrained functions that are inlined
  • unconstrained (no attribute): Unconstrained query functions
  • no attribute: Defaults to a private function, where the inputs and public inputs are set manually. Should not be used.

The last three are the most confusing. We have “external” functions that do not need an attribute (unconstrained query), we have an attribute that few people know exists (contract-library-method), and the default setting of not writing anything is basically a mistake.

I propose the following changes:

  • All functions with no aztec(*) attributes act as inlined helpers (ie what was contract-library-method). This means that, by default, functions are not callable via an Aztec contract function call.
  • We introduce the #[aztec(query)] macro to signal query methods. We force that these are annotated with unconstrained so it’s clear to devs. Flagging a non-unconstrained with aztec(query) is a compilation error.

So the result would be the following, with changes from original in bold:

  • #[aztec(private)]: Private constrained functions
  • #[aztec(public)]: Public constrained functions
  • #[aztec(internal)]: Functions only callable via an aztec function call from the same contract (used with either of the two above)
  • #[aztec(query)]: Unconstrained query function (requires unconstrained)
  • unconstrained (no attribute): Unconstrained helper function that is inlined
  • no attribute: Constrained helper function that is inlined

Eventually we may be able to remove aztec(query) altogether, in favor of new simulation and calling methods. But for now, I think this setup is clearer. Thoughts?

37 Likes

Shouldn’t this one be constrained? Other than that I fully support this, the current situation is quite confusing.

24 Likes

Hah, it was a copy-pasta error. Fixed!

28 Likes

There was a recent suggestion that the #[aztec(query)] function (in your proposed wording) could be called #[aztec(offchain)]. It’s not always going to be a query (e.g. compute_note_hash_and_nullify is such a function, but it isn’t making a “query”). Perhaps this then wouldn’t need to be accompanied by unconstrained, because it might be clear that if you execute something offchain, it’s not going to be constrained by the network?


One of the criticisms with fallback functions (which relates to this discussion), is that you’d need to declare a private, public and query version. Some people raised that maybe a query function isn’t needed in such a context, and that perhaps the private (or public) function could be called by aztec.js in such a way that it’s understood to be an offchain query, and that the private (or public) function would just implicitly get executed as brillig or acir, and the return_values extracted from the PrivateCircuitPublicInputs. Tl;dr, there’s a suggestion that maybe the #[aztec(query)] functions are superfluous.
I, personally, like the explicitness and the separation, but others don’t.


Edit: On the whole, though, I like your suggestions. I didn’t even know about the contract-library-method, and so I’m happy you’re getting rid of that in favour of a much more intuitive approach.

25 Likes
  • All functions with no aztec(*) attributes act as inlined helpers (ie what was contract-library-method). This means that, by default, functions are not callable via an Aztec contract function call.

I love this. Am in favour of removing it completely

#[aztec(query)]: Unconstrained query function (requires unconstrained)

Should this be called #[aztec(view)] to keep in line with solidity and AztecJS and to minimise burden on new devs? Ofc one argument against calling them view() is unlike solidity, these methods can’t be called by other contracts.
If we stick to query(), we should update AztecJS too.

  • unconstrained (no attribute): Unconstrained helper function that is inlined

This is exactly how aztecnr helper methods (like pop_capsule() or view_notes()) work today so that’s nice and it is closer to how noir works.
Out of curiousity, is there a reason a contract developer would write such a method in their contract?

  • #[aztec(private)]: Private constrained functions
  • #[aztec(public)]: Public constrained functions

Do we allow view() on AztecJS side in such functions?


In general like the approach!

28 Likes

Like the proposed use of empty default :+1:

Should this be called #[aztec(view)] to keep in line with solidity and AztecJS and to minimise

No! A solidity view is still “constrained”, unconstrained is explicitly not. So they are not really the same.

Do we allow view() on AztecJS side in such functions?

I think we should allow calling “view” on those as well, and have the view run simulate and return the values that way :person_shrugging: . Then for some cases, you might not even need to have an explicitly “unconstrained” getter (having multiple getters of the same things because it need to be in public as well pisses me off :angry:)

Nevertheless. I think having something like a view would be nice as it is a way for you to signal that this function don’t have any state changes :eyes:. With a better way to call functions it could be used to tell you that you are to make a static_call instead of a normal call.

Related to @Mike’s comments: As long as there is a difference in how much can be returned by the functions they are also very important to have. Not sure speed-wise how quickly each is executed, but you could have that the query would simply be quicker than the other if it is always running “constrained” for simulations etc.

9 Likes

Wholeheartedly agree, familiarity would be great to have but this is a fundamentally different concept. An Aztec developer must understand what an unconstrained function is, just like Solidity developers must understand to what extent external contracts can be trusted.

@alvaro proposed how this could be done here. I discussed this again with him recently and he still thinks it would not be too hard to pull this off.

We sort of have this already in that only functions that are passed &mut context can cause side effects, including function calls, right?

14 Likes

We sort of have this already in that only functions that are passed &mut context can cause side effects, including function calls, right?

It was as a way to convey to the caller how he should call it. If you think about having something where we have nicer call syntax like contract.function(arguments) the view marker on the implementation would indicate to the macro that the underlying call should be a static_call instead of fiddling with it yourself :eyes:

Also want it enforced in the protocol so we could make sure that it is not up to the caller to decide :person_shrugging:

9 Likes

Ah yes, for external calls that makes absolute sense. I was thinking of internal functions in my example.

8 Likes

Yeah, I’m not enamoured (gotta love the English language borrowing latinisms) with query. However, naming it offchain opens another discussion: should we allow txs to call these unconstrained “query” functions? After all, if a tx can call an oracle which is unconstrained, why not call unconstrained get_something in another contract?

Big +1!

Agree, but I wanted to leave this out of the discussion for now, in order to focus on what’s more easily attainable now.

As a nitpick, I’d keep the goal of moving as much as possible out of the protocol and say we can do this check in the app contract, just like we’re now doing with internal checks. But agree in the goal of not leaving it up to the caller to decide.


All in all, it seems we are all in agreement about the changes proposed in the 1st post, with the exception of the name for aztec(query). I’ll convert this into an issue so it can be actioned:

10 Likes

I think we need a visualisation (a nice pretty graph of arrows) of what kinds of functions can make what kinds of calls to other kinds of functions, because it’s becoming quite unwieldy :slight_smile:

Having some contract call another contract’s unconstrained function is a discussion we often come back to. I have concerns that it could lead to the called contract leaking its private data to the calling contract. Maybe the pxe can be configurable to authorise access to certain calling contracts, but it feels dangerous. It might also be a mess, if at runtime, the user is called-back with auth messages “would you like to allow contract 0x34765 to access the private state of contract 0x86787, via an unconstrained ‘aztec query/offchain’ function? This is dangerous. Think carefully before granting access.”.

I can’t find the GitHub issue, where I commented a big justification for not allowing this.

9 Likes

To be fair, we’ll already need to have some sort of authorization for the dapp who’s making these “offchain” calls. So we could simply extend this permission to all txs initiated by the dapp, so if a contract in a tx calls an unconstrained function in another one, we have the PXE apply the same permission rule as if it had been the dapp making the query.

Still, this doesn’t work on every case. If the dapp initiates a tx, and the tx ends up calling an unconstrained function in a contract unexpected by the dapp (which may happen, yay composability), we need to accommodate for that. I get the impression that, from a UX perspective, this can be tackled using the simulating simulations approach: we make an initial simulated run of the tx, we show the user a dialog explaining 1) what permissions is the tx requiring and 2) what stuff is the tx accessing, and only then we execute and return control to the calling dapp.

But we’re getting derailed. I’d suggest we try picking a name for “offchain” functions that is not offchain, so we can name it independently of this decision. I don’t have good proposal though.

11 Likes