Refactor: Change Pool API to have separate methods for `JoinPool` and `JoinPoolNoSwap` #2133

AlpinYukseloglu · 2022-07-19T01:28:46Z

Closes: #2128

What is the purpose of the change

Add JoinPoolNoSwap and CalcJoinPoolNoSwapShares to our GAMM pool interface to have a clear and accessible flow for no-swap joins.

Brief Changelog

Add JoinPoolNoSwap and CalcJoinPoolNoSwapShares to GAMM pool interface
Implement JoinPoolNoSwap and CalcJoinPoolNoSwapShares in balancer package
Route JoinPoolNoSwap in pool_service.go to the new JoinPoolNoSwap in our pool interface
Add tests for new balancer methods

TODO:

Create issue for stableswap implementation

Testing and Verifying

This change added tests and can be verified as follows:

TestCalcJoinPoolNoSwapShares in balancer's pool_test.go

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? (yes)
How is the feature or change documented? (not documented)

…alancer

AlpinYukseloglu · 2022-07-19T02:10:31Z

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

 		return numShares, tokensJoined, nil
 	}

 	// 4) Still more coins to join, so we update the effective pool state here to account for
 	// join that just happened.
 	// * We add the joined coins to our "current pool liquidity" object (poolAssetsByDenom)
 	// * We increment a variable for our "newTotalShares" to add in the shares that've been added.
-	tokensJoined = tokensIn.Sub(remainingTokensIn)


Context: CalcJoinPoolNoSwapShares includes this calculation and returns the resulting tokensJoined above

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

mattverse

Feature LGTM!
Left some comments, please take a look!

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

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

p0mvn

LGTM overall - would like to suggest adding new test cases

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

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

…osmosis into separate-join-pool

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

…osmosis into separate-join-pool

github-actions · 2022-08-05T00:01:58Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

ValarDragon · 2022-08-22T15:27:04Z

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

+	p.IncreaseLiquidity(numShares, newLiquidity)
+	return numShares, nil
+}
+
 // CalcJoinPoolShares calculates the number of shares created to join pool with the provided amount of `tokenIn`.


So now this method is basically not called in the codebase anywhere, right?

CalcJoinPoolShares? Do you mean calls outside of the module? Otherwise it's called in JoinPool

But JoinPool is no longer called in the state machien right?

I might be missing your point here but I believe JoinPool is still called in JoinSwapExactAmountIn:

osmosis/x/gamm/keeper/pool_service.go

Line 272 in 16e3e4b

sharesOut, err := pool.JoinPool(ctx, tokensIn, pool.GetSwapFee(ctx))

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

p0mvn · 2022-08-30T20:00:18Z

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

+	// get all 'pool assets' (aka current pool liquidity + balancer weight)
+	poolAssetsByDenom, err := getPoolAssetsByDenom(p.GetAllPoolAssets())
+	if err != nil {
+		return sdk.ZeroInt(), sdk.NewCoins(), tokensIn, err


I'm just going to point out which branches are not covered by tests.

@AlpinYukseloglu do you mind confirming please if it is possible to introduce test cases to cover these

This is the first branch

This one should actually not be possible as it only fails when there are duplicate assets in a pool and it's not possible to have duplicate denoms in a valid coin set. The error check is just here as a best practice.

Is it due to using a balancer pool constructor in TestCalcJoinPoolNoSwapShares where the constructor performs this validation?

What if we avoid it using it to trigger an invalid state and achieve full coverage?

We should be able to:

balancerPool := balancer.Pool{ Address: types.NewPoolAddress(defaultPoolId).String(), Id: defaultPoolId, PoolParams: balancer.PoolParams{SwapFee: defaultSwapFee, ExitFee: defaultExitFee}, PoolAssets: test.poolAssets, FuturePoolGovernor: defaultFutureGovernor, TotalShares: sdk.NewCoin(types.GetPoolShareDenom(defaultPoolId), types.InitPoolSharesSupply), }

Now, we can force duplicate pool assets

Please see the most recent case I added. It is still technically a trivial test case because duplicate pool assets are caught in multiple places throughout this function and ultimately end up in a division by zero even if you comment out all of them, but it should be covered now.

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

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

p0mvn

LGTM. Great work on this @AlpinYukseloglu . Thank you for maximizing test coverage.

The remaining comments are non-blocking. However, could you please double-check the following:

the error message about "no swap joins requiring all assets in pool". I just want to ensure that this is not exclusive of "joins with swaps" because CalcJoinPoolNoSwapShares is called from CalcJoinPoolShares.
the new empty sdk.Context being passed in to MaximalExactRatioJoin from CalcJoinPoolNoSwapShares

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

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

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

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

p0mvn · 2022-09-06T14:15:59Z

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

+
+	// ensure that there aren't too many or too few assets in `tokensIn`
+	if tokensIn.Len() != p.NumAssets() {
+		return sdk.ZeroInt(), sdk.NewCoins(), errors.New("no-swap joins require LP'ing with all assets in pool")


Just trying to understand:

Why do you emphasize "no-swap joins"?

CalcJoinPoolNoSwapShares is called from CalcJoinPoolShares which is called from regular JoinPool that does swaps. I also noticed a similar check n CalcJoinPoolShares.

My point is that it seems that this is relevant for joins with the swaps as well.

This specific function is for no-swap joins, so the errors returned by it specify that it is a no-swap pool join (this specific line wouldn't error if this was a join with swaps)

Even though CalcJoinPoolNoSwapShares is still called in CalcJoinPoolShares, it is called in the "no-swap"/"all-asset"/"exact ratio" join component of it, and all pool join logic related to swaps is handled elsewhere in that function

Awesome, thank you! Just wanted to make sure that this check was intentional to have on the path for regular joins as well

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

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

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

…osmosis into separate-join-pool

… `JoinPoolNoSwap` (#2133) * split out no swap joins on pool interface and implement changes for balancer * minor refactor and comment changes for clarity * minor comment changes for review clarity * add tests for new calcs * fix merge-related issues * add changelog entry * add failure case test and make existing tests more robust * clean up validation logic * factor out common denom check and add tests for it * add additional test cases * apply new internal fx where relevant and ensure proper testing * add test case and improve test readability * add tests for edge cases and clean up existing tests * add duplicate pool edge case test * minor comment updates from code review Co-authored-by: Roman <[email protected]> * remove remainingCoins return value from internal no-swap calcs * update mocks * move ensuredenominpool test to amm tests * minor comment updates from code review Co-authored-by: Roman <[email protected]> * pass ctx through no-swap pool joins * add multi asset and edge case tests to calcnoswap Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Roman <[email protected]> (cherry picked from commit 4de8bdf)

… `JoinPoolNoSwap` (#2133) (#2634) * split out no swap joins on pool interface and implement changes for balancer * minor refactor and comment changes for clarity * minor comment changes for review clarity * add tests for new calcs * fix merge-related issues * add changelog entry * add failure case test and make existing tests more robust * clean up validation logic * factor out common denom check and add tests for it * add additional test cases * apply new internal fx where relevant and ensure proper testing * add test case and improve test readability * add tests for edge cases and clean up existing tests * add duplicate pool edge case test * minor comment updates from code review Co-authored-by: Roman <[email protected]> * remove remainingCoins return value from internal no-swap calcs * update mocks * move ensuredenominpool test to amm tests * minor comment updates from code review Co-authored-by: Roman <[email protected]> * pass ctx through no-swap pool joins * add multi asset and edge case tests to calcnoswap Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Roman <[email protected]> (cherry picked from commit 4de8bdf) Co-authored-by: alpo <[email protected]> Co-authored-by: alpo <[email protected]>

AlpinYukseloglu added 3 commits July 18, 2022 18:05

split out no swap joins on pool interface and implement changes for b…

9f86b96

…alancer

minor refactor and comment changes for clarity

3cb4b3b

minor comment changes for review clarity

654fdf4

AlpinYukseloglu added the C:x/gamm Changes, features and bugs related to the gamm module. label Jul 19, 2022

AlpinYukseloglu added 3 commits July 18, 2022 18:54

add tests for new calcs

019e3e5

Merge branch 'main' into separate-join-pool

1fdf342

fix merge-related issues

5633945

AlpinYukseloglu requested review from ValarDragon and p0mvn July 19, 2022 02:08

AlpinYukseloglu marked this pull request as ready for review July 19, 2022 02:09

AlpinYukseloglu requested a review from a team July 19, 2022 02:09

AlpinYukseloglu commented Jul 19, 2022

View reviewed changes

add changelog entry

f1a00eb

github-actions bot added the C:docs Improvements or additions to documentation label Jul 19, 2022

mattverse reviewed Jul 19, 2022

View reviewed changes

x/gamm/pool-models/balancer/pool_test.go Show resolved Hide resolved

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

p0mvn reviewed Jul 19, 2022

View reviewed changes

x/gamm/pool-models/balancer/pool_test.go Show resolved Hide resolved

x/gamm/pool-models/balancer/pool.go Show resolved Hide resolved

AlpinYukseloglu and others added 3 commits July 19, 2022 19:51

add failure case test and make existing tests more robust

08d2b8e

Merge branch 'separate-join-pool' of https://github.com/osmosis-labs/…

eebb7cc

…osmosis into separate-join-pool

Merge branch 'main' into separate-join-pool

43772e7

AlpinYukseloglu requested review from mattverse and p0mvn July 20, 2022 02:59

ValarDragon reviewed Jul 20, 2022

View reviewed changes

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

ValarDragon added the A:backport/v10.x backport patches to v10.x branch label Jul 20, 2022

AlpinYukseloglu and others added 3 commits July 20, 2022 10:09

clean up validation logic

79e4232

Merge branch 'separate-join-pool' of https://github.com/osmosis-labs/…

82ff209

…osmosis into separate-join-pool

Merge branch 'main' into separate-join-pool

a251f7c

github-actions bot added the Stale label Aug 5, 2022

AlpinYukseloglu added 2 commits August 8, 2022 15:00

factor out common denom check and add tests for it

a370c6e

add additional test cases

dcbb25a

ValarDragon reviewed Aug 22, 2022

View reviewed changes

AlpinYukseloglu requested a review from ValarDragon August 25, 2022 11:12

ValarDragon self-assigned this Aug 29, 2022

p0mvn reviewed Aug 30, 2022

View reviewed changes

czarcas7ic added this to the V12 Blockers milestone Aug 30, 2022

p0mvn self-assigned this Aug 30, 2022

add tests for edge cases and clean up existing tests

4797f8f

AlpinYukseloglu requested a review from p0mvn August 31, 2022 02:16

p0mvn reviewed Aug 31, 2022

View reviewed changes

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

AlpinYukseloglu and others added 5 commits August 31, 2022 14:09

add duplicate pool edge case test

eda585c

minor comment updates from code review

0e5f5fa

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

remove remainingCoins return value from internal no-swap calcs

1eae187

update mocks

8b6f485

Merge branch 'main' into separate-join-pool

6147129

AlpinYukseloglu requested a review from p0mvn September 6, 2022 09:10

AlpinYukseloglu mentioned this pull request Sep 6, 2022

x/gamm: add direct unit tests for balancer JoinPool and JoinPoolNoSwap #2616

Merged

p0mvn approved these changes Sep 6, 2022

View reviewed changes

AlpinYukseloglu and others added 6 commits September 6, 2022 18:55

move ensuredenominpool test to amm tests

0e55e9d

minor comment updates from code review

005ae6a

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

Merge branch 'separate-join-pool' of https://github.com/osmosis-labs/…

096d5b2

…osmosis into separate-join-pool

pass ctx through no-swap pool joins

2f53651

add multi asset and edge case tests to calcnoswap

d91e0b5

Merge branch 'main' into separate-join-pool

0ef8a99

p0mvn approved these changes Sep 7, 2022

View reviewed changes

p0mvn added the A:backport/v12.x backport patches to v12.x branch label Sep 7, 2022

p0mvn merged commit 4de8bdf into main Sep 7, 2022

p0mvn deleted the separate-join-pool branch September 7, 2022 17:03

mergify bot mentioned this pull request Sep 7, 2022

Refactor: Change Pool API to have separate methods for JoinPool and JoinPoolNoSwap (backport #2133) #2634

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Change Pool API to have separate methods for `JoinPool` and `JoinPoolNoSwap` #2133

Refactor: Change Pool API to have separate methods for `JoinPool` and `JoinPoolNoSwap` #2133

AlpinYukseloglu commented Jul 19, 2022 •

edited

Loading

AlpinYukseloglu Jul 19, 2022

mattverse left a comment

p0mvn left a comment

github-actions bot commented Aug 5, 2022

ValarDragon Aug 22, 2022

AlpinYukseloglu Aug 22, 2022

ValarDragon Aug 23, 2022

AlpinYukseloglu Aug 25, 2022

p0mvn Aug 30, 2022

AlpinYukseloglu Aug 30, 2022

p0mvn Aug 31, 2022

AlpinYukseloglu Aug 31, 2022

p0mvn left a comment

p0mvn Sep 6, 2022

AlpinYukseloglu Sep 7, 2022

p0mvn Sep 7, 2022

Refactor: Change Pool API to have separate methods for JoinPool and JoinPoolNoSwap #2133

Refactor: Change Pool API to have separate methods for JoinPool and JoinPoolNoSwap #2133

Conversation

AlpinYukseloglu commented Jul 19, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2022

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

p0mvn 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

Refactor: Change Pool API to have separate methods for `JoinPool` and `JoinPoolNoSwap` #2133

Refactor: Change Pool API to have separate methods for `JoinPool` and `JoinPoolNoSwap` #2133

AlpinYukseloglu commented Jul 19, 2022 •

edited

Loading