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

Add send API to zkApp and Party classes #325

Merged
merged 5 commits into from
Aug 4, 2022
Merged

Conversation

MartinMinkov
Copy link
Contributor

@MartinMinkov MartinMinkov commented Aug 3, 2022

Implements #281

Adds the .send() method on both the Party and zkApp classes. Additionally added a test script to verify correct behavior.

src/lib/party.ts Outdated
@@ -670,6 +671,21 @@ class Party implements Types.Party {
};
}

send({ to, amount }: Omit<SendParams, 'from'>) {
let party = this;
let receiverParty = Party.createUnsigned(to);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the this party has a non-default token id, then this method could be used to send tokens (e.g., inside the smart contract callback we give to a token contract). This would be cool functionality! However, for this to work, the receiver party must get the same token id. (The current implementation would add Mina on the receiver party but subtract tokens from the this party, which would just make the txn fail)

So, I suggest to add the following line here:

receiverParty.body.tokenId = party.body.tokenId

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another thing: We should think about whether we want to make the receiverParty the child of the sender here, by default. Otherwise, the receiver is not linked to the circuit, so I'm scared that a user would add checks on the receiver which would do nothing, and they'd have a security hole that could be exploited

Copy link
Contributor Author

@MartinMinkov MartinMinkov Aug 3, 2022

Choose a reason for hiding this comment

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

Ahh, that's a good point I didn't think about. When you say by default, is there a case that the receiverParty wouldn't want to be a child in this case? Seems like we would always want to link the receiver to the circuit to verify its correctness instead of inserting a constant value. :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah let's do it always for now! I think it might make our lives a little harder when doing the token proofs authorization thing, but we'll figure it out then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, done! :D

@@ -0,0 +1,131 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

@MartinMinkov @mitschabaude do you have an opinion if we should be adding examples like these in the src/examples/zkapps directory or keep them in src/examples? I noticed apps like composability and simple_and_counter were added to the zkapps directory and others to examples. It's not a big deal either way as long as we are in agreement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good point, I think we'll accumulate more & more of those, so I'd add most of them to /src/examples/zkapps and reserve /src/examples for the most fundamental examples, to not overwhelm a user who explores these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I'll move it over

@MartinMinkov MartinMinkov merged commit fb264ee into main Aug 4, 2022
@MartinMinkov MartinMinkov deleted the feat/mina-transfer branch August 4, 2022 20:13
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.

4 participants