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

zkApp composability #297

Merged
merged 22 commits into from
Aug 1, 2022
Merged

zkApp composability #297

merged 22 commits into from
Aug 1, 2022

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Jul 22, 2022

This is the final PR for zkApp composability.

Copy link
Member Author

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

some comments to help reviewing!

@@ -0,0 +1,118 @@
/**
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the example that demonstrates composability

@@ -533,3 +546,38 @@ Circuit.constraintSystem = function <T>(f: () => T) {
);
return result;
};

let memoizationContext = Context.create<{
Copy link
Member Author

Choose a reason for hiding this comment

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

new global context that is used to remember prover values which are created during createTransaction, to be reused in the prover for the same method. In snarkyjs, only used for the blindingValue right now, but I figured it would be nice to also expose that capability to the outside world -- that's what the memoizeWitness function below is for.

@@ -467,6 +472,43 @@ function synthesizeMethodArguments(
return args;
}

function methodArgumentsToConstant(
{ allArgs, proofArgs, witnessArgs }: MethodInterface,
Copy link
Member Author

Choose a reason for hiding this comment

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

two new helper functions we need to convert the input arguments of a zkapp call to inputs to our hash function

fetchMode: inProver() ? 'cached' : 'test',
isFinalRunOutsideCircuit: false,
},
if (!smartContractContext.has()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This function -- wrappedMethod -- is what always gets called when you call any smart contract method. It is where most of the new logic lives. The function is now quite complex!

This first, large if branch is handling the case that was supported already -- calling a zkApp method when you're not inside another zkApp method.

assertStatePrecondition(this);
return result;
if (inCheckedComputation()) {
// important to run this with a fresh party everytime, otherwise compile messes up our circuits
Copy link
Member Author

Choose a reason for hiding this comment

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

the code below didn't really change in its entirety. It's just marked green because there is more nesting, causing two more spaces on the left

}
callDataFields.push(getMethodId(methodIntf, methodIndex));
callDataFields.push(blindingValue);
party.body.callData = Poseidon.hash(callDataFields);
Copy link
Member Author

Choose a reason for hiding this comment

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

This whole code block is the case that we're in a circuit. So, these commands are what is added to the circuit on top of the user code in a @method. It is also how a called method is executed inside its own prover (but not in the prover of the caller -- that case is further below!). On this line, we're populating our own callData field -- as explained in the RFC, this is done every time, to make the method callable by default

}
// if we're here, this method was called inside _another_ smart contract method
Copy link
Member Author

Choose a reason for hiding this comment

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

this is what happens inside the caller, inside calling another method. the code below has some comments to explain what's happening

if (!doProofs) zkapp.sign(zkappKey);
});
if (doProofs) {
console.log('proving (3 proofs.. can take a bit!)');
Copy link
Contributor

Choose a reason for hiding this comment

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

How long are the 3 proofs taking on your machine @mitschabaude?

Copy link
Member Author

Choose a reason for hiding this comment

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

takes 42 sec for me

@@ -0,0 +1,118 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great intuitive example to demonstrate composability.

` // ...\n` +
`}\n\n` +
`Note: Only types built out of \`Field\` are valid return types. This includes snarkyjs primitive types and custom CircuitValues.`;
// if we're lucky, analyzeMethods was already run on the callee smart contract, and we can catch this error early
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarifying question. When does analyzeMethods run for the first time on a contract?

Copy link
Member Author

Choose a reason for hiding this comment

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

It runs

  • in SmartContract.compile() (before actually compiling)
  • when computing the SmartContract.digest()
  • in a Mina.transaction block, it runs if one of the methods needs it under the hood; this is only the case when reducing sequence events right now.
    So, it definitely runs when we're proving things (because we compile first), but likely not if we don't.

The code above will throw the helpful error message if we're not proving, so that case is covered.

The case that's problematic is when we compile the caller first, and the callee second. Then, the "if we're lucky" case above is not true in the caller circuit. Then, we won't be able to nicely catch the missing return type annotation, and the caller's compile will throw an ugly, unhelpful error instead.

Thanks for poking into this :D Maybe I should make it so that analyzeMethods figures out all the SmartContracts called inside a method, and also runs analyzeMethods on them. This way, the callee would definitely be analyzed already in the code above, so we'd avoid the "ugly error" case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: I tried this out, but wasn't able to make it work. The ugly error is already thrown during analyzeMethods, so I have no way of finding the list of callees before. I think we'll have to live with a suboptimal error message in this edge case

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: I tried this out, but wasn't able to make it work. The ugly error is already thrown during analyzeMethods, so I have no way of finding the list of callees before. I think we'll have to live with a suboptimal error message in this edge case

At least we are aware of the edge case error behavior.

Base automatically changed from feature/composability to main July 26, 2022 10:15
@mitschabaude mitschabaude mentioned this pull request Jul 30, 2022
@mitschabaude
Copy link
Member Author

Update: Implemented callData hashing as proposed here: #303 (comment)
I think this is a solution that already makes sense, so leaving further tweaks to a separate PR

@mitschabaude mitschabaude merged commit c0bd0de into main Aug 1, 2022
@mitschabaude mitschabaude deleted the feature/composability-rebased branch August 1, 2022 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support interactions between 2 smart contracts (i.e. composability, call stack)
3 participants