Stableswap: LP logic #1419

ValarDragon · 2022-05-05T04:15:54Z

Component of: #1406

What is the purpose of the change

This PR abstracts the Balancer logic for exit pool, and JoinPoolNoSwap logic to internal code usable by both balancer and stableswap.

This PR then adds these functions to stableswap. What is missing from stableswap after this is then the logic for single asset LP'ing. I plan on doing that tomorrow, it will be more logically intense, so I was thinking of doing it in a separate PR to keep this one simple to review, if folks think that is a good plan.

Testing and Verifying

Balancer changes are covered by existing tests.
This lends some degree of confidence to their correctness within stableswap, as this code is reused.
Stableswap needs similar tests. This should imo be done by abstracting the inverse relation tests to allow more pools as input. Cref #{TODO create an issue}

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? yes
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? No
How is the feature or change documented? Needs documentation within stableswap spec

codecov-commenter · 2022-05-05T04:16:45Z

Codecov Report

Merging #1419 (ea60872) into main (f149135) will increase coverage by 0.04%.
The diff coverage is 36.04%.

@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
+ Coverage   19.50%   19.55%   +0.04%     
==========================================
  Files         231      235       +4     
  Lines       31527    31607      +80     
==========================================
+ Hits         6150     6181      +31     
- Misses      24251    24297      +46     
- Partials     1126     1129       +3

Impacted Files	Coverage Δ
x/gamm/pool-models/balancer/amm.go	`25.37% <0.00%> (+3.76%)`	⬆️
x/gamm/pool-models/stableswap/amm.go	`44.20% <0.00%> (-7.06%)`	⬇️
x/gamm/pool-models/stableswap/pool.go	`0.00% <0.00%> (ø)`
osmoutils/binary_search.go	`79.48% <79.48%> (ø)`
osmoutils/dec_helper.go	`0.00% <0.00%> (ø)`
osmoutils/cache_ctx.go	`0.00% <0.00%> (ø)`
osmoutils/cli_helpers.go	`0.00% <0.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 9463346...ea60872. Read the comment docs.

ValarDragon · 2022-05-05T04:19:35Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+		if exitAmt.GTE(asset.Amount) {
+			return sdk.Coins{}, errors.New("too many shares out")
+		}


This is new, but solves an edge case bug with potential rounding errors in the exitAmt multiplication.

thanks for adding more comments as well

srry I'm not sure I follow here, could you explain why we're comparing share amount with the actual asset amount?

exitAmt isn't a share amount. Its a ratio * assets = assets. (Ratios are unitless!)

TODO: Refactor this, to get more code reuse with existing code

ValarDragon · 2022-05-06T21:14:48Z

Next steps:

Add tests for newly introduced code:
- BinarySearch & err tolerance (a basic test, more extensive can be done in follow-up)
- SingleAssetJoin logic
SwapFee for SingleAssetJoin
Unify LP logic between Balancer and Stableswap, using more internal methods (for another PR)
Consider adding a minimum remCoins before doing a singelAssetJoin for multi-asset. (Informs decisions on [x/gamm][Misc Improvements] Fix API inconsistency between JoinPool and CalcJoinPoolShares #1443)

If this PR is growing too large, I'm happy to cut out the single asset join logic into a second PR, which is where all the complexity here is.

…into dev/stableswap_rem

p0mvn

Awesome work!

In terms of whether to split this, I think it would be great to get this approved and merged and continue the work in a separate PR / branch

p0mvn · 2022-05-07T00:46:32Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+// (by path-independence, swap all of B -> A, and then swap all of C -> A will yield same amount of A, regardless
+// of order and interleaving)
+//
+// This implementation all of pool.GetTotalPoolLiquidity, pool.ExitPool, and pool.SwapExactAmountIn to not


Suggested change

// This implementation all of pool.GetTotalPoolLiquidity, pool.ExitPool, and pool.SwapExactAmountIn to not

// This implementation includes calls to all of pool.GetTotalPoolLiquidity, pool.ExitPool, and pool.SwapExactAmountIn to not

We might want to consider just mentioning that this function updates state. Function specs shouldn't go into detail about implementation

The point is that we rely on calls to those pool functions not modifying state, only the pool struct, despite the functions taking in the context.

(So it only supports a subset of pools, which is at the moment all of our current pools. I think our DeFi plans should remove this soon)

x/gamm/pool-models/internal/cfmm_common/lp.go

p0mvn · 2022-05-07T01:02:33Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+		if exitAmt.GTE(asset.Amount) {
+			return sdk.Coins{}, errors.New("too many shares out")
+		}


thanks for adding more comments as well

x/gamm/pool-models/internal/cfmm_common/lp.go

p0mvn · 2022-05-07T03:23:36Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+
+	// Creates a pool with tokenIn liquidity added, where it created `sharesIn` number of shares.
+	// Returns how many tokens you'd get, if you then exited all of `sharesIn` for tokenIn.Denom
+	estimateCoinOutGivenShares := func(sharesIn sdk.Int) (tokenOut sdk.Int, err error) {


What do you think about making this an actual function so that it can be unit tested?

+1 on this comment!

x/gamm/pool-models/internal/cfmm_common/lp.go

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

ValarDragon · 2022-05-07T04:27:25Z

Thanks for the review on this!!

mattverse

Overall LGTM!

I'm surprised how this PR kinda puts all the particles of stable swap together! 🚀

osmoutils/binary_search.go

mattverse · 2022-05-08T11:20:37Z

osmoutils/binary_search_test.go

+
+		expectedCompareResult int8
+	}{
+		{"0 tolerance: <", ZeroErrTolerance, sdk.NewInt(1000), sdk.NewInt(1001), -1},


mattverse · 2022-05-08T11:20:41Z

osmoutils/binary_search_test.go

+func TestBinarySearch(t *testing.T) {
+	// straight line function that returns input. Simplest to binary search on,
+	// binary search directly reveals one bit of the answer in each iteration with this function.
+	lineF := func(a sdk.Int) (sdk.Int, error) {


x/gamm/pool-models/internal/cfmm_common/lp.go

mattverse · 2022-05-08T11:39:53Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+		if exitAmt.GTE(asset.Amount) {
+			return sdk.Coins{}, errors.New("too many shares out")
+		}


srry I'm not sure I follow here, could you explain why we're comparing share amount with the actual asset amount?

mattverse · 2022-05-08T11:57:34Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+	correctnessThreshold := sdk.NewInt(2)
+	maxIterations := 512
+	// upperbound of number of LP shares = existingShares * tokenIn.Amount / pool.totalLiquidity.AmountOf(tokenIn.Denom)
+	existingTokenLiquidity := pool.GetTotalPoolLiquidity(ctx).AmountOf(tokenIn.Denom)


Why do we need ctx as a parameter in the first place for GetTotalPoolLiquidity if we're using a dummy ctx? Do you think we can erase ctx for those methods in poolI that takes ctx but does not use it in a separate issue?

mattverse · 2022-05-08T12:00:08Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+
+	// Creates a pool with tokenIn liquidity added, where it created `sharesIn` number of shares.
+	// Returns how many tokens you'd get, if you then exited all of `sharesIn` for tokenIn.Denom
+	estimateCoinOutGivenShares := func(sharesIn sdk.Int) (tokenOut sdk.Int, err error) {


+1 on this comment!

mattverse · 2022-05-08T12:00:52Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+
+		return swapAllCoinsToSingleAsset(poolWithUpdatedLiquidity, ctx, exitedCoins, swapToDenom)
+	}
+	// TODO: Come back and revisit err tolerance


What's the exact task that this TODO is mentioning? Is it referring to creating test cases?

mattverse · 2022-05-08T12:22:35Z

osmoutils/binary_search.go

@@ -0,0 +1,95 @@
+package osmoutils


What's the standard for methods that go into osmoutils package and methods that stay within a specific package?

Good question. Right now I think the answer is things that seem generally re-usable, but not super well abstracted / haven't found a proper home in the code yet.

In this case, they could plausibly belong in osmomath

mconcat

Left some of comments!

osmoutils/binary_search.go

mconcat · 2022-05-09T12:54:22Z

x/gamm/pool-models/internal/cfmm_common/lp.go

+	correctnessThreshold := sdk.NewInt(2)
+	maxIterations := 512


I think 256 is enough to reduce down to single digit while dealing with Int256

Co-authored-by: Matt, Park <[email protected]>

ValarDragon added 5 commits May 4, 2022 17:28

Improve scale/descale API

ace4650

Generalize ExitPool logic, add to stableswap

5e9afb3

Refactor Maximal join to a common function

727e968

Start binary search on lp amts

da82dc7

JoinPool boilerplate

8d78683

ValarDragon requested a review from a team May 5, 2022 04:15

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

ValarDragon requested a review from mconcat May 5, 2022 04:16

ValarDragon commented May 5, 2022

View reviewed changes

ValarDragon and others added 2 commits May 5, 2022 10:56

Merge branch 'main' into dev/stableswap_rem

c0facac

Add first draft of Binary Search Single Asset Join code

e9fca05

TODO: Refactor this, to get more code reuse with existing code

ValarDragon force-pushed the dev/stableswap_rem branch from 8169edd to e9fca05 Compare May 6, 2022 05:46

Generalize out binary search logic

619b6ca

ValarDragon changed the title ~~Stableswap: Simple parts of LPing~~ Stableswap: LP logic May 6, 2022

ValarDragon and others added 2 commits May 6, 2022 16:11

Finish stableswap LP scaffolding

b170c44

Merge branch 'main' into dev/stableswap_rem

7d404bd

ValarDragon added 4 commits May 6, 2022 17:54

Binary search initial test

f87ef25

Merge branch 'dev/stableswap_rem' of github.com:osmosis-labs/osmosis …

ea2021a

…into dev/stableswap_rem

basic err tolerance test

189ffb9

ErrTolerance godoc

bf09f61

p0mvn reviewed May 7, 2022

View reviewed changes

mattverse mentioned this pull request May 8, 2022

[x/gamm][Testing] Add tests for Binary Search #1448

Closed

mattverse approved these changes May 8, 2022

View reviewed changes

mconcat reviewed May 9, 2022

View reviewed changes

ValarDragon and others added 3 commits May 9, 2022 09:59

Address some comments

3cc4177

Update osmoutils/binary_search.go

0056718

Co-authored-by: Matt, Park <[email protected]>

Merge branch 'main' into dev/stableswap_rem

6582367

ValarDragon mentioned this pull request May 16, 2022

Make tests for the generic CalcExitPool logic #1512

Closed

xBalbinus and others added 2 commits May 16, 2022 18:55

added nonzero tolerance test

44824ed

Fix go fmt

ea60872

ValarDragon merged commit f304ed8 into main May 17, 2022

ValarDragon deleted the dev/stableswap_rem branch May 17, 2022 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stableswap: LP logic #1419

Stableswap: LP logic #1419

ValarDragon commented May 5, 2022

codecov-commenter commented May 5, 2022 •

edited

Loading

ValarDragon May 5, 2022 •

edited

Loading

p0mvn May 7, 2022

mattverse May 8, 2022

ValarDragon May 16, 2022 •

edited

Loading

ValarDragon commented May 6, 2022 •

edited

Loading

p0mvn left a comment •

edited

Loading

p0mvn May 7, 2022

p0mvn May 7, 2022

ValarDragon May 7, 2022

p0mvn May 7, 2022

p0mvn May 7, 2022

mattverse May 8, 2022

ValarDragon commented May 7, 2022

mattverse left a comment

mattverse May 8, 2022

mattverse May 8, 2022

mattverse May 8, 2022

mattverse May 8, 2022

mattverse May 8, 2022

mattverse May 8, 2022

mattverse May 8, 2022

ValarDragon May 16, 2022

ValarDragon May 16, 2022

mconcat left a comment

mconcat May 9, 2022

	// This implementation all of pool.GetTotalPoolLiquidity, pool.ExitPool, and pool.SwapExactAmountIn to not
	// This implementation includes calls to all of pool.GetTotalPoolLiquidity, pool.ExitPool, and pool.SwapExactAmountIn to not

Stableswap: LP logic #1419

Stableswap: LP logic #1419

Conversation

ValarDragon commented May 5, 2022

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

codecov-commenter commented May 5, 2022 • edited Loading

Codecov Report

ValarDragon May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon May 16, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon commented May 6, 2022 • edited Loading

p0mvn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented May 7, 2022

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mconcat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 5, 2022 •

edited

Loading

ValarDragon May 5, 2022 •

edited

Loading

ValarDragon May 16, 2022 •

edited

Loading

ValarDragon commented May 6, 2022 •

edited

Loading

p0mvn left a comment •

edited

Loading