Generalize gamm Inverse relationship test , add Inverse relationship test for stableswap #1456

mattverse · 2022-05-09T15:20:22Z

Closes: #1371

What is the purpose of the change

Upon adding invariant test for inverse relationship on CalculateAmountOutAndIn(https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/pool-models/balancer/amm_test.go#L89-L216) the goal of this PR is to generalize the test case, and also add the same test for stableswap using the generalized test case.

*Add a description of the overall background and high level changes that this PR introduces *

(E.g.: This pull request improves documation of area A by adding ....

Testing and Verifying

No testings changed in balancer package, inverse relationship test added for stableswap

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 applicable

codecov-commenter · 2022-05-09T15:27:37Z

Codecov Report

Merging #1456 (8d65ee8) into main (f56fbe5) will decrease coverage by 0.51%.
The diff coverage is 16.26%.

@@            Coverage Diff             @@
##             main    #1456      +/-   ##
==========================================
- Coverage   19.82%   19.31%   -0.52%     
==========================================
  Files         202      229      +27     
  Lines       27685    31452    +3767     
==========================================
+ Hits         5489     6075     +586     
- Misses      21175    24269    +3094     
- Partials     1021     1108      +87

Impacted Files	Coverage Δ
x/epochs/client/cli/query.go	`0.00% <ø> (ø)`
x/gamm/client/cli/query.go	`36.74% <0.00%> (-0.36%)`	⬇️
x/gamm/pool-models/stableswap/msgs.go	`0.00% <0.00%> (ø)`
x/gamm/pool-models/stableswap/pool.go	`0.00% <0.00%> (ø)`
x/gamm/pool-models/stableswap/stableswap_pool.go	`0.00% <0.00%> (ø)`
.../gamm/pool-models/stableswap/stableswap_pool.pb.go	`0.58% <0.00%> (-0.09%)`	⬇️
x/gamm/types/pool.go	`0.00% <ø> (ø)`
x/incentives/client/cli/query.go	`0.00% <ø> (ø)`
x/incentives/keeper/distribute.go	`61.00% <0.00%> (ø)`
x/incentives/keeper/hooks.go	`50.00% <0.00%> (ø)`
... and 53 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 de69b2a...8d65ee8. Read the comment docs.

p0mvn

Great work @xBalbinus @mattverse ! Left some comments, please take a look

x/gamm/pool-models/internal/test_helpers/test_helpers.go

x/gamm/pool-models/stableswap/amm_test.go

app/apptesting/test_suite.go

x/gamm/pool-models/balancer/amm_test.go

x/gamm/pool-models/stableswap/util_test.go

Co-authored-by: Roman <[email protected]>

stackman27 · 2022-05-13T00:27:29Z

Closes #1349?

x/gamm/pool-models/stableswap/amm_test.go

mattverse · 2022-05-25T13:47:45Z

I didn't notice this PR was open for so long, just made the changes you've requested per last review can you review the updates? tysm!

x/gamm/pool-models/balancer/amm_test.go

alexanderbez · 2022-05-25T21:59:58Z

x/gamm/pool-models/internal/test_helpers/test_helpers.go

+	"github.com/osmosis-labs/osmosis/v7/x/gamm/types"
+)
+
+type CfmmCommonTestSuite struct {


can we add a small godoc to what CFMM abbreviation is?

x/gamm/pool-models/internal/test_helpers/test_helpers.go

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

ValarDragon · 2022-06-17T04:42:52Z

x/gamm/pool-models/internal/test_helpers/test_helpers.go

+// CfmmCommonTestSuite is the common test suite struct of Constant Function Market Maker,
+// that pool-models can inherit from.
+type CfmmCommonTestSuite struct {
+	suite.Suite


Why aren't we just using apptesting here?

We could also delete the CreateTestContext if we used apptesting

It's because we have import cycle problems when we do that since the apptesting package imports gamm module and the balancer module

ValarDragon · 2022-07-01T03:39:57Z

uhh, could you take a stab at solving the merge conflict here @mattverse , then we can get it merged in?

mattverse · 2022-07-01T06:06:24Z

np! Thanks for getting this reviewed! Merging once final CI checks have passed upon approval

mattverse added 2 commits May 9, 2022 20:54

Generalize inverse testing from balancer package to pool-models

01a4791

Add stableswap inverse testing

3714472

mattverse requested a review from a team May 9, 2022 15:20

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label May 9, 2022

xBalbinus and others added 3 commits May 9, 2022 18:13

Merge branch 'main' into mattverse/inverse-testing

41c8cf5

fixed failing lint test

17afdd2

fixed failing lint test

9488499

mattverse mentioned this pull request May 10, 2022

chore: auto add pull requests to project board #1454

Merged

p0mvn reviewed May 10, 2022

View reviewed changes

Update x/gamm/pool-models/internal/test_helpers/test_helpers.go

8d65ee8

Co-authored-by: Roman <[email protected]>

p0mvn mentioned this pull request May 11, 2022

refactor: stableswap inverse tests to be consistent with the stableswap pool #1470

Closed

stackman27 reviewed May 21, 2022

View reviewed changes

x/gamm/pool-models/stableswap/amm_test.go Show resolved Hide resolved

stackman27 mentioned this pull request May 22, 2022

Modified stableswap amm.test to accomodate Scaling Factor #1560

Closed

Add test

af6c51d

mattverse requested a review from p0mvn May 25, 2022 13:47

alexanderbez reviewed May 25, 2022

View reviewed changes

Address Bez's review

f6c8cb2

mattverse requested a review from alexanderbez May 27, 2022 07:14

ValarDragon self-requested a review June 2, 2022 15:43

Merge branch 'main' into bez/1225-pool-api-refactor

663af26

github-actions bot added the C:app-wiring Changes to the app folder label Jun 16, 2022

Fix test

b6718ab

alexanderbez approved these changes Jun 16, 2022

View reviewed changes

x/gamm/pool-models/internal/test_helpers/test_helpers.go Outdated Show resolved Hide resolved

Update x/gamm/pool-models/internal/test_helpers/test_helpers.go

09ccf6d

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

ValarDragon reviewed Jun 17, 2022

View reviewed changes

ValarDragon approved these changes Jul 1, 2022

View reviewed changes

Merge branch 'main']

4397838

mattverse merged commit 2007bab into main Jul 1, 2022

mattverse deleted the mattverse/inverse-testing branch July 1, 2022 07:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize gamm Inverse relationship test , add Inverse relationship test for stableswap #1456

Generalize gamm Inverse relationship test , add Inverse relationship test for stableswap #1456

mattverse commented May 9, 2022

codecov-commenter commented May 9, 2022 •

edited

Loading

p0mvn left a comment

stackman27 commented May 13, 2022

mattverse commented May 25, 2022

alexanderbez May 25, 2022

ValarDragon Jun 17, 2022

ValarDragon Jun 17, 2022

mattverse Jun 17, 2022 •

edited

Loading

ValarDragon commented Jul 1, 2022

mattverse commented Jul 1, 2022

Generalize gamm Inverse relationship test , add Inverse relationship test for stableswap #1456

Generalize gamm Inverse relationship test , add Inverse relationship test for stableswap #1456

Conversation

mattverse commented May 9, 2022

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

codecov-commenter commented May 9, 2022 • edited Loading

Codecov Report

p0mvn left a comment

Choose a reason for hiding this comment

stackman27 commented May 13, 2022

mattverse commented May 25, 2022

alexanderbez May 25, 2022

Choose a reason for hiding this comment

ValarDragon Jun 17, 2022

Choose a reason for hiding this comment

ValarDragon Jun 17, 2022

Choose a reason for hiding this comment

mattverse Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon commented Jul 1, 2022

mattverse commented Jul 1, 2022

codecov-commenter commented May 9, 2022 •

edited

Loading

mattverse Jun 17, 2022 •

edited

Loading