-
Notifications
You must be signed in to change notification settings - Fork 109
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
Transaction Construction, Pt 1. @method
and compile()
#115
Transaction Construction, Pt 1. @method
and compile()
#115
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor questions, looks great! Excited to see the following PRs!
@method async update(y: Field) { | ||
let x = await this.x.get(); | ||
@method update(y: Field) { | ||
let x = this.x.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will the .get
function work synchronously? I figured that it would eventually make a network call to get the state of a contract on the chain, and that would need to be async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I laid out here how I think it will work: #113
The only alternative I see would be to refactor Pickles so that it can handle async circuits. This would be better for users (enables them to make their methods async, which adds a lot of flexibility), but is probably much harder to implement.
console.log('compile'); | ||
let { | ||
provers: [updateProver], | ||
} = SimpleSnapp.compile(snappPubKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should references of snapp
be scrubbed for zkapp
? Not sure if that's already done in a future PR :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's in a future PR :P
'Cannot deploy SmartContract outside a transaction.' | ||
); | ||
} catch { | ||
throw new Error('Cannot deploy SmartContract outside a transaction.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these errors get surfaced up to the user? Maybe we can add an additional text snippet alongside this message that helpfully guides the user on how to fix this if there is a common fix. For this, something like Make sure you are using Mina.transaction
? Wondering what everyone else thinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current version, this particular error is actually not present any more, but a very similar error mentioning Mina.transaction
would be thrown: https://github.com/o1-labs/snarkyjs/blob/f0014217b9911438da621751a33f7bd3c97f3586/src/lib/zkapp.ts#L485
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #130 and my other comment
let snarkySpec = require('./snarky-class-spec.json'); | ||
|
||
let { picklesCompile } = snarky; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be really helpful to have a short comment above on what picklesCompile
is doing. Since it's defined in the OCaml side of things, it would be nice to know what it does without swapping to a different project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed here: f001421
provers: [updateProver], | ||
} = SimpleSnapp.compile(snappPubKey); | ||
|
||
console.log('deploy'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the console logs are removed in further PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't! Are you against logging out what's happening in examples? I tend to find that helpful, especially logging the initial and final states here helps developers confirm that it's working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fair -- these seem okay to keep. I think we should do something more fancy in the future, but we can do that later.
} | ||
return fields; | ||
} | ||
return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it less surprising for this to throw
or somehow accumulate the skipped data instead of return []
for non-toFields-able fields of an object? I'm thinking this will lead to lots of subtle errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function isn't used right now, but being able to return []
is important to handle mixed objects, which contain both circuit- and non-circuit properties. For example, a party.body.update.verificationKey
contains a property value: string
, which holds the base58-formatted verification key. This string should be ignored by toFieldsMagic()
, so that a Party can be turned into field elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accumulating the skipped data could be an option!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to remove the function instead of changing it (b80d39f), since it's unused and most likely won't be useful, because if we want to compute hash inputs we'll have to conform to the more elaborate system used in Mina.
witnessArgs.push(Parameter); | ||
} else { | ||
throw Error( | ||
`Argument ${i} of method ${methodName} is not a valid circuit value.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not in this PR, I made #130 to track it).
We should include details about why this is a not a valid circuit value and what a circuit value is in this error.
'Cannot deploy SmartContract outside a transaction.' | ||
); | ||
} catch { | ||
throw new Error('Cannot deploy SmartContract outside a transaction.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #130 and my other comment
Pickles.compile
and add methods to compile SmartContracts@method
decorator to easily specify SmartContract sub-circuits / branches