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.

Sign up for GitHub

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

Jump to bottom

feat: AMM #8644

Closed
wants to merge 67 commits into from
Closed

feat: AMM #8644

wants to merge 67 commits into from

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Sep 19, 2024 •
edited
Loading

This is an implementation of a Uni v2 style DEX pool. One of the main goals of this PR was to figure out what kind of token flows we will need.

Note for reviewers

I put a lot of comments and some questions in the code. The code does not yet compile as the token is not yet updated with the flows we need but I have a high confidence that it's very close to final.

Problems

There were a couple problems I have stumbled upon:

  1. The need for having the liquidity token be separate from the DEX/pair contract itself. This is because we don't have inheritance in Noir and copying the contract code into the DEX is not something I am willing to go through.
  2. We don't have a large enough integer type that is not terrible - (U128 is too cumbersome to use because of #8271). I currently use u64 everywhere but that is not sufficiently large for DeFi.

@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Sep 19, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @benesjan and the rest of your teammates on Graphite Graphite

@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch 6 times, most recently from e81e65b to b6ec0d9 Compare September 24, 2024 10:09
@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch 2 times, most recently from 18388c1 to a7f547c Compare October 7, 2024 17:52
@benesjan benesjan requested a review from nventuro October 8, 2024 08:49
@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch 4 times, most recently from d33c3fe to 577e834 Compare October 10, 2024 10:20
@benesjan benesjan changed the title feat: DEX draft feat: AMM DEX pool Oct 10, 2024
@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch from 2e4ece6 to d4c437e Compare October 10, 2024 16:47
@benesjan benesjan changed the title feat: AMM DEX pool feat: AMM Oct 10, 2024
nventuro
nventuro reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Didn't have sufficient time to also look at the swap fns, so only left some initial comments.

noir-projects/noir-contracts/contracts/amm_contract/src/lib.nr Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/lib.nr Outdated
Comment on lines 14 to 16
let amount_in_with_fee = amount_in * 997;
let numerator = amount_in_with_fee * reserve_out;
let denominator = reserve_in * 1000 + amount_in_with_fee;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we're hardcoding the 0.3% swap fee here. It'd be better to receive this as a fouth param (univ2 doesn't do this because their swap fee is immutable, but even if we kept ours immutable I really don't like this way of handling out).

We can do this in a follow up issue if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to keep it immutable as we want to implement here a minimal AMM.

but even if we kept ours immutable I really don't like this way of handling out
Would you like me to store the fee as a constant instead?

noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Outdated Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Outdated Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Outdated Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Outdated Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Outdated Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Outdated Show resolved Hide resolved
noir-projects/noir-contracts/contracts/amm_contract/src/main.nr Outdated Show resolved Hide resolved
@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch 3 times, most recently from 417f074 to 0d483c9 Compare October 30, 2024 21:36
@benesjan benesjan changed the base branch from master to 10-30-refactor_replacing_unshield_naming_with_transfer_to_public October 30, 2024 21:36
This was referenced Oct 30, 2024
Merged
Merged
@benesjan benesjan force-pushed the 10-30-refactor_replacing_unshield_naming_with_transfer_to_public branch from 348ab77 to 3b21a31 Compare October 31, 2024 01:47
@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch from 7dbd0ad to 5677cd6 Compare October 31, 2024 01:47
@benesjan benesjan changed the base branch from 10-30-refactor_replacing_unshield_naming_with_transfer_to_public to graphite-base/8644 October 31, 2024 02:33
@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch from 5677cd6 to 06f429e Compare October 31, 2024 02:34
@benesjan benesjan force-pushed the 09-19-feat_dex_draft branch from 3075b32 to a404b58 Compare November 14, 2024 15:58
@nventuro nventuro mentioned this pull request Nov 22, 2024
Merged
@nventuro
Copy link
Contributor

Superceded by #8644

@nventuro nventuro closed this Nov 22, 2024
@nventuro nventuro deleted the 09-19-feat_dex_draft branch November 22, 2024 20:05
nventuro added a commit that referenced this pull request Dec 3, 2024
@nventuro
feat: AMM (#10153)
90668c3 
Opening in favor of
#8644 due to
@benesjan being away on vacation, and that PR using Graphite. Work here
starts from commit a404b58, which has
been squashed into 81d7607.

This is the first implementation of Uniswap v2 style AMM that provides
identity privacy. The contract is a single pool for two tokens with a
fixed 0.3% swap fee. Adding and removing liquidity is done
proportionally to the current ratio, resulting in no price impact and
therefore no fees. Swaps can be performed by specifying either the
amount in or the amount out. All three operations are completed in a
single transaction each by leveraging partial notes.

I created #10225
to track pending work. Some of the tasks in that epic are only work that
arises from the AMM, but are not technically required to have a fully
functioning system: this PR already achieves that.

Only a happy-path end to end test is included here, since TXE currently
lacks some of the features required in order to properly deal with
partial notes. We should later write those as this will be a good test
of TXE's capabilities and user experience, given the complexity of the
setup.

---

I added the e2e to be run on CI on all branches since it combines
multiple complex features, and likely contains our largest transactions
yet.
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this pull request Dec 4, 2024
@nventuro @AztecBot
feat: AMM (#10153)
b74f341 
Opening in favor of
AztecProtocol/aztec-packages#8644 due to
@benesjan being away on vacation, and that PR using Graphite. Work here
starts from commit a404b58e7d049ee7a56310702046f03a624fd1ee, which has
been squashed into 81d7607d9d551aea4d6de78ec3ff535ec5d5a29a.

This is the first implementation of Uniswap v2 style AMM that provides
identity privacy. The contract is a single pool for two tokens with a
fixed 0.3% swap fee. Adding and removing liquidity is done
proportionally to the current ratio, resulting in no price impact and
therefore no fees. Swaps can be performed by specifying either the
amount in or the amount out. All three operations are completed in a
single transaction each by leveraging partial notes.

I created AztecProtocol/aztec-packages#10225
to track pending work. Some of the tasks in that epic are only work that
arises from the AMM, but are not technically required to have a fully
functioning system: this PR already achieves that.

Only a happy-path end to end test is included here, since TXE currently
lacks some of the features required in order to properly deal with
partial notes. We should later write those as this will be a good test
of TXE's capabilities and user experience, given the complexity of the
setup.

---

I added the e2e to be run on CI on all branches since it combines
multiple complex features, and likely contains our largest transactions
yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thunkar Thunkar Thunkar left review comments

@nventuro nventuro nventuro left review comments

@rahul-kothari rahul-kothari Awaiting requested review from rahul-kothari

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benesjan @nventuro @Thunkar