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

Flexible circuit values #416

Merged
merged 30 commits into from
Sep 28, 2022
Merged

Flexible circuit values #416

merged 30 commits into from
Sep 28, 2022

Conversation

mitschabaude
Copy link
Collaborator

@mitschabaude mitschabaude commented Sep 14, 2022

The first aspect this changes regards the default "circuit type" in snarkyjs, which is now more general:

  • Change most places which required an AsFieldElements<T> to now require an AsFieldsAndAux<T>, the more general type which allows non-circuit ("auxiliary") contents, like strings. This makes our system of "circuit values" similar to the one in OCaml, where a typ holds both field elements and auxiliary data.
  • Modify AsFieldElements<T> so that it extends AsFieldsAndAux<T>. Thus, a circuit type which only consists of field elements is now a special case of a general circuit type. AsFieldElements<T> can be passed in wherever AsFieldsAndAux<T> is required. But not the other way around.
    • API change that came out of this: ofFields is now called fromFields
    • ofBits -> fromBits for consistency
  • Only a few places remain which actually do require AsFieldElements<T> for a reason: For example, the type for events and actions, because the protocol requires to send events as an array of field elements, so it must not contain auxiliary data, because we wouldn't be able to recover them from fetched events
  • Changes Circuit.witness to witness an AsFieldsAndAux<T> (in compile mode, this means it creates field elements which are zero and aux values which have a default value, e.g. an empty string if the type is String)
  • Leave the CircuitValue class and built-in types like UInt32 as is, still being an AsFieldElements<T>. Thus, our public API is mostly unaffected by this batch of changes

More general circuit types can, for now, be generated with not very discoverable helpers circuitValue and circuitValueClass. I think it's worthy of discussion whether we want to make those the "default" / easily discoverable way of creating circuit types. However, I don't want to block the PR by these discussions.

  • Makes circuitValue support the following non-Field types: String, Number, Boolean, BigInt, null,, undefined
  • Makes circuitValue have very good typing, using recursive type inference:
    • you pass it a JSON-like object of "circuit types", like {x: Field, s: String}
    • it infers the TS type that corresponding values should have, in this case {x: Field, s: string}
    • if returns an AsFieldsAndAux<T>, which has static methods like toFields, toAuxiliary, toJSON, all of which are precisely-typed
    • it even infers the precise return type of toJSON, in this case {x: string: s: string}
  • Adds circuitValueClass which wraps circuitValue in a class, thus providing a new way to declare CircuitValues which can be used as method arguments. This class tries to be usable just as if it were the plain anonymous object:
class MyCircuitValue extends circuitValueClass({
  x: Field,
  s: String
}) {}

// ...

@method myMethod(value: MyCircuitValue) {
  value.s // has type `string` --> MyCircuitValue inherits the type inferred by circuitValue
}

// you can pass in a plain object bc `MyCircuitValue` is compatible with that as a typ
zkapp.myMethod({ x: Field.zero, s: "lalala" }) 

Finally, we use circuiValueClass to provide a new VerificationKey class which can be used to pass verification keys into methods.

  • fix token tests to actually change the verification key in their deploy methods
  • kicked out the old VerificationKey type which was related to plain Circuits and not really used right now

@mitschabaude mitschabaude marked this pull request as ready for review September 20, 2022 09:34
Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

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

This is great

I think we should take this opportunity to rename AsFields* to something better since we’re making big changes to those types here. I also still dislike CircuitValue as circuit couples our nomenclature too much to proof systems that have circuits as their backend (which in this case is just an implementation detail subject to change).

To start the conversation on this: How about ProvableValue (for circuit value) and Provable (for AsFieldsAndAux)? And then adding a “pure” modifier as relevant like you did in this PR.

it’s also probably a good time to make the circuitValue constructors more discoverable as you’ve suggested (like under the Provable namespace? Examples: Provable.Value, Provable.value). Up to you if you think this would be too big in scope to include as part of this PR, but probably we should avoid a snarkyjs release until making these changes.

and then we’d include doccoments on all the Provable stuff

src/lib/circuit_value.unit-test.ts Show resolved Hide resolved
src/lib/test.ts Outdated Show resolved Hide resolved
@mitschabaude
Copy link
Collaborator Author

@bkase good idea to do the name changes in this PR! I think ProvableValue is great. I was thinking about the same today and considering ProverValue, but ProvableValue is even better, has less potential for confusion. I also like Provable<T> and PureProvable<T> -- or maybe ProvablePure<T>?

I wonder whether having circuitValue or circuitValueClass under Provable.value has confusion potential with ProvableValue. But I think it'll works for now.

Another idea: Should we even call it a "value", or might it be even clearer if we call it a "type"? What about ProvableType?

@jasongitmail
Copy link
Contributor

I like this UX too, but prefer the term Struct instead of creating new terminology of ProvableValue. Then we can say “A Struct in SnarkyJS allows you to define custom types”. It's intuitive, shorter, and doesn't raise questions for devs to have to answer to feel they understand what it does AND avoids some devs from feeling like every value they want to use in the circuit must be in this class, as opposed to just compound values. So I'd find it more intuitive.

import { Struct } from 'snarkyjs';

class Animal extends Struct({
x: Field,
s: String
}) {}

@@ -43,6 +43,9 @@ type NonMethodKeys<T> = {
}[keyof T];
type NonMethods<T> = Pick<T, NonMethodKeys<T>>;

/**
* @deprecated `CircuitValue` is deprecated in favor of `Struct`, which features a simpler API and better typing.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Should you link to Struct in the comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How should we link? the [[ Struct ]] thing wasn't working

src/snarky.d.ts Outdated Show resolved Hide resolved
@mitschabaude mitschabaude merged commit f11008b into main Sep 28, 2022
@mitschabaude mitschabaude deleted the feature/flexible-circuit-values branch September 28, 2022 08:58
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.

Generalize method arguments to allow auxiliary data
3 participants