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][stableswap]: Add tests for core pool joining logic (JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal) #2346

Closed
Tracked by #1451
AlpinYukseloglu opened this issue Aug 10, 2022 · 2 comments
Assignees
Labels
C:x/gamm Changes, features and bugs related to the gamm module. T:tests

Comments

@AlpinYukseloglu
Copy link
Contributor

AlpinYukseloglu commented Aug 10, 2022

Background

Our current implementation of stableswap does not have tests for the core logic related to pool joining. Most of the core logic is in CalcJoinPoolShares, so I focused my example test cases/categories mainly on that function. My suggestions below should get this core logic in the ballpark of what we have for our balancer tests, but we should still make an effort to cover as many additional edge cases as possible.

Core logic to be tested:

func (p *Pool) joinPoolSharesInternal(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, newLiquidity sdk.Coins, err error) {
if len(tokensIn) == 1 {
numShares, err = p.calcSingleAssetJoinShares(tokensIn[0], swapFee)
newLiquidity = tokensIn
return numShares, newLiquidity, err
} else if len(tokensIn) != p.NumAssets() || !tokensIn.DenomsSubsetOf(p.GetTotalPoolLiquidity(ctx)) {
return sdk.ZeroInt(), sdk.NewCoins(), errors.New(
"stableswap pool only supports LP'ing with one asset, or all assets in pool")
}
// Add all exact coins we can (no swap). ctx arg doesn't matter for Stableswap
numShares, remCoins, err := cfmm_common.MaximalExactRatioJoin(p, sdk.Context{}, tokensIn)
if err != nil {
return sdk.ZeroInt(), sdk.NewCoins(), err
}
p.updatePoolForJoin(tokensIn.Sub(remCoins), numShares)
for _, coin := range remCoins {
// TODO: Perhaps add a method to skip if this is too small.
newShare, err := p.calcSingleAssetJoinShares(coin, swapFee)
if err != nil {
return sdk.ZeroInt(), sdk.NewCoins(), err
}
p.updatePoolForJoin(sdk.NewCoins(coin), newShare)
numShares = numShares.Add(newShare)
}
return numShares, tokensIn, nil
}

Suggested Design

Core cases to test:

  • single asset joins
  • multi asset exact ratio joins
  • multi asset non-exact ratio joins (both with large difference and with dust)

Specific things to test/errors to catch:

  • invalid length of tokensin (greater than 1, less than pool size should fail)
  • liquidity updates (keep in mind that CalcJoinPoolShares updates pool state in stableswap, unlike our balancer implementation)
  • correct number of output shares
  • for CalcJoinPoolShares (which just calls joinPoolSharesInternal), the number of tokens joined (both vs. expected amount and vs. actual tokens added to pool)

Acceptance Criteria

  • Existing tests pass and mutation tests on joinPoolSharesInternal show full coverage
@osmo-bot osmo-bot moved this to Needs Review 🔍 in Osmosis Chain Development Aug 10, 2022
@AlpinYukseloglu AlpinYukseloglu added C:x/gamm Changes, features and bugs related to the gamm module. T:tests labels Aug 10, 2022
@AlpinYukseloglu AlpinYukseloglu changed the title [x/gamm][StableSwap] Add tests for core JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal logic stableswap: Add tests for core JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal logic Aug 10, 2022
@AlpinYukseloglu AlpinYukseloglu changed the title stableswap: Add tests for core JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal logic stableswap: Add tests for core pool joining logic (JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal) Aug 10, 2022
@AlpinYukseloglu AlpinYukseloglu changed the title stableswap: Add tests for core pool joining logic (JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal) [x/gamm][stableswap]: Investigate critical precision issue with binary search CFMM solver: Add tests for core pool joining logic (JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal) Aug 10, 2022
@AlpinYukseloglu AlpinYukseloglu changed the title [x/gamm][stableswap]: Investigate critical precision issue with binary search CFMM solver: Add tests for core pool joining logic (JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal) [x/gamm][stableswap]: Add tests for core pool joining logic (JoinPool, CalcJoinPoolShares, and joinPoolSharesInternal) Aug 25, 2022
@AlpinYukseloglu
Copy link
Contributor Author

k derivation for tests: #1368 (comment)

@AlpinYukseloglu
Copy link
Contributor Author

Some multi-asset joins are blocked on precision increase in #3008

Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development Oct 16, 2022
Repository owner moved this from Done ✅ to In Progress🏃 in Osmosis Chain Development Oct 16, 2022
Repository owner moved this from In Progress🏃 to Done ✅ in Osmosis Chain Development Nov 5, 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
Development

No branches or pull requests

1 participant