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

Remove liquidity funcs and tests #276

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Remove liquidity funcs and tests #276

merged 2 commits into from
Oct 23, 2023

Conversation

Tcintra
Copy link
Collaborator

@Tcintra Tcintra commented Oct 20, 2023

Description

Addresses #258 (but only for StableSwap pools)

Added remove_liquidity and remove_liquidity_imbalanced for the vanilla StableSwap pools, along with their Vyper fixtures and unit tests. The unit tests just do the same as the other add/remove liquidity tests: it checks that the Vyper and Python instances of the 3pool will end with the same balances, tokens, and D.

The Vyper implementation is largely copied from what already exists in metapool.vy, with minor changes. The Python implementation is similar to the calc_token_amount function.

Disclaimer: this is my first time contributing and it is very possible that I am missing something. Please let me know and I will address any concerns.

Hygiene checklist

  • Changelog entry
  • Everything public has a Numpy-style docstring
    (modules, public functions, classes, and public methods)
  • Commit history is cleaned-up with minor changes squashed together
    and descriptive commit messages following Tim Pope's style

Cute Animal Picture

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2023

Coverage report

The coverage rate went from 75.75% to 75.96% ⬆️
The branch rate is 62%.

97.22% of new lines are covered.

Diff Coverage details (click to unfold)

curvesim/pool/stableswap/pool.py

97.22% of new lines are covered (96.79% of the complete file).
Missing lines: 554

@allt0ld
Copy link
Collaborator

allt0ld commented Oct 21, 2023

Nice work (haven't reviewed thoroughly though). By the way, you can run make changelog_entry in your terminal, which will generate a changelog file. I would put your additions to pool.py and basepool.vy under the "Added" section in the changelog file. You don't need to describe the tests. Delete the other changelog file sections. Then, git add <changelog file path> so you can commit and push it.

Happy first contribution!

curvesim/pool/stableswap/pool.py Outdated Show resolved Hide resolved
test/fixtures/curve/basepool.vy Outdated Show resolved Hide resolved
curvesim/pool/stableswap/pool.py Outdated Show resolved Hide resolved
curvesim/pool/stableswap/pool.py Outdated Show resolved Hide resolved
test/fixtures/curve/basepool.vy Show resolved Hide resolved
test/fixtures/curve/basepool.vy Outdated Show resolved Hide resolved
@allt0ld allt0ld self-requested a review October 23, 2023 15:37
@allt0ld
Copy link
Collaborator

allt0ld commented Oct 23, 2023

Good work @Tcintra. Before you merge, could you leave a comment in this thread explaining your use case for returning fees/amounts from remove_liquidity_imbalance in the .py version?

@Tcintra
Copy link
Collaborator Author

Tcintra commented Oct 23, 2023

The remove_liquidity_imbalance function is used by crvUSD's PegKeepers when withdrawing crvUSD from StableSwap pools. We are emulating this behavior in our Agent Based simulation models for stress testing crvUSD.

Since curvesim's StableSwap pools don't track the individual balances of users, we need a way to track the balance of the PegKeeper when it mints or burns LP tokens. The add_liquidity functions returns the LP tokens minted, and we need the remove_liquidity_imbalance function to return the tokens burnt. Returning the fees paid is not strictly necessary right now, but might be useful for analysis on PK profitability in the future.

@Tcintra Tcintra merged commit 5e0a36b into main Oct 23, 2023
15 checks passed
@Tcintra Tcintra deleted the remove-liquidity branch October 23, 2023 16:43
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.

2 participants