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(Simulate): Allow calling view on constrained functions #2665

Closed
Tracked by #5449
spalladino opened this issue Oct 3, 2023 · 6 comments
Closed
Tracked by #5449

feat(Simulate): Allow calling view on constrained functions #2665

spalladino opened this issue Oct 3, 2023 · 6 comments
Assignees
Labels
T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature).

Comments

@spalladino
Copy link
Collaborator

Because why not?

This lets an app developer expose a constrained private getter that can be used both for querying state from other contracts and for querying from off-chain by the app, rather than requiring two versions of the same function.

However, implementing this is not as easy as removing the client check. If the function called modifies state, we need to do so in a transient copy of the state, so we can discard it as soon as the call finishes.

@sirasistant
Copy link
Contributor

There are some moving parts for this:

  • This would be basically a wrapper of simulate that returns the first non-account contract return values. All changes made by the private simulator are already encapsulated inside the resulting TX, no changes to the DBs are commited on simulate!
  • The return values themselves have no form, they are an array of 4 fields. On unconstrained functions, we retain the shape of the return values because the function does not need to return the PrivateCircuitPublicInputs, so we have the shape of the return values in the ABI. For this to be useful, we'd need need some features first:
    • Update the aztec macros to expose the shape of the original return values (before changing them to the public inputs) in the ABI so it reaches the contract artifact
    • Update the return values to be a return values hash similar to the args hash, to have them be arbitrary length
    • In the previously created wrapper, unpack the return values hash and return it to the expected shape by reading the data added to the artifact

@sirasistant sirasistant self-assigned this Oct 26, 2023
@rahul-kothari
Copy link
Contributor

weird question but what is the difference between view() and simulate() - thinking if this might be confusing to the users since we have unconstrained already

@rahul-kothari
Copy link
Contributor

rahul-kothari commented Oct 31, 2023

This lets an app developer expose a constrained private getter that can be used both for querying state from other contracts and for querying from off-chain by the app, rather than requiring two versions of the same function.

@spalladino why would you need to duplicate code? A constrained method can call an unconstrained currently (like a normal fn call) right?

@spalladino
Copy link
Collaborator Author

why would you need to duplicate code? A constrained method can call an unconstrained currently (like a normal fn call) right?

The way I see it, it's because you want a constrain on the returned results, which you don't get when calling an unconstrained function. So if you have an app contract that, as part of a tx, calls an unconstrained getter in another contract, the sender can craft a proof where that getter returns pretty much anything.

@spalladino spalladino added the T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature). label Jan 5, 2024
@nventuro
Copy link
Contributor

nventuro commented Jan 24, 2024

weird question but what is the difference between view() and simulate() - thinking if this might be confusing to the users since we have unconstrained already

This is indeed being quite confusing to me as a new user - I expected to be able to call view on any function and get its return value, the same way the Ethereum eth_call RPC works.

Instead I can apparently only call simulate, which returns the very complex Tx object, and it doesn't seem like the data I want is included there (which makes sense due to the transformation into public inputs @sirasistant mentioned).

@LHerskind LHerskind changed the title Allow calling view on constrained functions feat(PXE): Allow calling view on constrained functions Mar 9, 2024
@LHerskind LHerskind self-assigned this Mar 21, 2024
@LHerskind LHerskind changed the title feat(PXE): Allow calling view on constrained functions feat(Simulate): Allow calling view on constrained functions Mar 26, 2024
Thunkar added a commit that referenced this issue Apr 4, 2024
# Goal

This PR aims to expose arbitrary types and values resulting from
contract compilation in the resulting JSON artifact, in a way that is
not tied to aztec-specific features or even smart contracts at all.

# Problem

Up until now, Noir compiled crates that used the `contract` keyword with
a specific flow, which also added additional structs and metadata to the
output such as whatever structs were marked with the `#[event]`
attribute. This coupled Noir to smart contract specific constructs,
which were propagated through the compiler (from the parser to the
actual compilation output). For
#5079 and several
other tasks that aim to reduce the mental load and improve the general
devex of our users, we ***need*** to expose several other structs that
are even more specific to aztec, which would only compromise the
generality of the compiler further.

# Proposed solution

The introduction of a new attribute `#[abi(tag)]` that can be applied to
both `structs` and `global` top-level statements, and export types (with
the current `ABIType` format) and values (with the new `ABIValue`
format) in a way that can be interpreted by components further
downstream (for example, our typescript codegen). This way, the noir
compiler doesn't know (or care) about whatever gets included in the
artifact.

The `events` contract artifact key gets replaced by:

```typescript
outputs: {
    structs: Record<string, ABIType[]>;
    globals: Record<string, ABIValue[]>;
};
```

# What this approach allows

- Removing the smart contract specific attribute `#[event]`, replacing
it by a more general `#[abi(events)]`.
- Substantial devex improvements, such as exposing storage layout, note
ids:

![image](https://github.com/AztecProtocol/aztec-packages/assets/5404052/dba1d6ca-1286-4d4d-912e-f222d3414f32)
...or even private function return values prior to macro processing for
decoding `.view` calls
#2665

---------

Co-authored-by: esau <[email protected]>
Co-authored-by: Tom French <[email protected]>
AztecBot added a commit to noir-lang/noir that referenced this issue Apr 4, 2024
# Goal

This PR aims to expose arbitrary types and values resulting from
contract compilation in the resulting JSON artifact, in a way that is
not tied to aztec-specific features or even smart contracts at all.

# Problem

Up until now, Noir compiled crates that used the `contract` keyword with
a specific flow, which also added additional structs and metadata to the
output such as whatever structs were marked with the `#[event]`
attribute. This coupled Noir to smart contract specific constructs,
which were propagated through the compiler (from the parser to the
actual compilation output). For
AztecProtocol/aztec-packages#5079 and several
other tasks that aim to reduce the mental load and improve the general
devex of our users, we ***need*** to expose several other structs that
are even more specific to aztec, which would only compromise the
generality of the compiler further.

# Proposed solution

The introduction of a new attribute `#[abi(tag)]` that can be applied to
both `structs` and `global` top-level statements, and export types (with
the current `ABIType` format) and values (with the new `ABIValue`
format) in a way that can be interpreted by components further
downstream (for example, our typescript codegen). This way, the noir
compiler doesn't know (or care) about whatever gets included in the
artifact.

The `events` contract artifact key gets replaced by:

```typescript
outputs: {
    structs: Record<string, ABIType[]>;
    globals: Record<string, ABIValue[]>;
};
```

# What this approach allows

- Removing the smart contract specific attribute `#[event]`, replacing
it by a more general `#[abi(events)]`.
- Substantial devex improvements, such as exposing storage layout, note
ids:

![image](https://github.com/AztecProtocol/aztec-packages/assets/5404052/dba1d6ca-1286-4d4d-912e-f222d3414f32)
...or even private function return values prior to macro processing for
decoding `.view` calls
AztecProtocol/aztec-packages#2665

---------

Co-authored-by: esau <[email protected]>
Co-authored-by: Tom French <[email protected]>
@LHerskind
Copy link
Contributor

Was made possible in #5432, look at #5762 for decoded return values.

nventuro added a commit that referenced this issue Apr 19, 2024
With #5762 we can
now use `simulate()` to directly get values out of contract calls (i.e.
#2665). This PR
refactors some e2e tests that relied on events to get values out and
replaces those with proper `simulate()` calls.

Co-authored-by: ludamad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature-request Type: Adding a brand new feature (not to be confused with improving an existing feature).
Projects
Archived in project
Development

No branches or pull requests

5 participants