Refactor contract interaction #2

just-mitch · 2024-06-13T19:25:05Z

Design for AztecProtocol/aztec-packages#6973

iAmMichaelConnor

Great doc! I've only skimmed it, since the last time I interacted with contracts I was using Truffle, so others will have more modern opinions.
Some Q's:

At what stage does the PXE get populated with data about the tx that the user has submitted? Is it simulate, prove, or send?
- Is some data stored at the time of submission? And some at finalisation? And some when decrypting outgoing logs (although I suppose outgoing logs only need to be processed if re-syncing from scratch?)?
With the wip block building design, there will be several stages to finalising a tx. This is just something to be aware of when designing the promise that's returned by send().
- Arrival in the tx pool
- Tx included in a block by the sequencer (a "preconfirmation") on L2 p2p network.
- Block (in which the tx was included) is submitted to L1, with validator votes (or a commitment bond) but not-yet proven.
- This submission could be reverted if there's an L1 reorg.
- This submission could also be reverted if the 51% of signatures / the bond are lying.
- Block is eventually proven and L1 verifies it as correct.
- This proof verification could be reverted if there's an L1 reorg, so proper finalisation would need to wait sufficient etc epochs for proper eth finalisation.
If a user or an app wants to store a record of all inputs to every nested function call of a tx (all args, oracle calls and responses, capsule data, (reverse capsule data?)), so that the tx can be replayed exactly in future, how can that be achieved? Is sufficient data being returned by simulate / prove / send?
At the moment, I think we're missing auth steps (because we haven't got round to thinking about them). Steps such as (pasted from a list elsewhere):
- The PXE should prevent apps from accessing other apps' secrets via oracle calls
- The PXE should check whether the code about to be run actually does belong to a particular contract address
- The PXE might need to call back to a user/wallet/dapp to ask "Hey, this contract address needs this capsule data to continue - provide it at your own risk"
- The PXE might need to convey to a dapp "Hey, I've simulated this tx and here are the details, here's what's going to be made public, and it's going to cost this much"
- The dapp says to the wallet "Please sign this mysterious message".
- The dapp says to the wallet "Please sign this mysterious transaction"
- Can this interface support these use cases?
  - How would we achieve callbacks back to the dapp?
What happens if, during simulate, the source code / bytecode for a nested function call isn't available? Can the simulate function gracefully pause to download the data, or call back to the dapp to request the data, or somehow get the data?
What happens if, during prove, the proving key / vk for a nested function call isn't available? Can the prove function gracefully pause to generate the proving key / vk from the bytecode?

iAmMichaelConnor · 2024-06-18T09:38:28Z

in-progress/6973-refactor-base-contract-interaction.md

+    tx: simulatedTx.tx,
+    publicOutput: simulatedTx.publicOutput,
+    privateOutput: simulatedTx.privateReturnValues,
+    executionResult: simulatedTx.executionResult,


What do these objects contain? Maybe they're well-known objects, but I've never been into this part of the codebase. :)

tx is our transaction object (PrivateKernelTailCircuitPublicInputs, private proof, logs, enqueued public functions)

publicOutput is

/** * Outputs of processing the public component of a transaction. */ export class PublicSimulationOutput { constructor( public encryptedLogs: EncryptedTxL2Logs, public unencryptedLogs: UnencryptedTxL2Logs, public revertReason: SimulationError | undefined, public constants: CombinedConstantData, public end: CombinedAccumulatedData, public publicReturnValues: NestedProcessReturnValues[], public gasUsed: Partial<Record<PublicKernelType, Gas>>, ) {}

The privateReturnValues are:

/** Return values of simulating a circuit. */ export type ProcessReturnValues = Fr[] | undefined; /** Return values of simulating complete callstack. */ export class NestedProcessReturnValues { values: ProcessReturnValues; nested: NestedProcessReturnValues[];

and executionResult is a bit redundant in that it includes all the stuff in tx, but also stuff we need for proving like the verification key. It may be that that is all we truly need from this.

iAmMichaelConnor · 2024-06-18T09:43:27Z

in-progress/6973-refactor-base-contract-interaction.md

+
+```ts
+
+const addPublicDeployment = (


Small nit: I'd always include "Contract" as an adjective when describing deployments, classes, instances. There are several structs in this doc without this adjective, but that could confuse users, since "deploy", "class" and "instance" are overloaded terms.

just-mitch · 2024-06-18T20:04:32Z

Thank you for the pass @iAmMichaelConnor ! Great questions.

At what stage does the PXE get populated with data about the tx that the user has submitted?

The transient auth witnesses, capsules, and contracts should be populated in the PXE during every call (simulate/prove/send), but always removed when the call finishes. This should be an in-memory store that gets initialized for each request and acts as a cache over the persistent lmdb store the pxe has, which is what we do for authwits today.

Regarding persistent storage, I think the PXE should have a configuration parameter that specifies the minimum stage in finalization reached by blocks it receives from its node, e.g. "submitted" | "proven" | "finalized".

A user will need to configure their PXE to something other than "finalized" if they want to build ahead, in which case we need a way to overwrite the persistent store.

Regarding .wait, I think we should add an option such as until which accepts one of those "block finalization stages", possibly unioned with "tx-pool" | "included"

replayed exactly

Yes the TxExecutionRequest returned by getTxExecutionRequest should allow any pxe/node with the required acir/bytecode to run the transaction.

What happens if, during simulate, the source code / bytecode for a nested function call isn't available?

Presently the PXE/node will error, and this work is independent from that. I don't see a reason why, in the future, a PXE couldn't inspect a TxExecutionRequest and try to download the contract from a source, then verify that the artifact hash matches what the user expects to run. Similarly, the node could check that a contract class has been registered and ask a peer for the requisite bytecode and verify it by recomputing the contract class ID.

gracefully pause to generate the proving key / vk

I think this too is independent from this interface, but yes, I don't see why we couldn't, similar to the above.

auth steps

Interesting! We could add some kind of oracle "trace/audit" to the simulation output to show the user the access requests of everything.

So it wouldn't be like an interactive "pause mid-simulation and ask the user for permission", it would just be a more complete "I ran the simulation and here is who asked for what". The wallet could display that information to the user after simulation but before proving/submitting, similar to the gas costs.

I think that would cover most of the points there. I believe the PXE already can/should check that the code about to be run belongs to the specified instance using information in the contract class.

LHerskind · 2024-06-21T09:54:45Z

in-progress/6973-refactor-base-contract-interaction.md

+```ts
+export interface UserAPI {
+  getTxExecutionRequest(userRequest: UserRequest): Promise<TxExecutionRequest>;
+  simulate(userRequest: UserRequest): Promise<SimulationOutput>;


Simulate might be a bit different from what you would expect to for example see for a thing you are sending, e.g., in the sense that it should be possible to specify the from without needing to actually to be able to create a valid tx for the entrypoint.

ATM, we can pass the msg_sender that is used for the entrypoint, so that would also be a nice thing to consider 🤷

Yes we can use from in the UserRequest which PXE will use as msg_sender. So I think this use case is covered?

LHerskind · 2024-06-21T09:59:32Z

in-progress/6973-refactor-base-contract-interaction.md

+  send(userRequest: UserRequest): Promise<SentTx>;
+}
+
+export type Wallet = UserAPI & AccountInterface & PXE & AccountKeyRotationInterface;


I'm not really a fan of the Wallet being a superset of the PXE. At least when we are using it is as the way you communicate with the nodes. For example some of the data heavy functions for paths etc, might not make much sense to expose all the way at the wallet, but could make sense at the PXE.

When needing to deal with some of the data behind I have connected to the node, but it seems slightly annoying to have to deal with a Wallet, a PXE and the Node to get data, where I could deal with wallet for user interaction and then PXE for more advanced access 🤷.

Similar with Wallet needing to support the account interface. For authwits for example, it can be quite useful to have the wallet provide some of the values for the createAuthwit function on the account. e.g., the chainid and version, and therefore having a different interface.

I was the culprit of having Wallet as a superset of the PXE, and I agree. I don't think it was a good decision in retrospective. But I think that changing this should be part of an effort of reviewing the PXE and Node interfaces as well, plus introducing a good auth scope API, which are unrelated to this design. For now, I'm with Mitch in keep piling stuff on top of the Wallet interface.

I agree. I'm not sure what the separation here should be.

Do you think refactoring that should be part of this design?

I'm fine with it being handled separately to this design 👍

LHerskind · 2024-06-21T10:00:55Z

in-progress/6973-refactor-base-contract-interaction.md

+);
+
+// Register the account in PXE, and wait for it to sync.
+await aliceWallet.registerAccountInPXE({ sync: true });


Looking to the future beyond a 10 block lifetime network, do we expect that people would use sync: true? Or would that potentially be taking hours? 🤷

True. This would probably be a two step:

aliceWallet.registerAccountInPXE() // later, in some high level context in the UI aliceWallet.waitForAccountSynch() // show loading spinner

LHerskind · 2024-06-21T10:06:37Z

in-progress/6973-refactor-base-contract-interaction.md

+);
+
+// Changes to the PXE (e.g. notes, nullifiers, auth wits, contract deployments, capsules) are not persisted.
+const { request: deployAliceAccountRequest } = await aliceWallet.simulate({


As mentioned earlier, something like the from might make sense in here, but it get slight more strange than for something like EVM, since we are usually going into a account contract before the calls where you really want to "fake" the msg.sender

LHerskind · 2024-06-21T10:09:30Z

in-progress/6973-refactor-base-contract-interaction.md

+    args: bananaCoinDeploymentArgs.constructorArgs,
+    deploymentOptions: {
+      registerClass: true,
+      publicDeploy: true,


I'm curious around the publicDeploy here. If I set it to false will it then not be published, and not feasible to call the public functions on it? Can I deploy public later easily?

It is easy, but the user would need to make an explicit function call to the instance deployer contract.

LHerskind · 2024-06-21T12:29:11Z

in-progress/6973-refactor-base-contract-interaction.md

+Like `simulate`, but without the gas estimation.
+
+```ts
+async read(userRequest: UserRequest): DecodedReturn | [] {


As mentioned above. Read not exactly the same as the viem read I think.

In my mind, the difference between read and simulate is deeper than just gas estimation. simulate runs the same as the real tx but skips proving. read should handle the tx differently, by skipping fee payment entirely (as the code seems to hint at).

Still, one thing I don't like about the current approach for read is that it requires the wallet to produce an authwit, so the calls can be sent with the appropriate msg.sender. I'd like to have a design where that's not needed, and we can still properly set msg.sender, and if possible support reading the result of multicalls.

Same applies for gas estimation: are we going to ask the user to sign an authwit (which may require a faceid or hardware wallet interaction) for every round of gas estimation? We need to figure out a way to bypass auth for simulation. Though, now that I think of it, may be a different problem to this one.

Indeed. We could scope it to only support static calls, or just nuke it in favor of simulate.

@spalladino I'm missing where read would require the wallet to produce an authwit. User could just use the "signerless" wallet and set from on their request?

Hmm but doesn't the private kernel init check that msg.sender on the first call of the private stack is zero?

Is the "from" value not mostly bogus atm? Since the first call is not what you are actually calling. Otherwise the first authwit at least should be possible to skip with the simulation.

LHerskind · 2024-06-21T12:34:45Z

in-progress/6973-refactor-base-contract-interaction.md

+    const functionSelector = FunctionSelector.fromNameAndParameters(abi.name, abi.parameters);
+
+    const innerHash = computeInnerAuthWitHash([Fr.ZERO, functionSelector.toField(), entrypointPackedArgs.hash]);
+    const outerHash = computeOuterAuthWitHash(


This was updated lately to be a bit more clear 🫡.

const outerHash = computeAuthWitMessageHash( { consumer: this.dappEntrypointAddress, innerHash }, { chainId: new Fr(this.chainId), version: new Fr(this.version) }, );

LHerskind · 2024-06-21T12:38:46Z

in-progress/6973-refactor-base-contract-interaction.md

+      new Fr(this.version),
+      innerHash,
+    );
+    const authWitness = await this.userAuthWitnessProvider.createAuthWit(outerHash);


This is just this.createAuthWit(), can be written as that to just make it easier to see that they are the same functions though? Think the createAuthWit might just be using a variable that is not defined actually.

The only difference between the two as the code presently exists (or at least when I wrote this) is that this.createAuthWit actually adds the result to the PXE which is not what is desired here.

LHerskind · 2024-06-21T12:39:06Z

in-progress/6973-refactor-base-contract-interaction.md

+
+
+  createAuthWit(messageHash: Fr): Promise<AuthWitness> {
+    return this.authWitnessProvider.createAuthWit(messageHash);


Here. The authWitnessProvider is undefined, it is userAuthWitnessProvider.

LHerskind · 2024-06-21T12:40:19Z

in-progress/6973-refactor-base-contract-interaction.md

+// Finds a balance that is enough to cover the gas costs of the simulation.
+// Marks as converged if the simulation did not run out of gas,
+// or if it is not possible to increase the balance further.
+export class BinarySearchGasEstimator implements GasEstimator {


Not super into how you are doing much gas estimation, so I will mostly ignore this for now.

spalladino

Thanks for taking this on Mitch! I left several comments around the doc, but I think they mostly boil down to:

Not reusing the same types with optional fields for different things
Disagree on removing the Contract abstraction
Agree on reviewing the PXE API
Agree on having a standard for "tweaking" a tx execution request as it goes through payment methods and entrypoints (though I'm not sold on the builder pattern)
Unclear on how dapps override wallet entrypoints
Clearly outlining what goes into a high-level aztec-js, a low-level set of helpers, and a wallet (or UserAPI) interface

If we agree on not killing the Contract class, I'd propose working this from higher to lower level instead, by first cleaning up the ContractInteraction, and from there deriving what are the lower-level helpers and Wallet functions we need. Happy to chat more about this on tomorrow's sync.

spalladino · 2024-06-24T16:50:22Z

in-progress/6973-refactor-base-contract-interaction.md

+  getTxExecutionRequest(userRequest: UserRequest): Promise<TxExecutionRequest>;
+  simulate(userRequest: UserRequest): Promise<SimulationOutput>;
+  read(userRequest: UserRequest): Promise<SimulationOutput>;
+  prove(userRequest: UserRequest): Promise<UserRequest>;


I expect this to be clarified later, but at first glance, it feels odd that a method returns the same thing that it accepts. I'd rather see a "ProvenUserRequest" (could have a better name for sure) as a return type.

Thinking is that the UserRequest will get filled in as you call simulate/prove. E.g. the proof is optional unless you're calling send. We could certainly create types that enforce properties to be defined if we think that would make things clearer.

spalladino · 2024-06-24T16:52:19Z

in-progress/6973-refactor-base-contract-interaction.md

+  send(userRequest: UserRequest): Promise<SentTx>;
+}
+
+export type Wallet = UserAPI & AccountInterface & PXE & AccountKeyRotationInterface;


I was the culprit of having Wallet as a superset of the PXE, and I agree. I don't think it was a good decision in retrospective. But I think that changing this should be part of an effort of reviewing the PXE and Node interfaces as well, plus introducing a good auth scope API, which are unrelated to this design. For now, I'm with Mitch in keep piling stuff on top of the Wallet interface.

spalladino · 2024-06-24T16:56:44Z

in-progress/6973-refactor-base-contract-interaction.md

+> // REFACTOR: Having a `request` method with different semantics than the ones in the other
+  // derived ContractInteractions is confusing. We should unify the flow of all ContractInteractions.


Hah, I believe I authored pretty much all of the TODOs quoted here

spalladino · 2024-06-24T16:59:11Z

in-progress/6973-refactor-base-contract-interaction.md

+// new
+export interface ArtifactAndInstance {
+  artifact: ContractArtifact;
+  instance: ContractInstanceWithAddress;


ContractInstance is ContractInstanceWithoutAddress. I think we should make ContractInstance be ContractInstanceWithAddress, and just remove the current version without an address.

spalladino · 2024-06-24T16:59:58Z

in-progress/6973-refactor-base-contract-interaction.md

+    /**
+     * Transient capsules needed for this execution.
+     */
+    public capsules: Fr[][],


This type should change when the capsules rework lands, but agree on the idea of scoping them to a specific tx

spalladino · 2024-06-24T18:18:49Z

in-progress/6973-refactor-base-contract-interaction.md

+
+#### `UserRequest` is a kitchen sink
+
+The `UserRequest` object is a bit of a kitchen sink. It might be better to have a `DeployRequest`, `CallRequest`, etc. that extends `UserRequest`.


spalladino · 2024-06-24T18:20:21Z

in-progress/6973-refactor-base-contract-interaction.md

+
+#### Just shifting the mutable subclass problem
+
+Arguably the builder + adapter pattern just shifts the "mutable subclass" problem around. I think that since the entire lifecycle of the builder is contained to the `getTxExecutionRequest` method within a single abstract class, it's not nearly as bad as the current situation.


I think we have 2 different things: the building of the TxExecutionRequest, and then the simulation and proving. I'm against using the same class for containing the output of simulation and proving, but I can be convinced of the builder pattern for building the tx exec request (though maybe we don't need it and can just use a lot of js spreads!).

spalladino · 2024-06-24T18:22:32Z

in-progress/6973-refactor-base-contract-interaction.md

+bananaCoin.methods.mint_public(this.aliceAddress, this.ALICE_INITIAL_BANANAS).send().wait()
+```
+
+I think this is a good thing. It's not clear what `mint_public` does (which is create a stateful `ContractFunctionInteraction`), and it's not clear what `send().wait()` does. It's not even clear what `at` does (which asks the PXE for the underlying instance at the provided address). It's also hard to specify gas settings and payment methods: they're presently pushed into `send`, which doesn't make sense because they're needed for the simulation.


Not sure I agree here. The contract instance abstraction is useful (viem supports it) and a lot more clear than the lower-level alternative. I think I'd push for having a lower-level API that follows most of what you described here, but then keeping the contract instances as they are today (but without mutability and using your API under the hood to remove logic from them) and market that API to end-users. It'd also make the migration much easier - I wouldn't want to be the guy that has to update every single e2e test (though I reckon that should not be a reason for picking a design).

spalladino · 2024-06-24T19:25:57Z

in-progress/6973-refactor-base-contract-interaction.md

+const aliceContractInstance = getContractInstanceFromDeployParams(
+  SchnorrAccountContract.artifact, 
+  deploymentArgs
+);


I'd rather keep this method "hidden" under a higher-level API. The AccountManager was the initial attempt to do that, but it definitely needs more work.

IMO it feels odd to have to repeat the SchnorrAccountContract.artifact here, which is known by the getSchnorrWallet helper.

Totally makes sense! This was a little effort to peel back the onion on the AccountManager, but I agree with you.

spalladino · 2024-06-24T19:28:19Z

in-progress/6973-refactor-base-contract-interaction.md

+
+```
+
+### Deploy Token


I'm fine with all the individual steps outlined here, but since most flows will require all these steps in this order without much change, I'd look into hiding them into something higher-level.

Agreed! I think we will come up with the higher-level API as we go. Perhaps that's worth calling out at the top of the design: this is an effort to introduce more functional/modular primitives that we can build better, higher-level abstractions off.

just-mitch requested review from LHerskind and PhilWindle June 13, 2024 19:25

iAmMichaelConnor reviewed Jun 18, 2024

View reviewed changes

LHerskind reviewed Jun 21, 2024

View reviewed changes

spalladino reviewed Jun 24, 2024

View reviewed changes

just-mitch mentioned this pull request Jul 3, 2024

EPIC: Refactor contract interactions AztecProtocol/aztec-packages#6973

Open

just-mitch added 9 commits August 16, 2024 18:33

start outlining idealized flows

9b0e831

continue improving design. begin implementation thoughts

4b16012

flesh out implementation for simulate with gas estimation

c75ee1f

draft design complete

f3699a0

multicall support

0f74acd

add dapp funded transactions

370edd5

add gas estimation algo

a60dca4

clarify "contract" registration and deployment

93a3422

Add concern on simulation times.

79a1c62

just-mitch force-pushed the refactor-base-contract-interaction branch from 2acf140 to 79a1c62 Compare August 16, 2024 22:34



		createAuthWit(messageHash: Fr): Promise<AuthWitness> {
		return this.authWitnessProvider.createAuthWit(messageHash);

		> // REFACTOR: Having a `request` method with different semantics than the ones in the other
		// derived ContractInteractions is confusing. We should unify the flow of all ContractInteractions.


		#### `UserRequest` is a kitchen sink

		The `UserRequest` object is a bit of a kitchen sink. It might be better to have a `DeployRequest`, `CallRequest`, etc. that extends `UserRequest`.


		#### Just shifting the mutable subclass problem

		Arguably the builder + adapter pattern just shifts the "mutable subclass" problem around. I think that since the entire lifecycle of the builder is contained to the `getTxExecutionRequest` method within a single abstract class, it's not nearly as bad as the current situation.


		```

		### Deploy Token

Refactor contract interaction #2

Are you sure you want to change the base?

Refactor contract interaction #2

Conversation

just-mitch commented Jun 13, 2024

iAmMichaelConnor 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

just-mitch commented Jun 18, 2024

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

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

Choose a reason for hiding this comment

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

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