Refactor: Change Pool API to have separate methods for JoinPool and…

… `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]>
osmosis-labs · Sep 7, 2022 · 4de8bdf · 4de8bdf
1 parent 5380f62
commit 4de8bdf
Show file tree

Hide file tree

Showing 11 changed files with 380 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -77,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#1667](https://github.com/osmosis-labs/osmosis/pull/1673) Move wasm-bindings code out of app package into its own root level package.
 * [#2013](https://github.com/osmosis-labs/osmosis/pull/2013) Make `SetParams`, `SetPool`, `SetTotalLiquidity`, and `SetDenomLiquidity` GAMM APIs private
 * [#1857](https://github.com/osmosis-labs/osmosis/pull/1857) x/mint rename GetLastHalvenEpochNum to GetLastReductionEpochNum
+* [#2133](https://github.com/osmosis-labs/osmosis/pull/2133) Add `JoinPoolNoSwap` and `CalcJoinPoolNoSwapShares` to GAMM pool interface and route `JoinPoolNoSwap` in pool_service.go to new method in pool interface
 * [#2353](https://github.com/osmosis-labs/osmosis/pull/2353) Re-enable stargate query via whitelsit
 * [#2394](https://github.com/osmosis-labs/osmosis/pull/2394) Remove unused interface methods from expected keepers of each module
 * [#2390](https://github.com/osmosis-labs/osmosis/pull/2390) x/mint remove unused mintCoins parameter from AfterDistributeMintedCoin

diff --git a/tests/mocks/mock_pool.go b/tests/mocks/mock_pool.go
diff --git a/x/gamm/keeper/pool_service.go b/x/gamm/keeper/pool_service.go
@@ -236,7 +236,7 @@ func (k Keeper) JoinPoolNoSwap(
 		}
 	}
 
-	sharesOut, err = pool.JoinPool(ctx, neededLpLiquidity, pool.GetSwapFee(ctx))
+	sharesOut, err = pool.JoinPoolNoSwap(ctx, neededLpLiquidity, pool.GetSwapFee(ctx))
 	if err != nil {
 		return nil, sdk.ZeroInt(), err
 	}

diff --git a/x/gamm/pool-models/balancer/amm.go b/x/gamm/pool-models/balancer/amm.go
@@ -228,3 +228,15 @@ func calcPoolSharesInGivenSingleAssetOut(
 	sharesInFeeIncluded := sharesIn.Quo(sdk.OneDec().Sub(exitFee))
 	return sharesInFeeIncluded
 }
+
+// ensureDenomInPool check to make sure the input denoms exist in the provided pool asset map
+func ensureDenomInPool(poolAssetsByDenom map[string]PoolAsset, tokensIn sdk.Coins) error {
+	for _, coin := range tokensIn {
+		_, ok := poolAssetsByDenom[coin.Denom]
+		if !ok {
+			return sdkerrors.Wrapf(types.ErrDenomNotFoundInPool, invalidInputDenomsErrFormat, coin.Denom)
+		}
+	}
+
+	return nil
+}
diff --git a/x/gamm/pool-models/balancer/amm_test.go b/x/gamm/pool-models/balancer/amm_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/osmosis-labs/osmosis/v12/x/gamm/pool-models/balancer"
 	"github.com/osmosis-labs/osmosis/v12/x/gamm/pool-models/internal/test_helpers"
+  "github.com/osmosis-labs/osmosis/v12/x/gamm/types"
 )
 
 type BalancerTestSuite struct {
@@ -55,3 +56,48 @@ func TestBalancerPoolParams(t *testing.T) {
 		}
 	}
 }
+
+func (suite *KeeperTestSuite) TestEnsureDenomInPool() {
+	tests := map[string]struct {
+		poolAssets  []balancer.PoolAsset
+		tokensIn    sdk.Coins
+		expectPass  bool
+		expectedErr error
+	}{
+		"all of tokensIn is in pool asset map": {
+			poolAssets:  []balancer.PoolAsset{defaultOsmoPoolAsset, defaultAtomPoolAsset},
+			tokensIn:    sdk.NewCoins(sdk.NewCoin("uatom", sdk.OneInt())),
+			expectPass:  true,
+			expectedErr: nil,
+		},
+		"one of tokensIn is in pool asset map": {
+			poolAssets:  []balancer.PoolAsset{defaultOsmoPoolAsset, defaultAtomPoolAsset},
+			tokensIn:    sdk.NewCoins(sdk.NewCoin("uatom", sdk.OneInt()), sdk.NewCoin("foo", sdk.OneInt())),
+			expectPass:  false,
+			expectedErr: types.ErrDenomNotFoundInPool,
+		},
+		"none of tokensIn is in pool asset map": {
+			poolAssets:  []balancer.PoolAsset{defaultOsmoPoolAsset, defaultAtomPoolAsset},
+			tokensIn:    sdk.NewCoins(sdk.NewCoin("foo", sdk.OneInt())),
+			expectPass:  false,
+			expectedErr: types.ErrDenomNotFoundInPool,
+		},
+	}
+
+	for name, tc := range tests {
+		suite.Run(name, func() {
+			suite.SetupTest()
+
+			poolAssetsByDenom, err := balancer.GetPoolAssetsByDenom(tc.poolAssets)
+			suite.Require().NoError(err, "test: %s", name)
+
+			err = balancer.EnsureDenomInPool(poolAssetsByDenom, tc.tokensIn)
+
+			if tc.expectPass {
+				suite.Require().NoError(err, "test: %s", name)
+			} else {
+				suite.Require().ErrorIs(err, tc.expectedErr, "test: %s", name)
+			}
+		})
+	}
+}
diff --git a/x/gamm/pool-models/balancer/export_test.go b/x/gamm/pool-models/balancer/export_test.go
@@ -15,6 +15,7 @@ var (
 	UpdateIntermediaryPoolAssetsLiquidity = updateIntermediaryPoolAssetsLiquidity
 
 	GetPoolAssetsByDenom = getPoolAssetsByDenom
+	EnsureDenomInPool    = ensureDenomInPool
 )
 
 func (p *Pool) CalcSingleAssetJoin(tokenIn sdk.Coin, swapFee sdk.Dec, tokenInPoolAsset PoolAsset, totalShares sdk.Int) (numShares sdk.Int, err error) {

diff --git a/x/gamm/pool-models/balancer/pool.go b/x/gamm/pool-models/balancer/pool.go
@@ -661,6 +661,19 @@ func (p *Pool) JoinPool(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (n
 	return numShares, nil
 }
 
+// JoinPoolNoSwap calculates the number of shares needed for an all-asset join given tokensIn with swapFee applied.
+// It updates the liquidity if the pool is joined successfully. If not, returns error.
+func (p *Pool) JoinPoolNoSwap(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, err error) {
+	numShares, tokensJoined, err := p.CalcJoinPoolNoSwapShares(ctx, tokensIn, swapFee)
+	if err != nil {
+		return sdk.Int{}, err
+	}
+
+	// update pool with the calculated share and liquidity needed to join pool
+	p.IncreaseLiquidity(numShares, tokensJoined)
+	return numShares, nil
+}
+
 // CalcJoinPoolShares calculates the number of shares created to join pool with the provided amount of `tokenIn`.
 // The input tokens must either be:
 // - a single token
@@ -687,12 +700,10 @@ func (p *Pool) CalcJoinPoolShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee s
 		return sdk.ZeroInt(), sdk.NewCoins(), err
 	}
 
-	// check to make sure the input denoms exist in the pool
-	for _, coin := range tokensIn {
-		_, ok := poolAssetsByDenom[coin.Denom]
-		if !ok {
-			return sdk.ZeroInt(), sdk.NewCoins(), sdkerrors.Wrapf(types.ErrDenomAlreadyInPool, invalidInputDenomsErrFormat, coin.Denom)
-		}
+	// check to make sure the input denom exists in the pool
+	err = ensureDenomInPool(poolAssetsByDenom, tokensIn)
+	if err != nil {
+		return sdk.ZeroInt(), sdk.NewCoins(), err
 	}
 
 	totalShares := p.GetTotalShares()
@@ -713,12 +724,15 @@ func (p *Pool) CalcJoinPoolShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee s
 	// * numShares is how many shares are perfectly matched.
 	// * remainingTokensIn is how many coins we have left to join, that have not already been used.
 	// if remaining coins is empty, logic is done (we joined all tokensIn)
-	numShares, remainingTokensIn, err := cfmm_common.MaximalExactRatioJoin(p, sdk.Context{}, tokensIn)
+	numShares, tokensJoined, err = p.CalcJoinPoolNoSwapShares(ctx, tokensIn, swapFee)
 	if err != nil {
 		return sdk.ZeroInt(), sdk.NewCoins(), err
 	}
-	if remainingTokensIn.Empty() {
-		tokensJoined = tokensIn
+
+	// safely ends the calculation if all input tokens are successfully LP'd
+	if tokensJoined.IsAnyGT(tokensIn) {
+		return sdk.ZeroInt(), sdk.NewCoins(), errors.New("an error has occurred, more coins joined than tokens passed in")
+	} else if tokensJoined.IsEqual(tokensIn) {
 		return numShares, tokensJoined, nil
 	}
 
@@ -727,13 +741,13 @@ func (p *Pool) CalcJoinPoolShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee s
 	// Instead, it mutates the pool assets argument to be further used by the caller.
 	// * 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)
 	if err := updateIntermediaryPoolAssetsLiquidity(tokensJoined, poolAssetsByDenom); err != nil {
 		return sdk.ZeroInt(), sdk.NewCoins(), err
 	}
 	newTotalShares := totalShares.Add(numShares)
 
 	// 5) Now single asset join each remaining coin.
+	remainingTokensIn := tokensIn.Sub(tokensJoined)
 	newNumSharesFromRemaining, newLiquidityFromRemaining, err := p.calcJoinSingleAssetTokensIn(remainingTokensIn, newTotalShares, poolAssetsByDenom, swapFee)
 	if err != nil {
 		return sdk.ZeroInt(), sdk.NewCoins(), err
@@ -749,6 +763,49 @@ func (p *Pool) CalcJoinPoolShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee s
 	return numShares, tokensJoined, nil
 }
 
+// CalcJoinPoolNoSwapShares calculates the number of shares created to execute an all-asset pool join with the provided amount of `tokensIn`.
+// The input tokens must contain the same tokens as in the pool.
+//
+// Returns the number of shares created, the amount of coins actually joined into the pool, (in case of not being able to fully join),
+// and the remaining tokens in `tokensIn` after joining. If an all-asset join is not possible, returns an error.
+//
+// Since CalcJoinPoolNoSwapShares is non-mutative, the steps for updating pool shares / liquidity are
+// more complex / don't just alter the state.
+// We should simplify this logic further in the future using multi-join equations.
+func (p *Pool) CalcJoinPoolNoSwapShares(ctx sdk.Context, tokensIn sdk.Coins, swapFee sdk.Dec) (numShares sdk.Int, tokensJoined sdk.Coins, err error) {
+	// get all 'pool assets' (aka current pool liquidity + balancer weight)
+	poolAssetsByDenom, err := getPoolAssetsByDenom(p.GetAllPoolAssets())
+	if err != nil {
+		return sdk.ZeroInt(), sdk.NewCoins(), err
+	}
+
+	err = ensureDenomInPool(poolAssetsByDenom, tokensIn)
+	if err != nil {
+		return sdk.ZeroInt(), sdk.NewCoins(), err
+	}
+
+	// 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")
+	}
+
+	// execute a no-swap join with as many tokens as possible given a perfect ratio:
+	// * numShares is how many shares are perfectly matched.
+	// * remainingTokensIn is how many coins we have left to join that have not already been used.
+	numShares, remainingTokensIn, err := cfmm_common.MaximalExactRatioJoin(p, ctx, tokensIn)
+	if err != nil {
+		return sdk.ZeroInt(), sdk.NewCoins(), err
+	}
+
+	// ensure that no more tokens have been joined than is possible with the given `tokensIn`
+	tokensJoined = tokensIn.Sub(remainingTokensIn)
+	if tokensJoined.IsAnyGT(tokensIn) {
+		return sdk.ZeroInt(), sdk.NewCoins(), errors.New("an error has occurred, more coins joined than token In")
+	}
+
+	return numShares, tokensJoined, nil
+}
+
 // calcJoinSingleAssetTokensIn attempts to calculate single
 // asset join for all tokensIn given totalShares in pool,
 // poolAssetsByDenom and swapFee. totalShares is the number

diff --git a/x/gamm/pool-models/balancer/pool_suite_test.go b/x/gamm/pool-models/balancer/pool_suite_test.go
@@ -790,6 +790,28 @@ func (suite *KeeperTestSuite) TestCalcJoinPoolShares() {
 			),
 			expectShares: sdk.NewInt(100_000_000),
 		},
+		{
+			// Input assets do not exist in pool
+			// P_issued = 1e20 * 1e-12 = 1e8
+			name:    "attempt join with denoms not in pool",
+			swapFee: sdk.MustNewDecFromStr("0"),
+			poolAssets: []balancer.PoolAsset{
+				{
+					Token:  sdk.NewInt64Coin("uosmo", 1_000_000_000_000),
+					Weight: sdk.NewInt(100),
+				},
+				{
+					Token:  sdk.NewInt64Coin("uatom", 1_000_000_000_000),
+					Weight: sdk.NewInt(100),
+				},
+			},
+			tokensIn: sdk.NewCoins(
+				sdk.NewInt64Coin("foo", 1),
+				sdk.NewInt64Coin("baz", 1),
+			),
+			expectShares: sdk.NewInt(0),
+			expErr:       types.ErrDenomNotFoundInPool,
+		},
 	}
 	testCases = append(testCases, calcSingleAssetJoinTestCases...)