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

[x/gamm][Testing] generalize TestCalculateAmountOutAndIn_InverseRelationship #1371

Closed
Tracked by #1452
p0mvn opened this issue Apr 28, 2022 · 4 comments · Fixed by #1456
Closed
Tracked by #1452

[x/gamm][Testing] generalize TestCalculateAmountOutAndIn_InverseRelationship #1371

p0mvn opened this issue Apr 28, 2022 · 4 comments · Fixed by #1456
Assignees
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:tests

Comments

@p0mvn
Copy link
Member

p0mvn commented Apr 28, 2022

Background

The following PRs contributed to TestCalculateAmountOutAndIn_InverseRelationship in balancer package:

From "Gamm Refactor", CalcInAmtGivenOut and CalcOutAmtGivenIn are defined on the PoolI interface.

There are 2 pool implementations for PoolI: balancer and stableswap. The previously added tests only test balancer at the moment. However, since the inverse relationship applies to both balancer and stableswap, these tests can be generalized to apply to all possible implementations.

Suggested Design

  • create a test file in x/gamm/pool-models
  • extract TestCalculateAmountOutAndIn_InverseRelationship test from x/gamm/pool-models/balancer to x/gamm/pool-models
  • generalize the test to apply to both balancer and stableswap pool implementations

Acceptance Criteria

  • TestCalculateAmountOutAndIn_InverseRelationship tests both pool implementations
  • the test is easily extensible to add more implementations if we have any in the future
  • all new and existing unit tests pass
@daniel-farina daniel-farina moved this to 🔍 Needs Review in Osmosis Chain Development Apr 28, 2022
@p0mvn p0mvn added T:tests C:x/gamm Changes, features and bugs related to the gamm module. labels Apr 28, 2022
@p0mvn
Copy link
Member Author

p0mvn commented Apr 28, 2022

@xBalbinus would you be interested in working on this? Thanks for offering to help with the tests

@xBalbinus
Copy link
Contributor

Yo @p0mvn ! Of course! Taking a look.

@p0mvn
Copy link
Member Author

p0mvn commented May 9, 2022

From @ValarDragon :

Recently, great tests have been added that test certain invariants (in the tests called "inverse relations") that are expected to hold trrue for any pool: https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/pool-models/balancer/amm_test.go#L89-L216

We should make code reuse possible for using the same core logic in stableswap tests. Theres a couple ways this can be done:

I suggest we do the following:

  • Make a general function, perhaps in pool-models/internal/test_helpers that defines a testcase struct, which takes in a PoolI and some input to try the invariant relation on
  • Define testcases in amm_test.go (in the case of balancer, adapt the existing tests, to use a single function call to generate the pool)
    • This should not increase the length of testcase definition notably. Ideally its a one or two liner.
  • Add the invariant relation tests to stableswap

There are alternative approaches if for some reason this produces extra overheads though.

@p0mvn p0mvn changed the title test: generalize TestCalculateAmountOutAndIn_InverseRelationship [x/gamm][Testing] generalize TestCalculateAmountOutAndIn_InverseRelationship May 9, 2022
@p0mvn p0mvn mentioned this issue May 9, 2022
7 tasks
@mattverse mattverse self-assigned this May 9, 2022
@xBalbinus
Copy link
Contributor

^ Huge shoutout to @mattverse for helping with this PR, I also fixed a small thing to keep the linter running. All tests are passing & ready for merge upon approval.

Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development May 31, 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. T:tests
Projects
Archived in project
3 participants