-
Notifications
You must be signed in to change notification settings - Fork 136
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
Open
mitschabaude
wants to merge
14
commits into
main
Choose a base branch
from
feature/zkprogram-value-inputs
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+96
−50
Open
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
35a36b6
allow normal JS values as zkprogram proof inputs
mitschabaude fb8ca65
test JS value inputs in some examples
mitschabaude af183db
proper value type for Signature
mitschabaude 70d0940
minor
mitschabaude e456a99
support JS values as public input, and be careful not to use the top …
mitschabaude a57e753
add index argument to mapObject
mitschabaude 36a5091
test public input JS values
mitschabaude cb5ce5d
improve `From` types for uints
mitschabaude 5669375
minor
mitschabaude 46dad3b
Merge branch 'feature/zkprogram-qol' into feature/zkprogram-value-inputs
mitschabaude 986c073
changelog
mitschabaude e6e24d0
Merge branch 'main' into feature/zkprogram-value-inputs
mitschabaude e1b4e70
fixup
mitschabaude 5af2f00
adapt hash chain example
mitschabaude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 tonew UInt8(ff.value)
orUInt8.from(ff)
, which are not recommended to use instead ofUInt8.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 theProvable<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 handlenumber
inputs (which is not unsafe), becausenumber
is the most natural type that you'd want to convert a UInt8 fromThere 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:
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 #1248UInt8
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.
side note:
UInt8.from(field)
is perfectly fine, because it adds a range-checkThere 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!
I recall encountering an issue a while ago where I couldn't use
Provable.witness
for aUInt32
withproofsEnabled=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 withStruct
(but with the olderCircuitValue
) so that might be a different issue!in the case of
UInt8
, one thing that bites me now and then is thatUInt8 satisfies Provable<UInt8>
is not true (and similar for other Structs), because e.g. the return type ofUInt8.fromFields()
is{ value: Field }
and notUInt8
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: