-
Notifications
You must be signed in to change notification settings - Fork 110
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
Recursion Pt 1 #245
Recursion Pt 1 #245
Conversation
This reverts commit 6846bb9.
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.
Added some comments to aid review:
@@ -0,0 +1,129 @@ | |||
import { |
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 file tests the additions of this PR!
|
||
// TODO properly type this at the interface | ||
// create recursive type that describes JSON-like structures of circuit types | ||
function circuitValue<T>(typeObj: any): AsFieldElements<T> { |
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.
helper function which provides functionality equivalent to CircuitValue
, but is less brittle, doesn't require decorators and can be applied in situations where a circuit value should be created from a plain JS object
i.e.
let MyCircuitValue = circuitValue({ a: Group, b: Bool, c: [Field, Field] });
is equivalent to (but is more precisely typed than)
class MyCircuitValue extends CircuitValue {
@prop a: Group;
@prop b: Bool;
@arrayProp(Field, 2) c: Field[];
constructor(a: Group, b: Bool, c: Field[]) {
this.a = a;
this.b = b;
this.c = c;
}
}
circuitValue
is only used in one place right now but I want to explore further usage, for example to simplify non-decorator ways of specifying circuit inputs
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 I think we should move towards the more precise typed versions of things with generics in general.
src/lib/mina.ts
Outdated
@@ -43,7 +45,7 @@ interface Transaction { | |||
toJSON(): string; | |||
toGraphqlQuery(): string; | |||
sign(additionialKeys?: PrivateKey[]): Transaction; | |||
prove(): Promise<Transaction>; | |||
prove(): Promise<(Proof<ZkappStatement> | undefined)[]>; |
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.
only breaking change: tx.prove
now returns a list of proofs (one for each party, undefined
for parties without proofs) instead of the transaction itself. (Because it was async, returning the transaction wasn't very useful for chaining anyway)
): any {} | ||
class Proof<T> { | ||
static publicInputType: AsFieldElements<any> = undefined as any; | ||
static tag: () => { name: string } = () => { |
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.
the new Proof
class needs a tag
to identify the compiled SmartContract
that this proof belongs to
src/snarky.d.ts
Outdated
} | ||
|
||
declare interface AsFieldElements<T> { | ||
toFields(x: T): Field[]; | ||
ofFields(x: Field[]): T; | ||
sizeInFields(): number; | ||
check?: (x: T) => void; | ||
check(x: T): void; |
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 made this change to facilitate inferring <T>
from and AsFieldElements<T>
.
That every circuit type has a check
method is now also enforced by Circuit.witness
.
@imeckler had mentioned that he should've made check
required instead of optional, so I went with that; it makes for simpler types and less special-casing.
This change has minor repercussions throughout the code base
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.
It would have been better if this change was made separately.
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 you're right. It was made to enable some typing that I intended to add in this PR, but ended up saving for the next PR.
src/snarky.d.ts
Outdated
static sizeInFields(): number; | ||
} | ||
// TODO: Add this when OCaml bindings are implemented: | ||
// declare class EndoScalar { |
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.
removed this because it didn't actually exist!
await Promise.all(global.wasm_workers.map((worker) => worker.terminate())); | ||
process.exit(0); | ||
} | ||
process.exit(0); |
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.
shutdown
as previously implemented failed to always shut down.. so I just made it exit the node process directly
src/lib/zkapp.ts
Outdated
methodName, | ||
(...args: unknown[]) => (instance[methodName] as any)(...args), | ||
witnessArgs | ||
// TODO: it's problematic that this refers to the instance that was passed in during compile |
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.
=>
arrow functions should be considered dangerous! If you write function() { ... }
instead, you get the this
dispatched by call
/apply
/the method's owner/global object.
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.
fixed 9f60ff4
src/snarky.d.ts
Outdated
verify: (statement: Statement, proof: unknown) => Promise<boolean>; | ||
compile: ( | ||
rules: Pickles.Rule[], | ||
statementSize: number |
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.
To quote this comment from the Mina-side PR:
Getting the user to manually pass the size of the public input seems error-prone at best, and can lead to under-constrained circuits at worst. Is there not a cleaner way?
I know, for example, that we keep a
size_in_field_elements
in eachTyp.t
on the OCaml side. This also has the benefit of not forcing the user to handle an array of field elements when they actually want a structured public input.
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 understand the objection. This is not intended as a user-facing API though, it's internal functionality.
That Pickles
ends up being exported from snarkyjs is an unintentional side effect of exporting *
from this file.
I propose to fix this by removing it from the exports. OK?
(It's a good idea anyway to reduce unintentional exporting from snarkyjs)
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 propose to fix this by removing it from the exports. OK?
Sgtm
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.
fixed! e7d69ba
src/snarky.d.ts
Outdated
} | ||
|
||
declare interface AsFieldElements<T> { | ||
toFields(x: T): Field[]; | ||
ofFields(x: Field[]): T; | ||
sizeInFields(): number; | ||
check?: (x: T) => void; | ||
check(x: T): void; |
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.
It would have been better if this change was made separately.
|
||
// TODO properly type this at the interface | ||
// create recursive type that describes JSON-like structures of circuit types | ||
function circuitValue<T>(typeObj: any): AsFieldElements<T> { |
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 I think we should move towards the more precise typed versions of things with generics in general.
@@ -278,6 +288,64 @@ function circuitMain( | |||
} | |||
|
|||
let primitives = new Set(['Field', 'Bool', 'Scalar', 'Group']); | |||
let complexTypes = new Set(['object', 'function']); | |||
|
|||
// TODO properly type this at the interface |
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.
Can you add a todo for unit tests here too?
SmartContract
s. That is, smart contracts can now verify their own proofs as well as proofs from other smart contracts