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

add test for CalcExitPool #1540

Merged
merged 8 commits into from
May 19, 2022
Merged

add test for CalcExitPool #1540

merged 8 commits into from
May 19, 2022

Conversation

catShaark
Copy link
Contributor

@catShaark catShaark commented May 19, 2022

Closes: #1512

What is the purpose of the change

  • Unit test for CalcExitPool.

Brief Changelog

  • Add unit test for the generic CalcExitPool Logic in x/gamm/pool-models/internal/cfmm_common/lp.go.
  • Change this error returned by CalcExitPool when exitingShares is grater or equal to pool's total shares so that there's no Int64() out of bound panic.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not documented

@catShaark catShaark requested a review from a team May 19, 2022 04:38
@github-actions github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label May 19, 2022
@catShaark catShaark requested a review from vuong177 May 19, 2022 04:44
@catShaark catShaark marked this pull request as draft May 19, 2022 04:56
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #1540 (8d3da5b) into main (ae19a4c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1540   +/-   ##
=======================================
  Coverage   19.46%   19.46%           
=======================================
  Files         241      242    +1     
  Lines       32183    32255   +72     
=======================================
+ Hits         6264     6279   +15     
- Misses      24767    24822   +55     
- Partials     1152     1154    +2     
Impacted Files Coverage Δ
x/gamm/pool-models/internal/cfmm_common/lp.go 20.83% <100.00%> (ø)

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 ae19a4c...8d3da5b. Read the comment docs.

@catShaark catShaark marked this pull request as ready for review May 19, 2022 05:22
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Awesome work on this! The tests look great. Had a couple of minor comments, please let me know what you think

x/gamm/pool-models/internal/cfmm_common/lp_test.go Outdated Show resolved Hide resolved
x/gamm/pool-models/internal/cfmm_common/lp_test.go Outdated Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Awesome work on making the calculations for expected exit coins! Left a few minor comments

x/gamm/pool-models/internal/cfmm_common/lp_test.go Outdated Show resolved Hide resolved
x/gamm/pool-models/internal/cfmm_common/lp_test.go Outdated Show resolved Hide resolved
x/gamm/pool-models/internal/cfmm_common/lp_test.go Outdated Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM

@catShaark catShaark merged commit 583cfec into osmosis-labs:main May 19, 2022
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.

Make tests for the generic CalcExitPool logic
5 participants