Skip to content

Refactor contract interaction #2

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Refactor contract interaction #2

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9b0e831
start outlining idealized flows
just-mitch Jun 10, 2024
4b16012
continue improving design. begin implementation thoughts
just-mitch Jun 11, 2024
c75ee1f
flesh out implementation for simulate with gas estimation
just-mitch Jun 12, 2024
f3699a0
draft design complete
just-mitch Jun 13, 2024
0f74acd
multicall support
just-mitch Jun 14, 2024
370edd5
add dapp funded transactions
just-mitch Jun 14, 2024
a60dca4
add gas estimation algo
just-mitch Jun 14, 2024
93a3422
clarify "contract" registration and deployment
just-mitch Jun 18, 2024
79a1c62
Add concern on simulation times.
just-mitch Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
multicall support
  • Loading branch information
@just-mitch
just-mitch committed Aug 16, 2024
commit 0f74acd98f2025eb86d2a15a1063b46e23cef263
220 changes: 124 additions & 96 deletions in-progress/6973-refactor-base-contract-interaction.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ This is a refactor of the API for interacting with contracts to improve the user

The refactored approach mimics Viem's API, with some enhancements and modifications to fit our needs.

In a nutshell, by being more verbose in the API, we can remove a lot of complexity and make the code easier to understand and maintain; this also affords greater understanding and control over the lifecycle of their contracts and transactions.
In a nutshell, by being more verbose in the API, we can remove a lot of complexity and make the code easier to understand and maintain; this also affords greater understanding and control over the lifecycle of contracts and transactions.

Key changes:
- the wallet is the central point of interaction to simulate/prove/send transactions instead of `BaseContractInteraction`
Expand Down Expand Up @@ -67,10 +67,13 @@ const paymentMethod = new SomeFeePaymentMethod(

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

Choose a reason for hiding this comment

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

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

artifact: SchnorrAccountContract.artifact,
instance: aliceContractInstance,
functionName: deploymentArgs.constructorName,
args: deploymentArgs.constructorArgs,
// easy multicall support
calls: [{
artifact: SchnorrAccountContract.artifact,
instance: aliceContractInstance,
functionName: deploymentArgs.constructorName,
args: deploymentArgs.constructorArgs,
}],
paymentMethod,
// gasSettings: undefined => automatic gas estimation. the returned `request` will have the gasSettings set.
});
Expand Down Expand Up @@ -112,14 +115,16 @@ const bananaCoinInstance = getContractInstanceFromDeployParams(
);

const { request: deployTokenRequest } = await aliceWallet.simulate({
artifact: TokenContract.artifact,
instance: bananaCoinInstance,
functionName: bananaCoinDeploymentArgs.constructorName,
args: bananaCoinDeploymentArgs.constructorArgs,
deploymentOptions: {
registerClass: true,
publicDeploy: true,
},
calls: [{
artifact: TokenContract.artifact,
instance: bananaCoinInstance,
functionName: bananaCoinDeploymentArgs.constructorName,
args: bananaCoinDeploymentArgs.constructorArgs,
deploymentOptions: {
Copy link
Contributor

Choose a reason for hiding this comment

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

When having the deployment options we know behind the scenes that it is deployment? Or should we not be calling the deployer instead with the banana things as arguments?

Something that I don't think got much more clear here is when the calls are going through account contract or into a deployer or is directly the call you are outlining, so it is more verbose, but I'm not sure if it give much more clarity really 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it makes sense to keep the constructor as the "main function" (i.e. the one specified by functionName) since it is no different from another function call.

The deploymentOptions are meant to provide syntactic sugar for "add ons" that add function calls to our AppPayload for our account contract entrypoint. However, I had had this option before I converted calls to take an array, so I can be easily persuaded to drop deploymentOptions in favor of just having the explicit calls:

const tokenContractClass = getContractClassFromArtifact(TokenContract.artifact);
aliceWallet.simulate({
  calls: [{
    artifact: TokenContract.artifact,
    instance: bananaCoinInstance,
    functionName: bananaCoinDeploymentArgs.constructorName,
    args: bananaCoinDeploymentArgs.constructorArgs
  }, {
    ...getCanonicalClassRegisterer(),
    functionName: 'register',
    // would need to add `capsules` to UserFunctionRequest`, but that's probably good.
    capsules: [bufferAsFields(tokenContractClass.packedBytecode)],
    args: {
      artifact_hash: tokenContractClass.artifactHash,
      private_functions_root: tokenContractClass.privateFunctionsRoot,
      public_bytecode_commitment: tokenContractClass.publicBytecodeCommitment
    }
  }, {
    ...getCanonicalInstanceDeployer(),
    functionName: 'deploy',
    args: {
        salt: bananaCoinInstance.salt,
        contract_class_id: bananaCoinInstance.contractClassId,
        initialization_hash: bananaCoinInstance.initializationHash,
        public_keys_hash: bananaCoinInstance.publicKeysHash,
        universal_deploy: bananaCoinInstance.deployer.isZero(),
    }
  }]
})

This would also make answering your question below about delayed public deployments easy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not having the options seems fairly bloaty, my main confusion here was around wanting to be more explicit, and then doing things implicit for deployments anyway. In my mind deployments are a bit different from most usual contracts, but I get why having the same flow is useful.

registerClass: true,
publicDeploy: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

},
}],
paymentMethod
})

Expand All @@ -133,9 +138,11 @@ const receipt = await sentTx.wait()

```ts
const { result: privateBalance } = await aliceWallet.read({
contractInstance: bananaCoinInstance,
functionName: 'balance_of_private'
args: {owner: aliceWallet.getAddress()},
calls: [{
contractInstance: bananaCoinInstance,
functionName: 'balance_of_private'
args: {owner: aliceWallet.getAddress()}
}]
});


Expand Down Expand Up @@ -276,16 +283,20 @@ export interface DeploymentOptions {
publicDeploy?: boolean;
}

// new
export interface UserRequest {
export interface UserFunctionCall {
contractInstance: ContractInstanceWithAddress;
functionName: string;
args: any;
deploymentOptions?: DeploymentOptions;
gasSettings?: GasSettings;
paymentMethod?: FeePaymentMethod;
contractArtifact?: ContractArtifact;
functionAbi?: FunctionAbi;
}

// new
export interface UserRequest {
calls: UserFunctionCall[];
gasSettings?: GasSettings;
paymentMethod?: FeePaymentMethod;
from?: AztecAddress;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected use of from? For simulation it is useful to when mocking a caller, but how should it be used otherwise? If one "Wallet" have multiple keys I guess I can see it being used to figure out what contract you are to use to wrap it, but otherwise seems like a vuln or bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, exactly, it is to be used just for simulation. It gets passed through to PXE as msgSender:

  public async simulateTx(
    txRequest: TxExecutionRequest,
    simulatePublic: boolean,
    msgSender: AztecAddress | undefined = undefined,
  ): Promise<SimulatedTx> { ... }

Copy link
Contributor

Choose a reason for hiding this comment

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

In the current flow, the from is fairly useless in the simulations since it is just applying to the very first call, e.g., the call to the account contract where you don't even use the msg_sender. So we will need to deal with that separately.

simulatePublicFunctions?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick thought. Is it actually useful to have this option? In my mind, you either simulate the full thing and not at all. I get we need some simulation for proving still, but I guess I would remove the option entirely and then have them rely on using simulate to simulate instead of the proving as well 🤷. So if you wanna know you do simulate + prove, but if you just try to blast as quickly as possible you will just do prove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I agree. It is in there because it was there already. If no one has strong opinions otherwise, I'd say we nuke it.

executionResult?: ExecutionResult; // the raw output of a simulation that can be proven
Expand Down Expand Up @@ -371,14 +382,16 @@ Consider that we have, e.g.:

```ts
{
contractInstance: bananaCoinInstance,
functionName: 'transfer',
args: {
from: aliceAddress,
to: bobAddress,
value: privateBalance,
nonce: 0n
},
calls: [{
Copy link
Contributor

Choose a reason for hiding this comment

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

While this interface might make it more clear, for cases where it is not a multi-call it seems worse to me, seems more tricky to get nice autocomplete for and early compiler error or type errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to first define where we want to have type-safe args. IMO it'd be ok to have lower-level APIs, not meant to be used normally by end-users, where we don't have typed args.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't come up with a way to support multi-call and single call without accepting an array here or doubling the size of the UserAPI to have, e.g. simulateMultiCall.

I'm kind of thinking that the typescript that gets generated for each contract can actually still help us a lot, and provide autocomplete etc, but they will just return these (much simpler) UserFunctionRequests. Then the fact that calls is an array is maybe not so bad.

E.g.

{
  calls: [bananaCoinInstance.transfer({ 
      from: aliceAddress,
      to: bobAddress,
      value: privateBalance,
      nonce: 0n
    })]
}

My hope with this design is to just some solid low-level primitives that we can build more ergonomics on top of after they're a bit more battle tested. So I don't mind forgoing autocomplete in the immediate term if we think we can extend this (or some other) design to support it.

contractInstance: bananaCoinInstance,
functionName: 'transfer',
args: {
from: aliceAddress,
to: bobAddress,
value: privateBalance,
nonce: 0n
},
}],
paymentMethod
}
```
Expand Down Expand Up @@ -417,40 +430,60 @@ function makeFunctionCall(

```

#### main function calls
Copy link
Contributor

Choose a reason for hiding this comment

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

The "main" part is a little weird to me. If you are developing, I would just see it as adding a function call not a main one, what is the mental model difference? To distinguish between inner calls being made? But a user would not be doing those anyway 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it's to distinguish between things that would be added by, say, deployment/registration. But if we ditch those options as suggested above then "main" is just the function you're calling from the entrypoint (app payload).


Define a helper somewhere as:

```ts
const addMainFunctionCall: TxExecutionRequestAdapter = (
builder: TxExecutionRequestBuilder, call: UserFunctionCall
) => {
if (!call.functionAbi) {
throw new Error('Function ABI must be provided');
}
Comment on lines +442 to +444
Copy link
Contributor

Choose a reason for hiding this comment

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

-1 on relying on runtime checks, as opposed to stronger types

builder.addAppFunctionCall(
makeFunctionCall(
call.functionAbi,
call.contractInstance.address,
call.args
));
}
```

#### class registration

Define a helper somewhere as:

```ts
export const addClassRegistration: TxExecutionRequestAdapter = (
builder: TxExecutionRequestBuilder, request: UserRequest
const addClassRegistration = (
builder: TxExecutionRequestBuilder, call: UserFunctionCall
) => {
if (!request.contractArtifact) {
throw new Error('Contract artifact must be provided to register class');
}
if (!call.contractArtifact) {
throw new Error('Contract artifact must be provided to register class');
}

const contractClass = getContractClassFromArtifact(request.contractArtifact);
const contractClass = getContractClassFromArtifact(call.contractArtifact);

builder.addCapsule(
bufferAsFields(
contractClass.packedBytecode,
MAX_PACKED_PUBLIC_BYTECODE_SIZE_IN_FIELDS
));
builder.addCapsule(
bufferAsFields(
contractClass.packedBytecode,
MAX_PACKED_PUBLIC_BYTECODE_SIZE_IN_FIELDS
));

const { artifact, instance } = getCanonicalClassRegisterer();
const { artifact, instance } = getCanonicalClassRegisterer();

const registerFnAbi = findFunctionAbi(artifact, 'register');
const registerFnAbi = findFunctionAbi(artifact, 'register');

builder.addAppFunctionCall(
makeFunctionCall(
registerFnAbi,
instance.address,
{
artifact_hash: contractClass.artifactHash,
private_functions_root: contractClass.privateFunctionsRoot,
public_bytecode_commitment: contractClass.publicBytecodeCommitment
}
));
builder.addAppFunctionCall(
makeFunctionCall(
registerFnAbi,
instance.address,
{
artifact_hash: contractClass.artifactHash,
private_functions_root: contractClass.privateFunctionsRoot,
public_bytecode_commitment: contractClass.publicBytecodeCommitment
}
));
}
```

Expand All @@ -460,8 +493,8 @@ Define a helper somewhere as

```ts

export const addPublicDeployment: TxExecutionRequestAdapter = (
builder: TxExecutionRequestBuilder, request: UserRequest
const addPublicDeployment = (
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

builder: TxExecutionRequestBuilder, call: UserFunctionCall
) => {
const { artifact, instance } = getCanonicalInstanceDeployer();
const deployFnAbi = findFunctionAbi(artifact, 'deploy');
Expand All @@ -478,6 +511,7 @@ export const addPublicDeployment: TxExecutionRequestAdapter = (
}
));
}

```

#### Entrypoints implement `TxExecutionRequestComponent`
Expand Down Expand Up @@ -544,9 +578,6 @@ The abstract `BaseWallet` can implement:

```ts
async getTxExecutionRequest(userRequest: UserRequest): Promise<TxExecutionRequest> {
if (!userRequest.functionAbi) {
throw new Error('Function ABI must be provided');
}
if (!userRequest.gasSettings) {
throw new Error('Gas settings must be provided');
}
Expand All @@ -556,34 +587,27 @@ async getTxExecutionRequest(userRequest: UserRequest): Promise<TxExecutionReques

const builder = new TxExecutionRequestBuilder();

// Add the "main" function call
builder.addAppFunctionCall(
makeFunctionCall(
userRequest.functionAbi,
userRequest.contractInstance.address,
userRequest.args
));
for (const call of request.calls) {
addMainFunctionCall(builder, call);
if (call.deploymentOptions?.registerClass) {
addClassRegistration(builder, call);
}
if (call.deploymentOptions?.publicDeploy) {
addPublicDeployment(builder, call);
}
// if the user is giving us an artifact,
// allow the PXE to access it
if (call.contractArtifact) {
builder.addTransientContract({
artifact: call.contractArtifact,
instance: call.contractInstance,
});
}
}

// Add stuff needed for setup, e.g. function calls, auth witnesses, etc.
await userRequest.paymentMethod.adaptTxExecutionRequest(builder, userRequest);

if (userRequest.deploymentOptions?.registerClass) {
addClassRegistration(builder, userRequest);
}

if (userRequest.deploymentOptions?.publicDeploy) {
addPublicDeployment(builder, userRequest);
}

// if the user is giving us an artifact,
// allow the PXE to access it
if (userRequest.contractArtifact) {
builder.addTransientContract({
artifact: userRequest.contractArtifact,
instance: userRequest.contractInstance,
});
}

// Adapt the request to the entrypoint in use.
// Since BaseWallet is abstract, this will be implemented by the concrete class.
this.adaptTxExecutionRequest(builder, userRequest);
Expand All @@ -597,8 +621,8 @@ async getTxExecutionRequest(userRequest: UserRequest): Promise<TxExecutionReques

```ts
// Used by simulate and read
async #simulateInner(userRequest: UserRequest): ReturnType<Wallet['simulate']> {
const txExecutionRequest = await this.getTxExecutionRequest(initRequest);
async #simulateInner(userRequest: UserRequest): ReturnType<BaseWallet['simulate']> {
const txExecutionRequest = await this.getTxExecutionRequest(userRequest);
const simulatedTx = await this.simulateTx(txExecutionRequest, builder.simulatePublicFunctions, builder.from);
const decodedReturn = decodeSimulatedTx(simulatedTx, builder.functionAbi);
return {
Expand All @@ -607,7 +631,7 @@ async #simulateInner(userRequest: UserRequest): ReturnType<Wallet['simulate']> {
privateOutput: simulatedTx.privateReturnValues,
executionResult: simulatedTx.executionResult,
result: decodedReturn,
request: initRequest,
request: userRequest,
};
}
```
Expand All @@ -632,7 +656,7 @@ async simulate(userRequest: UserRequest): {

const builder = new UserRequestBuilder(userRequest);

await this.#ensureFunctionAbi(builder);
await this.#ensureFunctionAbis(builder);

if (builder.gasSettings) {
return this.#simulateInner(builder.build());
Expand All @@ -653,16 +677,18 @@ async simulate(userRequest: UserRequest): {
return result;
}

async #ensureFunctionAbi(builder: UserRequestBuilder): void {
// User can call simulate without the artifact if they have the function ABI
if (!builder.functionAbi) {
// If the user provides the contract artifact, we don't need to ask the PXE
if (!builder.contractArtifact) {
const contractArtifact = await this.getContractArtifact(builder.contractInstance.contractClassId);
builder.setContractArtifact(contractArtifact);
async #ensureFunctionAbis(builder: UserRequestBuilder): void {
for (const call of builder.calls) {
// User can call simulate without the artifact if they have the function ABI
if (!call.functionAbi) {
// If the user provides the contract artifact, we don't need to ask the PXE
if (!call.contractArtifact) {
const contractArtifact = await this.getContractArtifact(call.contractInstance.contractClassId);
call.setContractArtifact(contractArtifact);
}
const functionAbi = findFunctionAbi(call.contractArtifact, call.functionName);
call.setFunctionAbi(functionAbi);
}
const functionAbi = findFunctionAbi(builder.contractArtifact, builder.functionName);
builder.setFunctionAbi(functionAbi);
}
}

Expand Down Expand Up @@ -695,7 +721,7 @@ async read(userRequest: UserRequest): DecodedReturn | [] {
builder.setGasSettings(GasSettings.default());
}

await this.#ensureFunctionAbi(builder);
await this.#ensureFunctionAbis(builder);

return this.#simulateInner(builder.build());
}
Expand All @@ -710,7 +736,7 @@ async prove(request: UserRequest): Promise<UserRequest> {
throw new Error('Execution result must be set before proving');
}
const builder = new UserRequestBuilder(request);
await this.#ensureFunctionAbi(builder);
await this.#ensureFunctionAbis(builder);
const initRequest = builder.build();
const txExecutionRequest = await this.getTxExecutionRequest(initRequest);
const provenTx = await this.proveTx(txExecutionRequest, request.executionResult);
Expand All @@ -730,7 +756,7 @@ async send(request: UserRequest): Promise<UserRequest> {
throw new Error('Tx must be proven before sending');
}
const builder = new UserRequestBuilder(request);
await this.#ensureFunctionAbi(builder);
await this.#ensureFunctionAbis(builder);
const initRequest = builder.build();
const txExecutionRequest = await this.getTxExecutionRequest();
const txHash = await this.sendTx(txExecutionRequest, request.tx);
Expand All @@ -744,6 +770,8 @@ async send(request: UserRequest): Promise<UserRequest> {

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

Choose a reason for hiding this comment

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

+1!


Downside here is that the "pipeline" it goes through would be less clear, and components would have to be more aware of the type of request they are dealing with.

#### 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Expand Down