-
Notifications
You must be signed in to change notification settings - Fork 529
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
Tokens #11449
Tokens #11449
Conversation
8f6237d
to
8bed816
Compare
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.
awesome!
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.
actually, one ask: could you bump the snarkyjs submodule to the tip of your token PR so we can run it against CI?
let custom_token_id pk token = | ||
Mina_base.Account_id.derive_token_id | ||
~owner:(Mina_base.Account_id.create (public_key pk) (token_id token)) | ||
|> Mina_base.Token_id.to_string |> Js.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.
I think we need to cover the case where pk
and token
represent variables, see comments I'll add on the other PR. possibly cover that in a separate function custom_token_id_checked
. so that both can have the same interface (pk: PublicKey, token: Field) => Field
, I think probably this should return a Field
as well
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's probably something like Mina_base.Account_id.Checked.derive_token_id
in the code base that we can use
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.
Got it! Added support for a Checked
version, mind taking a look if the new implementation is correct?
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.
looks good except for the two comments!
Mina_base.Account_id.Checked.derive_token_id | ||
~owner: | ||
(Mina_base.Account_id.Checked.create | ||
(Signature_lib.Public_key.Compressed.var_of_t (public_key pk)) |
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.
using var_of_t
is wrong because it's only possible if the variable has been converted to a value first, which is what the public_key
function does. The forced converting to a value that public_key
does only works inside the prover, or if the pk
is a constant, but not when compiling the circuit.
We want to preserve the same underlying variables that pk
contains (two field elements; possibly variables) and convert them to a Public_key.Compressed.var
in an in-snark way. This is code that I got to type-check that I think achieves that:
let public_key_checked (pk : public_key) :
Signature_lib.Public_key.Compressed.var =
let x = pk##.g##.x##.value in
let y = pk##.g##.y##.value in
Signature_lib.Public_key.compress_var (x, y) |> Impl.run_checked
(although I'm unsure what run_checked
means.. but it has the right type signature :D)
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.
Got it! I changed the implementation and removed the use of var_of_t
. The code snippet type checks and seems to pass existing test cases 👍
|
||
let default_token_id_js = | ||
Mina_base.Token_id.default |> Mina_base.Token_id.to_field_unsafe | ||
|> Field.constant |> to_js_field | ||
|
||
let account_id_checked pk token = | ||
Mina_base.Account_id.Checked.create | ||
(Signature_lib.Public_key.Compressed.var_of_t (public_key pk)) |
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.
same applies here about public_key
+ var_of_t
not being the approach we want
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.
Changed implementation 👍
@@ -46,6 +46,8 @@ module Digest : sig | |||
|
|||
val of_field : Pickles.Impls.Step.Field.t -> t | |||
|
|||
val to_field_unsafe : t -> Pickles.Impls.Step.Field.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.
This feels super hacky, does this work? I need a way to convert a Mina_base.Token_id.Checked.t
to a Field.t
. This type checks but I'm pretty sure the logic is wrong. If there is a better way, can someone advise how to do so?
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 this is the right approach 👍🏻 Would've done the same
168d372
to
e8e36cc
Compare
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.
LGTM!
5f5cb41
to
464d2d9
Compare
dec6980
to
5d3a4f5
Compare
143f86f
to
e924994
Compare
e924994
to
0280361
Compare
Description
Adds custom token and token symbol support in SnarkyJS. This PR adds several helper functions for interacting with custom token ids and token symbols. Additionally adds token id and token symbol to the account type to enable querying the local ledger in SnarkyJS tests. Finally, this PR adds a test script for token and token symbol functionality.
These OCaml changes are needed for o1-labs/o1js#273
Tested
Implemented a JS test script and ran with succession.