-
Notifications
You must be signed in to change notification settings - Fork 134
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
Allow JS values as ZkProgram inputs #1934
base: main
Are you sure you want to change the base?
Conversation
…level SelfProof as private input type
Ready for review! |
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.
Great work @mitschabaude! However, I’m concerned that this PR might make ZkProgram
code less readable 🤔
@@ -1842,6 +1842,15 @@ class UInt8 extends Struct({ | |||
return new UInt8(x); | |||
} | |||
|
|||
static fromValue( |
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 case of fromField(ff)
, isn't this similar to new UInt8(ff.value)
or UInt8.from(ff)
, which are not recommended to use instead of UInt8.Unsafe.fromField(ff)
because they don't clearly indicate the potential unsafety of wrapping a Field into a UInt8, making the code less readable.
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 absolutely, and this method isn't really intended for developers to use (that's what .from()
was designed for).
fromValue()
is part of the Provable<T>
API and what we use internally to convert JS values into the type -- it's needed here for that reason.
Note that this PR doesn't add UInt8.fromValue()
-- it was already there, we just override it to also handle number
inputs (which is not unsafe), because number
is the most natural type that you'd want to convert a UInt8 from
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.
there are two things to improve IMO, both of which aren't related to this PR though:
- internal
Provable
methods should be on a separate namespace.provable
instead of on the class itself, to mark them more clearly as not-developer facing MoveProvable<T>
methods to a separate.provable
namespace, on each provable type #1248 UInt8
shouldn't be aStruct
, because whileStruct
is great for easily composing types, it's not great for defining the base primitives. In this case, the problem is that thefromValue()
type ofUInt8
is inferred from theStruct({ value: Field })
definition to include{ value: Field }
, which really should beUInt8
instead (but that doesn't work because the Struct definition doesn't know anything aboutUInt8
)
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.
isn't this similar to new UInt8(ff.value) or UInt8.from(ff), which are not recommended to use
side note: UInt8.from(field)
is perfectly fine, because it adds a range-check
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.
Thanks for the insight!
UInt8 shouldn't be a Struct, because while Struct is great for easily composing types, it's not great for defining the base primitives.
I recall encountering an issue a while ago where I couldn't use Provable.witness
for a UInt32
with proofsEnabled=true
. Is this related to the same issue, or is it unrelated? I’ll need to revisit and check this again!
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.
UInt32
is not implemented with Struct
(but with the older CircuitValue
) so that might be a different issue!
in the case of UInt8
, one thing that bites me now and then is that UInt8 satisfies Provable<UInt8>
is not true (and similar for other Structs), because e.g. the return type of UInt8.fromFields()
is { value: Field }
and not UInt8
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.
While if you look at newer types like Bytes
, you'll see that because of the way we made .provable
aware of the class itself, this works:
Bytes(32) satisfies ProvableType<Bytes>;
@mitschabaude FYI we are discussing this change but with the holidays and some other urgent work going on at the moment, not everyone has had a chance to review. |
We can already pass in normal js values when witnessing provable types. This PR enables passing normal values into ZkProgram provers (both normal and recursive provers).
Also improves the supported JS values for a few important types like
Signature
andUIntX