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

Bug: balance check in router fails when providers of account changes during the trade #797

Closed
dmoka opened this issue Apr 5, 2024 · 3 comments · Fixed by #812
Closed
Assignees

Comments

@dmoka
Copy link
Contributor

dmoka commented Apr 5, 2024

Issue:

  • in router we still do the balance check, but only check if the user spent at least the amount we calculate for the trade
  • we use reducible_balance with mode Preservation.Expendable which behaves differently when the system.account.providers of the user account changes
  • during this trade, the number of providers changes, increased by one, leading to a different behaviour for the balance check, namely an off-by-ED error

Fix would be to always use reducible_balance with Preservation.Preserve so the ED is always deducted from balances, resulting in consistent behaviour.

First we need to fix a bug in orml package: open-web3-stack/open-runtime-module-library#986

Cases on prod:

@dmoka dmoka self-assigned this Apr 5, 2024
@dmoka
Copy link
Contributor Author

dmoka commented Apr 5, 2024

Opened a PR for the fix in ORML: open-web3-stack/open-runtime-module-library#987

@dmoka
Copy link
Contributor Author

dmoka commented Apr 12, 2024

It root cause is coming from other part too:

What happens is that the account of insufficient asset is killed, so the ED (1 HDX) is transferred back to the account. So our ensure_that_user_received_asset_out_at_most balance check fails, as the user have more balance with an ED.

Two solutions I can see:

  • check if the insufficient asset account is killed, so we can incorporate ED in the balance check
  • we discard the post balance checks. This might look easier first, but remember we added it back to prevent any issues like we had with stableswap a few weeks ago

@dmoka
Copy link
Contributor Author

dmoka commented Apr 23, 2024

Related: #810

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant