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

Dex happy path #484

Merged
merged 30 commits into from
Oct 19, 2022
Merged

Dex happy path #484

merged 30 commits into from
Oct 19, 2022

Conversation

MartinMinkov
Copy link
Contributor

@MartinMinkov MartinMinkov commented Oct 13, 2022

This PR finishes the happy path of the first DEX version, without proofs. Proofs currently run into an exotic Pickles error when executing "redeemLiquidity". We checked that the account update structures created by the DEX methods make sense and are as expected.

This also

@mitschabaude
Copy link
Collaborator

Happy path without proving works!

Only TODO for this PR that I see is to investigate & potentially remove the unexpected account updates in redeemLiquidity

And fix composability test

Copy link
Collaborator

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

A couple of comments to help reviewing!

};
let ctx = snarkContext.get();
let fields =
inCheckedComputation() && !ctx.inWitnessBlock
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change improves Circuit.witness in two ways:

  • it doesn't throw an error when used outside the circuit
  • it doesn't create another witness block when already inside one

@@ -1003,18 +1009,30 @@ Circuit.constraintSystem = function <T>(f: () => T) {
return result;
};

Circuit.log = function (...args: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the log function requested in #489

Copy link
Member

Choose a reason for hiding this comment

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

great addition

Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome!

@@ -227,21 +227,6 @@ interface Mina {
) => { hash: string; actions: string[][] }[];
}

interface MockMina extends Mina {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this interface wasn't exported and having it internally just made updating LocalBlockchain more annoying.

General philosophy: Let TS just infer types if it can, instead of writing out return types (unless we want to change something about the return type on purpose). This is less work, leads to more accurate types, and better for users because they can see the type instead of its name

@@ -1028,6 +1026,17 @@ class AccountUpdate implements Types.AccountUpdate {
}
return accountUpdate;
}
static attachToTransaction(accountUpdate: AccountUpdate) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will sometimes (but rarely) need to be called if something is done in a transaction block without calling methods, because we stopped implicitly attaching the account update to the transaction when this.self is called (which led to bugs). Being in the spirit of less opaqueness.

TODO: is this a good name / API?

@@ -702,6 +702,7 @@ class SmartContract {
}
this.setValue(this.self.update.permissions, Permissions.default());
this.sign(zkappKey);
AccountUpdate.attachToTransaction(this.self);
Copy link
Collaborator

Choose a reason for hiding this comment

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

here we can see an example where we need explicit attaching of an account update to the transaction

@mitschabaude mitschabaude changed the title Dex Redeem liquidity debug Dex happy path Oct 19, 2022
Copy link
Member

@Trivo25 Trivo25 left a comment

Choose a reason for hiding this comment

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

lgtm, just left a few questions :)

tx = await Mina.transaction(userKey, () => {
// pay fees for creating 2 token contract accounts, and fund them so each can create 1 account themselves
let feePayerUpdate = AccountUpdate.createSigned(userKey);
feePayerUpdate.balance.subInPlace(accountFee.mul(1));
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for multiplying it times one? Probably not, just curious tho

Copy link
Collaborator

Choose a reason for hiding this comment

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

no there isn't :D it was copied from another place where it was multiplied by 2 I guess

@@ -58,6 +64,7 @@ class Dex extends SmartContract {
let isXZero = dexXBalance.equals(UInt64.zero);
let xSafe = Circuit.if(isXZero, UInt64.one, dexXBalance);

// FIXME
Copy link
Member

Choose a reason for hiding this comment

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

is this important?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes! reminds us to fix it

@@ -1003,18 +1009,30 @@ Circuit.constraintSystem = function <T>(f: () => T) {
return result;
};

Circuit.log = function (...args: any) {
Copy link
Member

Choose a reason for hiding this comment

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

great addition

Copy link
Contributor

@ymekuria ymekuria left a comment

Choose a reason for hiding this comment

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

LGTM! I just had some minor comments/questions.

tx = await Mina.transaction(userKey, () => {
// pay fees for creating user's token X account
AccountUpdate.createSigned(userKey).balance.subInPlace(accountFee.mul(1));
// 😈😈😈 mint any number of tokens to our account 😈😈😈
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the comment emojis ha!

@@ -9,9 +16,9 @@ import {
} from './dex.js';

Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a reason you created a separate arbitrary_token_interaction.ts file and this run.ts file? Is the goal to only execute the run.ts file in CI and not the arbitrary_token_interaction.ts file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes -- this arbitrary_token_interaction.ts file is a reproduction of that token protocol bug I found yesterday. so, it's an example of an interaction that actually shouldn't be possible. I'd like to come back to this when the bug is fixed and make a test from it or something

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that makes sense. Maybe we should document the file with this info so we don't forget later on.

@@ -1003,18 +1009,30 @@ Circuit.constraintSystem = function <T>(f: () => T) {
return result;
};

Circuit.log = function (...args: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome!

@mitschabaude mitschabaude merged commit 0f17d72 into main Oct 19, 2022
@mitschabaude mitschabaude deleted the dex/redeem-liquidity branch October 19, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants