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

chore: inverse unit tests for CalcInAmtGivenOut and CalcOutAmtGivenIn with zero swap fee #1254

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 14, 2022

Closes: #XXX

Description

Trying to investigate #1196 by incrementally adding more unit tests

These tests do not allow us to conclude anything with regards to #1196 yet. The tests added so far have a swap fee of 0. I'm planning to continue adding unit tests with a non-zero swap fee. Also, test other functionality and methods that are being called in ExitSwapShareAmountIn and JoinSwapExactAmountIn


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@github-actions github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Apr 14, 2022
@p0mvn p0mvn marked this pull request as ready for review April 14, 2022 21:27
@p0mvn p0mvn force-pushed the roman/calc-inverse-ut branch from 8faa836 to 0b04698 Compare April 14, 2022 21:30
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #1254 (b8560e2) into main (b6d342a) will decrease coverage by 0.63%.
The diff coverage is 9.51%.

@@            Coverage Diff             @@
##             main    #1254      +/-   ##
==========================================
- Coverage   20.90%   20.26%   -0.64%     
==========================================
  Files         196      203       +7     
  Lines       25425    26824    +1399     
==========================================
+ Hits         5316     5437     +121     
- Misses      19118    20377    +1259     
- Partials      991     1010      +19     
Impacted Files Coverage Δ
x/claim/abci.go 0.00% <ø> (ø)
x/claim/client/cli/query.go 34.45% <ø> (ø)
x/claim/client/cli/tx.go 0.00% <ø> (ø)
x/claim/handler.go 0.00% <ø> (ø)
x/claim/keeper/claim.go 64.33% <ø> (ø)
x/claim/keeper/grpc_query.go 2.50% <ø> (ø)
x/claim/keeper/hooks.go 28.00% <ø> (ø)
x/claim/keeper/keeper.go 87.50% <ø> (ø)
x/claim/keeper/params.go 81.81% <ø> (ø)
x/claim/module.go 58.06% <ø> (ø)
... and 108 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e12c8e...b8560e2. Read the comment docs.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

So what did we find out here? There's a test case commented out in TestJoinSwapExactAmountInConsistency.

@p0mvn
Copy link
Member Author

p0mvn commented Apr 16, 2022

So what did we find out here? There's a test case commented out in TestJoinSwapExactAmountInConsistency.

Nothing yet, I'm planning to continue increasing tests / coverage for all methods called within JoinSwapExactAmountIn.

Currently, it's hard to determine where exactly the error is happening because we are missing unit tests for each of the methods called byJoinSwapExactAmountIn. By increasing the number of unit tests for each of the called methods, we should make it more likely to find the problem

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Hrmm interesting. So I think these test cases tell us that running CalcInAmtGivenOut and CalcOutAmtGiven straight away returns the correct value we expect, and that the problem is coming from other steps?

x/gamm/pool-models/balancer/util_test.go Show resolved Hide resolved
@p0mvn
Copy link
Member Author

p0mvn commented Apr 18, 2022

Hrmm interesting. So I think these test cases tell us that running CalcInAmtGivenOut and CalcOutAmtGiven straight away returns the correct value we expect, and that the problem is coming from other steps?

It's unclear yet. I have tested with 0 swap fees so far. I'm planning to add the tests with a non-zero swap fee in a separate PR

@mattverse
Copy link
Member

Sounds great! Can we address that this is a test for zero fee either in the comment or in the test case name?

@alexanderbez
Copy link
Contributor

Can we leave this PR in Draft mode to indicate that it's still being worked on?

@p0mvn p0mvn marked this pull request as draft April 18, 2022 23:46
@p0mvn p0mvn changed the title chore: inverse unit tests for CalcInAmtGivenOut and CalcOutAmtGivenIn chore: inverse unit tests for CalcInAmtGivenOut and CalcOutAmtGivenIn with zero swap fee Apr 18, 2022
@p0mvn
Copy link
Member Author

p0mvn commented Apr 18, 2022

Sounds great! Can we address that this is a test for zero fee either in the comment or in the test case name?

Done! I also created an issue to continue with the tests with a non-zero swap fee: #1288

@alexanderbez this is ready for review. I'm trying to work on this in small parts / PRs to make it easier to review

@p0mvn p0mvn marked this pull request as ready for review April 18, 2022 23:55
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

🌮

@alexanderbez alexanderbez requested a review from mattverse April 20, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gamm Changes, features and bugs related to the gamm module.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants