Tokens #273

MartinMinkov · 2022-07-05T22:34:52Z

Description

This PR adds support for custom tokens and token symbols for SnarkyJS. It implements the requirements mentioned in the following RFC: #233

Mina PR: MinaProtocol/mina#11449

To enable minting, burning, and sending, changes to the Party structure was made to support this.token()... as well as this.tokenSymbol.set(...). Additionally, changes to account fetching have been made to support querying for a specific token id in addition to a public key.

Tested

Implemented unit tests in token.test.ts to check different behaviors that tokens offer. These tests are only related to the local ledger and thus are not tested with a live network yet. To test with a live network, a new QA network should be deployed and then a zkApp deployed that acts similar to the zkApp in the test file.

mitschabaude

started review, will continue tomorrow!

src/lib/fetch.ts

src/lib/party.ts

src/lib/precondition.ts

src/snarky.d.ts

src/lib/mina.ts

ymekuria

Great work Martin! I just had a couple very minor comments/questions.

src/lib/party.ts

src/lib/state.ts

mitschabaude

Finished reviewing! Just a couple of smaller suggestions from me -- great work!

The most important change I'm suggesting is to remove the tokenId argument in token(tokenId?), because it has to be the same as this.tokenId anyway. (Correct me if I'm wrong.)

src/lib/party.ts

mitschabaude · 2022-07-19T20:53:52Z

src/lib/party.ts

@@ -516,8 +577,105 @@ class Party {
    return new (Party as any)(body, authorization, party.isSelf);
  }

+  token(tokenId?: Field) {


could we rename this argument to parentTokenId for clarity?

oh wait -- doesn't the parentTokenId have to equal this.self.body.tokenId? Otherwise the this account isn't the owner and can't do this? maybe we don't need that argument after all??

This comment is a little confusing to me, I'll have to come back to it :P

src/lib/party.ts

src/lib/token.test.ts

src/lib/circuit_value.ts

…ional feedback

mitschabaude

lgtm now!

src/lib/token.test.ts

jasongitmail · 2022-07-21T22:26:33Z

src/lib/token.test.ts

+    it('should have a valid custom token id', async () => {
+      const tokenId = zkapp.token().id;
+      const expectedTokenId = new Token({ tokenOwner: zkappAddress }).id;
+      expect(tokenId).toBeDefined();


could delete this line expect(tokenId).toBeDefined(); given the following assertion, no? Or is there a chance it returns undefined?

jasongitmail · 2022-07-21T22:32:51Z

src/lib/token.test.ts

+
+  @method init() {
+    this.tokenSymbol.set(tokenSymbol);
+    this.totalAmountInCirculation.set(UInt64.zero);


I think we should consider using the term Quantity, instead of Amount, everywhere. On trading dashboards, qty refers to the number of tokens and amount prefers to price * amount, which would be USD etc. when trading on fiat markets. So qty is more consistent with how it's used on most financial platforms.

I also typically see (and we use consistently in the backend code) the term "totalSupply" for this -- probably for consistency we should go with that.

bkase

This looks great! I just have a few questions that either you can just explain away in comments or perhaps I found a bug

bkase · 2022-07-15T18:22:27Z

src/lib/fetch.ts

-const accountQuery = (publicKey: string) => `{
-  account(publicKey: "${publicKey}") {
+const accountQuery = (publicKey: string, tokenId: string) => `{
+  account(publicKey: "${publicKey}" tokenId: "${tokenId}") {


don't you need a comma between the publicKey and tokenId in the query?

Good catch!

bkase · 2022-07-22T17:26:06Z

src/lib/mina.ts

      if (ledgerAccount == undefined) {
        throw new Error(
-          `getAccount: Could not find account for public key ${publicKey.toBase58()}`
+          `getAccount: Could not find account for public key ${publicKey.toBase58()} with the tokenId ${Ledger.fieldToBase58(


can you omit the with the tokenId ... in the case that the tokenId is the default one?

Good call, fixed!

bkase · 2022-07-22T17:26:23Z

src/lib/mina.ts

        if (account !== undefined) return account;
      }
      throw Error(
-        `getAccount: Could not find account for public key ${publicKey.toBase58()}.\nGraphql endpoint: ${graphqlEndpoint}`
+        `getAccount: Could not find account for public key ${publicKey.toBase58()} with the tokenId ${Ledger.fieldToBase58(


bkase · 2022-07-22T17:26:47Z

src/lib/mina.ts

        Fetch.defaultGraphqlEndpoint
      );
      if (account === undefined)
        throw Error(
-          `getAccount: Could not find account for public key ${publicKey.toBase58()}. Either call Mina.setActiveInstance first or explicitly add the account with addCachedAccount`
+          `getAccount: Could not find account for public key ${publicKey.toBase58()} with the tokenId ${Ledger.fieldToBase58(


same here (should this string be a constant stored somewhere instead of inlined all the time?)

bkase · 2022-07-22T17:28:34Z

src/lib/party.ts

+      tokenOwner.toFields().every((x) => x.isConstant()) &&
+      parentTokenId.isConstant()
+    ) {
+      this.id = Ledger.customTokenIdChecked(tokenOwner, this.parentTokenId);


What's the difference between these branches?

hey @MartinMinkov, I think these branches are wrong.. the else branch should use the checked version, not the if branch

@bkase, if the token owner or parent token id is a variable then we need to derive the token id in-snark. (Not sure if we could always use the checked version, depends on how flexible it is written, but IIRC checked hashing returns a new variable even when the inputs are constant, which wouldn't work outside the circuit)

Yikes! Good catch, switched these. 😅

bkase · 2022-07-22T17:30:13Z

src/lib/party.ts

+        receiverParty.body.balanceChange =
+          receiverParty.body.balanceChange.sub(amount);
+
+        // Require signature from the receiver account being deducted


Is this always true? Can't certain tokens be burned via proof?

Good question, I'm not sure how this would work with the current SnarkyJS API. @mitschabaude, is there a way to require either a proof or signature? Also, how would one construct a proof to do this (in the context of a developer creating a proof with SnarkyJS)?

Aaah yes. I forgot about that. It depends on the permissions on the sending party's account whether it needs a proof or a signature.

This is a puzzle 🤔 because different zkApps have completely different ways of authorizing transactions. So my fear is token contracts will typically impose a certain interface on zkApps for that token.

I'm pretty sure @mrmr1993 has thought deeper about this

In any case, I think there's no easy solution here. The token() API we designed seems fundamentally not flexible enough to handle parties with proofs, because they are all different. The API must be one where users provide their own parties, not where we create parties for them. But even then it seems very tricky to make a circuit for the token owner contract that's flexible enough to handle arbitrary zkApp children. So I'm not even sure we can ever provide a ready-made, general implementation for parties with proofs.

That makes it all the more important to at least expose the low-level pieces to create token contracts -- I think that's the Token class (which I still think should be just a function)

Maybe a fairly general solution is not that hard to build after all.
The token() functions must witness their child parties instead of hardcoding them into the circuit, and only assert the properties of them that they care about - balanceChanges and tokenId.

The challenge is how to create a good API for this. I imagine its functions must take a party as input, which might mean we have to make Party one of the possible arguments of a smart contract method. A smart contract could sort of "call" a token contract with 'this' as an argument, and that token contract would make the smart contract its child. A sort of inverted zkapp composability

I guess the natural API from the protocol perspective would be if the token contract methods would accept a callback, which is like a transaction block in that you can run a (or possibly multiple) smart contract methods in it, which would create parties that are authorized with a proof. And the token contract would then assert certain conditions on the resulting parties, for example

They can't have children

There must be exactly / at most X of them

The balances must cancel in send

In mint / burn, conditions X, Y, Z must hold

I created a separate issue (#300) so that we can discuss the priority & path forward on this. It's out of scope for this PR, it's already nice to have token transfers authorized by a signature

bkase · 2022-07-22T17:30:57Z

src/lib/party.ts

+        senderParty.body.balanceChange =
+          senderParty.body.balanceChange.sub(amount);
+
+        // Require signature from the sender party


same here -- aren't there certain senders that need a proof instead?

bkase · 2022-07-22T17:33:59Z

src/lib/token.test.ts

+
+  @method init() {
+    this.tokenSymbol.set(tokenSymbol);
+    this.totalAmountInCirculation.set(UInt64.zero);


I also typically see (and we use consistently in the backend code) the term "totalSupply" for this -- probably for consistency we should go with that.

MartinMinkov force-pushed the test/tokens-playground branch 6 times, most recently from 065f60b to a714246 Compare July 18, 2022 19:08

MartinMinkov mentioned this pull request Jul 18, 2022

Tokens MinaProtocol/mina#11449

Merged

MartinMinkov marked this pull request as ready for review July 18, 2022 19:26

MartinMinkov changed the title ~~[WIP] - Tokens~~ Tokens Jul 18, 2022

MartinMinkov requested review from mitschabaude and bkase July 18, 2022 19:27

jasongitmail assigned MartinMinkov Jul 18, 2022

mitschabaude reviewed Jul 18, 2022

View reviewed changes

ymekuria approved these changes Jul 18, 2022

View reviewed changes

src/lib/party.ts Outdated Show resolved Hide resolved

src/lib/party.ts Outdated Show resolved Hide resolved

MartinMinkov force-pushed the test/tokens-playground branch from 4b9b9f7 to 0b6be23 Compare July 19, 2022 17:23

mitschabaude reviewed Jul 19, 2022

View reviewed changes

src/lib/state.ts Outdated Show resolved Hide resolved

mitschabaude reviewed Jul 19, 2022

View reviewed changes

src/lib/circuit_value.ts Outdated Show resolved Hide resolved

mitschabaude reviewed Jul 19, 2022

View reviewed changes

src/lib/circuit_value.ts Outdated Show resolved Hide resolved

MartinMinkov added 11 commits July 20, 2022 11:43

feat: Add transfer of custom tokens naively

f9ae535

Mess around with Parties

980c1d8

feat: Add token class

7ddbaa2

this.token.transfer successfully changes balances with public key input

c9ebc35

feat: Brute force custom token transfer

c5a34d5

Change createUnsigned to use optional object parameter for tokens

c87de4c

Remove old transfer

4d44977

Use Bool() for account creation fee for token accounts

25b06b7

Add basic mint/burn methods

74bbd4f

wip: burn and transfer pass transaction logic

432abc1

Set lazy-signature for required parties in token transfers

a94b68b

MartinMinkov added 12 commits July 20, 2022 11:43

Refactor getAccount and getBalance parameters

f0c2d72

Implement burn and send tests

ece0c82

Fix fetchAccount type parameters

6ff6785

Use init method for zkApp state

f5178ce

Add support for checked tokenId, convert tokenId type to Field, addit…

d94b75f

…ional feedback

Refactor checked computation check and error messages

48c9101

Update bindings

4802f95

PR feedback

cc3d690

Fix parentTokenId parameter for new Token()

92be6a4

Add getter for tokenId on Party structure

3f78945

Update bindings

1225452

Fix CircuitValue.isConstant

fe55f6e

MartinMinkov force-pushed the test/tokens-playground branch from e9dd4e1 to fe55f6e Compare July 20, 2022 19:01

Update for isNew

c359962

mitschabaude approved these changes Jul 20, 2022

View reviewed changes

MartinMinkov added 2 commits July 20, 2022 18:57

Fix intg tests

d870ae1

Merge branch 'main' into test/tokens-playground

39281e7

MartinMinkov force-pushed the test/tokens-playground branch from b9919ab to 39281e7 Compare July 21, 2022 19:53

Use createChildParty instead of createUnsigned

e73a4f6

jasongitmail reviewed Jul 21, 2022

View reviewed changes

Merge branch 'main' into test/tokens-playground

903a860

bkase approved these changes Jul 22, 2022

View reviewed changes

MartinMinkov added 3 commits July 22, 2022 11:35

Add formatted getAccount error

8203973

Fix checked and unchecked condition in token constructor

dc19579

Fix GraphQL query parameter

4788a9a

mitschabaude mentioned this pull request Jul 25, 2022

Token transfers between zkApps #300

Closed

mitschabaude added 2 commits July 25, 2022 13:42

integrate deploy() with qanet

997eecd

fix integration test

c0035b6

MartinMinkov merged commit a4fae70 into main Jul 25, 2022

MartinMinkov deleted the test/tokens-playground branch July 25, 2022 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tokens #273

Tokens #273

MartinMinkov commented Jul 5, 2022 •

edited

Loading

mitschabaude left a comment

ymekuria left a comment

mitschabaude left a comment

mitschabaude Jul 19, 2022

mitschabaude Jul 19, 2022

MartinMinkov Jul 19, 2022

mitschabaude left a comment

jasongitmail Jul 21, 2022

jasongitmail Jul 21, 2022

bkase Jul 22, 2022

bkase left a comment

bkase Jul 15, 2022

MartinMinkov Jul 22, 2022

bkase Jul 22, 2022

MartinMinkov Jul 22, 2022

bkase Jul 22, 2022

bkase Jul 22, 2022

bkase Jul 22, 2022

mitschabaude Jul 22, 2022

MartinMinkov Jul 22, 2022 •

edited

Loading

bkase Jul 22, 2022

MartinMinkov Jul 22, 2022 •

edited

Loading

mitschabaude Jul 22, 2022 •

edited

Loading

mitschabaude Jul 22, 2022

mitschabaude Jul 22, 2022 •

edited

Loading

mitschabaude Jul 23, 2022

mitschabaude Jul 25, 2022 •

edited

Loading

bkase Jul 22, 2022

bkase Jul 22, 2022

Tokens #273

Tokens #273

Conversation

MartinMinkov commented Jul 5, 2022 • edited Loading

Description

Tested

mitschabaude left a comment

Choose a reason for hiding this comment

ymekuria left a comment

Choose a reason for hiding this comment

mitschabaude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinMinkov Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinMinkov Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

mitschabaude Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinMinkov commented Jul 5, 2022 •

edited

Loading

MartinMinkov Jul 22, 2022 •

edited

Loading

MartinMinkov Jul 22, 2022 •

edited

Loading

mitschabaude Jul 22, 2022 •

edited

Loading

mitschabaude Jul 22, 2022 •

edited

Loading

mitschabaude Jul 25, 2022 •

edited

Loading