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

fix: remove stableswap ed check #1003

Merged
merged 12 commits into from
Jan 31, 2025
Merged

fix: remove stableswap ed check #1003

merged 12 commits into from
Jan 31, 2025

Conversation

dmoka
Copy link
Contributor

@dmoka dmoka commented Jan 27, 2025

We should not do this check as we can have accounts with balance less than T::MinPoolLiquidity then the following check prevents the shares withdrawal

ensure!(
current_share_balance == share_amount
|| current_share_balance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
Error::<T>::InsufficientShareBalance
);

TODO:

  • check on chopsticks the wasm if works

Copy link

github-actions bot commented Jan 27, 2025

Crate versions that have been updated:

  • runtime-integration-tests: v1.31.0 -> v1.32.0
  • pallet-stableswap: v4.2.0 -> v4.3.0
  • hydradx-runtime: v282.0.0 -> v283.0.0

Runtime version has been increased.

@dmoka dmoka marked this pull request as ready for review January 28, 2025 08:09
@dmoka dmoka self-assigned this Jan 28, 2025
@jak-pan
Copy link
Contributor

jak-pan commented Jan 30, 2025

I think this check was supposed to check the shares left after withdrawal to stop you from being locked out as there is similar check in XYK if I remember correctly.

However, this won't prevent it as you can just send shares away. This means we can end up in a state where there is enough shares in a pool but there is not enough shares in anybody's account to actually withdraw anything. I think the solution to this is to tie the minimal pool balances/shares to existential deposit.

This should be viable thing after we implement #1007

However this number probably shouldn't change. I would consider setting it to the fallback values so we don't end up in the same situation.

@dmoka
Copy link
Contributor Author

dmoka commented Jan 30, 2025

However this number probably shouldn't change. I would consider setting it to the fallback values so we don't end up in the same situation.

So you mean that instead of removing the checks, we should lower the MinPoolLiquidity so to 1_000 from 1_000_000?

@jak-pan
Copy link
Contributor

jak-pan commented Jan 30, 2025

However this number probably shouldn't change. I would consider setting it to the fallback values so we don't end up in the same situation.

So you mean that instead of removing the checks, we should lower the MinPoolLiquidity so to 1_000 from 1_000_000?

No. :D I don't know how this was deduced. We should remove this check and I made a new issue for the problem this was probably supposed to fix (but in a wrong way here)

@dmoka dmoka merged commit 450a986 into master Jan 31, 2025
7 checks passed
@dmoka dmoka deleted the fix/remove-stableswap-ed-check branch January 31, 2025 15:03
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