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

Minimum reserves check #19

Closed
wants to merge 11 commits into from
Closed

Minimum reserves check #19

wants to merge 11 commits into from

Conversation

JanKuczma
Copy link
Collaborator

No description provided.

Copy link
Contributor

@deuszx deuszx left a comment

Choose a reason for hiding this comment

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

Let's add some tests around this. Ideally we'd test that no matter the decimals of the pool tokens the minimum reserves constraint makes sense.

@JanKuczma
Copy link
Collaborator Author

Let's add some tests around this. Ideally we'd test that no matter the decimals of the pool tokens the minimum reserves constraint makes sense.

I added some tests. Ideally, these tests will be refactored appropriately in #17.

@deuszx
Copy link
Contributor

deuszx commented Jul 4, 2024

Let's add some tests around this. Ideally we'd test that no matter the decimals of the pool tokens the minimum reserves constraint makes sense.

I added some tests. Ideally, these tests will be refactored appropriately in #17.

I hope not in #17 as it's already huge as hell

Copy link
Contributor

@deuszx deuszx left a comment

Choose a reason for hiding this comment

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

As discussed on Slack – let's add tests for removing liquidity.

Copy link
Contributor

@deuszx deuszx left a comment

Choose a reason for hiding this comment

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

I left comments under commit (f24facb#comments) – do you think you can take a look there? ;)

LGTM otherwise 💪

Alternative would be to write a proptest with proper generators but given the amount of tests now that's probably unnecessary.

@woocash2
Copy link
Contributor

woocash2 commented Jul 4, 2024

Potential problem

I see that MIN_RESERVE is set to TARGET_PRECISION which means that for any token, we require at least 1 full unit of that token to be present in the pool at any time, after the first liquidity deposit.

This doesn't look nice to me, considering tokens whose $ value is very high. Take for example that we or someone else wants to create a pair which contains BTC, for example (BTC, stakedBTC) (I don't know if such a pair makes sense, but it's the best example I could think of). MIN_RESERVE implies that:

  • the first person making a deposit has to be pretty rich - they have to invest at least 120K$ into this pair
  • at least 1 BTC and 1 stakedBTC have to exist in the pool forever, which means that effectively ~120K$ is "irrecoverable" by this pool.

ETH is a less dramatic example as it costs ~3000$, but still, (ETH, stakedETH) pool makes around 6000$ "irrecoverable".

Questions

  • Are trades impossible in the case of reserves (X, 0) or (0, 0), i.e. will contract panic (abort), or will the user just get 0 profit, or giga profit (depending on the trade direction), but contract call will be successful? This means: is reserve[i] >= MIN_RESERVE required for the pool to work without error?
  • Have you considered at least making the MIN_RESERVE smaller than 1 unit? I suppose that the only reserve values which prevent the contract from working are 0. So why not MIN_RESERVE = 1 or some other less significat value than 1 unit?

@JanKuczma JanKuczma closed this Jul 5, 2024
@JanKuczma JanKuczma deleted the min-reserve branch July 9, 2024 10:56
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.

3 participants