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

[CL]: SwapInverseTest errTolerance #4475

Closed
Tracked by #4424
czarcas7ic opened this issue Mar 1, 2023 · 0 comments · Fixed by #4533
Closed
Tracked by #4424

[CL]: SwapInverseTest errTolerance #4475

czarcas7ic opened this issue Mar 1, 2023 · 0 comments · Fixed by #4533
Assignees
Labels
C:x/concentrated-liquidity F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board
Milestone

Comments

@czarcas7ic
Copy link
Member

Background

Previously for the SwapIn/OutAmtGivenOut/In inverse tests, we checked if the user and pool balances after the swap were EXACTLY the same:

// Assure that user balance now as it was before both swaps.
userBalanceAfterSwap := s.App.BankKeeper.GetAllBalances(s.Ctx, s.TestAccs[0])
poolBalanceAfterSwap := s.App.BankKeeper.GetAllBalances(s.Ctx, poolBefore.GetAddress())
s.Require().Equal(userBalanceBeforeSwap, userBalanceAfterSwap)
s.Require().Equal(poolBalanceBeforeSwap, poolBalanceAfterSwap)

This passed because we weren't actually doing bank sends on the expected method. In a recent PR, bank sends were added, and now this test only passes when adding a error tolerance.

// Assure that user balance now as it was before both swaps.
userBalanceAfterSwap := s.App.BankKeeper.GetAllBalances(s.Ctx, s.TestAccs[0])
poolBalanceAfterSwap := s.App.BankKeeper.GetAllBalances(s.Ctx, poolBefore.GetAddress())
for _, coin := range userBalanceBeforeSwap {
s.Require().Equal(0, errTolerance.Compare(userBalanceBeforeSwap.AmountOf(coin.Denom), userBalanceAfterSwap.AmountOf(coin.Denom)))
}
for _, coin := range poolBalanceBeforeSwap {
s.Require().Equal(0, errTolerance.Compare(poolBalanceBeforeSwap.AmountOf(coin.Denom), poolBalanceAfterSwap.AmountOf(coin.Denom)))
}

Suggested Design

  • Prior to addressing this, investigate the potential use of BigDec for swaps
    • If we use BigDecs, the diff here should be minimal if not non-existent and this can we swapped back
    • If we dont use BigDec, check if the tolerance used here is acceptable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/concentrated-liquidity F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants