Change Pool interface calc methods to using integers (#1345)

* Change Pool interface calc methods to using integers * Update breaking change notes * Update x/gamm/pool-models/balancer/amm_test.go Co-authored-by: Roman <[email protected]> * Update failing tests due to panic * Change tolerance precision * Comment updates * Fix amm_test for calc amount and in inverse relation * Address Bez' comments Co-authored-by: Roman <[email protected]>
osmosis-labs · Apr 28, 2022 · e96679b · e96679b
1 parent 69811b3
commit e96679b
Show file tree

Hide file tree

Showing 12 changed files with 136 additions and 104 deletions.
diff --git a/app/wasm/test/custom_msg_test.go b/app/wasm/test/custom_msg_test.go
@@ -11,7 +11,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/osmosis-labs/osmosis/v7/app"
-	"github.com/osmosis-labs/osmosis/v7/app/wasm/bindings"
+	wasmbindings "github.com/osmosis-labs/osmosis/v7/app/wasm/bindings"
 )
 
 // func TestMintMsg(t *testing.T) {
@@ -63,13 +63,15 @@ type BaseState struct {
 
 func TestSwapMsg(t *testing.T) {
 	// table tests with this setup
-	cases := map[string]struct {
+	cases := []struct {
+		name       string
 		msg        func(BaseState) *wasmbindings.SwapMsg
 		expectErr  bool
 		initFunds  sdk.Coin
 		finalFunds []sdk.Coin
 	}{
-		"exact in: simple swap works": {
+		{
+			name: "exact in: simple swap works",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -93,7 +95,8 @@ func TestSwapMsg(t *testing.T) {
 				sdk.NewInt64Coin("ustar", 120000000),
 			},
 		},
-		"exact in: price too low": {
+		{
+			name: "exact in: price too low",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -114,7 +117,8 @@ func TestSwapMsg(t *testing.T) {
 			initFunds: sdk.NewInt64Coin("uosmo", 13000000),
 			expectErr: true,
 		},
-		"exact in: not enough funds to swap": {
+		{
+			name: "exact in: not enough funds to swap",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -135,7 +139,8 @@ func TestSwapMsg(t *testing.T) {
 			initFunds: sdk.NewInt64Coin("uosmo", 7000000),
 			expectErr: true,
 		},
-		"exact in: invalidPool": {
+		{
+			name: "exact in: invalidPool",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -183,8 +188,8 @@ func TestSwapMsg(t *testing.T) {
 		//		sdk.NewInt64Coin("ustar", 120000000),
 		//	},
 		//},
-
-		"exact out: simple swap works": {
+		{
+			name: "exact out: simple swap works",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -209,8 +214,8 @@ func TestSwapMsg(t *testing.T) {
 				sdk.NewInt64Coin("uosmo", 2000000),
 			},
 		},
-
-		"exact in: 2 step multi-hop": {
+		{
+			name: "exact in: 2 step multi-hop",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -237,7 +242,8 @@ func TestSwapMsg(t *testing.T) {
 				sdk.NewInt64Coin("uatom", 1999999),
 			},
 		},
-		"exact out: 2 step multi-hop": {
+		{
+			name: "exact out: 2 step multi-hop",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -251,24 +257,24 @@ func TestSwapMsg(t *testing.T) {
 					}},
 					Amount: wasmbindings.SwapAmountWithLimit{
 						ExactOut: &wasmbindings.ExactOut{
-							MaxInput: sdk.NewInt(6000000),
-							Output:   sdk.NewInt(24000000),
+							MaxInput: sdk.NewInt(2000000),
+							Output:   sdk.NewInt(12000000 - 12),
 						},
 					},
 				}
 			},
-			initFunds: sdk.NewInt64Coin("uosmo", 6000000),
+			initFunds: sdk.NewInt64Coin("uosmo", 2000000),
 			finalFunds: []sdk.Coin{
-				// 6 OSMO -> 2 ATOM
-				// 2 ATOM -> 24 REGEN (with minor rounding)
-				sdk.NewInt64Coin("uosmo", 5),
-				sdk.NewInt64Coin("uregen", 24000000),
+				// 2 OSMO -> 1.2 ATOM
+				// 1.2 ATOM -> 12 REGEN (with minor rounding)
+				sdk.NewInt64Coin("uosmo", 2),
+				sdk.NewInt64Coin("uregen", 12000000-12),
 			},
 		},
-
 		// FIXME: this panics in GAMM module !?! hits a known TODO
 		// https://github.com/osmosis-labs/osmosis/blob/a380ab2fcd39fb94c2b10411e07daf664911257a/osmomath/math.go#L47-L51
-		// "exact out: panics on math power stuff": {
+		// {
+		// 	name: "exact out: panics on math power stuff",
 		// 	msg: func(state BaseState) *wasmbindings.SwapMsg {
 		// 		return &wasmbindings.SwapMsg{
 		// 			First: wasmbindings.Swap{
@@ -296,8 +302,8 @@ func TestSwapMsg(t *testing.T) {
 		// 		sdk.NewInt64Coin("ustar", 5000),
 		// 	},
 		// },
-
-		"exact in: 3 step multi-hop": {
+		{
+			name: "exact in: 3 step multi-hop",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -328,8 +334,8 @@ func TestSwapMsg(t *testing.T) {
 				sdk.NewInt64Coin("uregen", 23999990),
 			},
 		},
-
-		"exact out: 3 step multi-hop": {
+		{
+			name: "exact out: 3 step multi-hop",
 			msg: func(state BaseState) *wasmbindings.SwapMsg {
 				return &wasmbindings.SwapMsg{
 					First: wasmbindings.Swap{
@@ -346,26 +352,25 @@ func TestSwapMsg(t *testing.T) {
 					}},
 					Amount: wasmbindings.SwapAmountWithLimit{
 						ExactOut: &wasmbindings.ExactOut{
-							MaxInput: sdk.NewInt(240000000),
-							Output:   sdk.NewInt(24000000),
+							MaxInput: sdk.NewInt(50000000),
+							Output:   sdk.NewInt(12000000),
 						},
 					},
 				}
 			},
-			initFunds: sdk.NewInt64Coin("ustar", 240000000),
+			initFunds: sdk.NewInt64Coin("ustar", 50000000),
 			finalFunds: []sdk.Coin{
-				// 240 STAR -> 6 OSMO
-				// 6 OSMO -> 2 ATOM
-				// 2 ATOM -> 24 REGEN (with minor rounding)
-				sdk.NewInt64Coin("uregen", 24000000),
-				sdk.NewInt64Coin("ustar", 400),
+				// ~48 STAR -> 2 OSMO
+				// 2 OSMO -> .857 ATOM
+				// .857 ATOM -> 12 REGEN (with minor rounding)
+				sdk.NewInt64Coin("uregen", 12000000),
+				sdk.NewInt64Coin("ustar", 1999971),
 			},
 		},
 	}
-
-	for name, tc := range cases {
+	for _, tc := range cases {
 		tc := tc
-		t.Run(name, func(t *testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 			creator := RandomAccountAddress()
 			osmosis, ctx := SetupCustomApp(t, creator)
 			state := prepareSwapState(t, ctx, osmosis)

diff --git a/app/wasm/test/custom_query_test.go b/app/wasm/test/custom_query_test.go
@@ -17,7 +17,7 @@ import (
 
 	"github.com/osmosis-labs/osmosis/v7/app"
 	"github.com/osmosis-labs/osmosis/v7/app/wasm"
-	"github.com/osmosis-labs/osmosis/v7/app/wasm/bindings"
+	wasmbindings "github.com/osmosis-labs/osmosis/v7/app/wasm/bindings"
 	"github.com/osmosis-labs/osmosis/v7/x/gamm/pool-models/balancer"
 )
 
@@ -175,7 +175,7 @@ func TestQuerySpotPrice(t *testing.T) {
 func TestQueryEstimateSwap(t *testing.T) {
 	actor := RandomAccountAddress()
 	osmosis, ctx := SetupCustomApp(t, actor)
-	epsilon := 1e-3
+	epsilon := 2e-3
 
 	fundAccount(t, ctx, osmosis, actor, defaultFunds)
 

diff --git a/osmoutils/dec_helper.go b/osmoutils/dec_helper.go
@@ -0,0 +1,14 @@
+package osmoutils
+
+import (
+	"testing"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+// intended to be used with require/assert:  require.True(DecEq(...))
+// TODO: Replace with function in SDK types package when we update
+func DecApproxEq(t *testing.T, d1 sdk.Dec, d2 sdk.Dec, tol sdk.Dec) (*testing.T, bool, string, string, string) {
+	diff := d1.Sub(d2).Abs()
+	return t, diff.LTE(tol), "expected |d1 - d2| <:\t%v\ngot |d1 - d2| = \t\t%v", tol.String(), diff.String()
+}
diff --git a/x/gamm/breaking_changes_notes.md b/x/gamm/breaking_changes_notes.md
@@ -21,6 +21,8 @@
   * Before the message would fail if you had too few shares to get a single token out for any given denom. Now you can get 0 tokens of one side out, if the min amount is also not present.
 * ExitSwapShareAmountIn
   * Switched to a more inefficient algorithm for now, so gas numbers will be much higher.
+* MsgSwapExactAmountOut
+  * Prior behavior rounded down the required AmountIn input. The logic now rounds up. Any prior test vectors will likely be off by one.
 * Messages now have responses
 
 ## Events

diff --git a/x/gamm/keeper/multihop.go b/x/gamm/keeper/multihop.go
@@ -84,9 +84,8 @@ func (k Keeper) createMultihopExpectedSwapOuts(ctx sdk.Context, routes []types.S
 			return nil, err
 		}
 
-		insExpected[i] = tokenIn.Amount.TruncateInt()
-
-		tokenOut, _ = tokenIn.TruncateDecimal()
+		insExpected[i] = tokenIn.Amount
+		tokenOut = tokenIn
 	}
 
 	return insExpected, nil

diff --git a/x/gamm/keeper/swap.go b/x/gamm/keeper/swap.go
@@ -109,22 +109,20 @@ func (k Keeper) swapExactAmountOut(
 		return sdk.Int{}, sdkerrors.Wrapf(types.ErrTooManyTokensOut,
 			"can't get more tokens out than there are tokens in the pool")
 	}
-	tokenInCoin, err := pool.SwapInAmtGivenOut(ctx, sdk.Coins{tokenOut}, tokenInDenom, swapFee)
+	tokenIn, err := pool.SwapInAmtGivenOut(ctx, sdk.Coins{tokenOut}, tokenInDenom, swapFee)
 	if err != nil {
 		return sdk.Int{}, err
 	}
-	tokenInAmount = tokenInCoin.Amount
+	tokenInAmount = tokenIn.Amount
 
 	if tokenInAmount.LTE(sdk.ZeroInt()) {
 		return sdk.Int{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount is zero or negative")
 	}
 
 	if tokenInAmount.GT(tokenInMaxAmount) {
-		return sdk.Int{}, sdkerrors.Wrapf(types.ErrLimitMaxAmount, "%s token required is larger than max amount", tokenInDenom)
+		return sdk.Int{}, sdkerrors.Wrapf(types.ErrLimitMaxAmount, "Swap requires %s, which is greater than the amount %s", tokenIn, tokenInMaxAmount)
 	}
 
-	tokenIn := sdk.Coin{Denom: tokenInDenom, Amount: tokenInAmount}
-
 	err = k.updatePoolForSwap(ctx, pool, sender, tokenIn, tokenOut)
 	if err != nil {
 		return sdk.Int{}, err

diff --git a/x/gamm/keeper/swap_test.go b/x/gamm/keeper/swap_test.go
@@ -125,7 +125,7 @@ func (suite *KeeperTestSuite) TestBalancerPoolSimpleSwapExactAmountOut() {
 				tokenInDenom:          "foo",
 				tokenInMaxAmount:      sdk.NewInt(900000000),
 				tokenOut:              sdk.NewCoin("bar", sdk.NewInt(100000)),
-				expectedTokenInAmount: sdk.NewInt(206164),
+				expectedTokenInAmount: sdk.NewInt(206165),
 			},
 			expectPass: true,
 		},
@@ -135,7 +135,7 @@ func (suite *KeeperTestSuite) TestBalancerPoolSimpleSwapExactAmountOut() {
 				tokenInDenom:          "foo",
 				tokenInMaxAmount:      sdk.NewInt(900000000),
 				tokenOut:              sdk.NewCoin("baz", sdk.NewInt(316721)),
-				expectedTokenInAmount: sdk.NewInt(1084570),
+				expectedTokenInAmount: sdk.NewInt(1084571),
 			},
 			expectPass: true,
 		},

diff --git a/x/gamm/pool-models/balancer/amm.go b/x/gamm/pool-models/balancer/amm.go
@@ -30,7 +30,7 @@ func solveConstantFunctionInvariant(
 	// weightRatio = (weightX/weightY)
 	weightRatio := tokenWeightFixed.Quo(tokenWeightUnknown)
 
-	// y = balanceXBefore/balanceYAfter
+	// y = balanceXBefore/balanceXAfter
 	y := tokenBalanceFixedBefore.Quo(tokenBalanceFixedAfter)
 
 	// amountY = balanceY * (1 - (y ^ weightRatio))
@@ -47,10 +47,10 @@ func (p Pool) CalcOutAmtGivenIn(
 	tokensIn sdk.Coins,
 	tokenOutDenom string,
 	swapFee sdk.Dec,
-) (sdk.DecCoin, error) {
+) (sdk.Coin, error) {
 	tokenIn, poolAssetIn, poolAssetOut, err := p.parsePoolAssets(tokensIn, tokenOutDenom)
 	if err != nil {
-		return sdk.DecCoin{}, err
+		return sdk.Coin{}, err
 	}
 
 	tokenAmountInAfterFee := tokenIn.Amount.ToDec().Mul(sdk.OneDec().Sub(swapFee))
@@ -67,7 +67,13 @@ func (p Pool) CalcOutAmtGivenIn(
 		poolAssetOut.Weight.ToDec(),
 	)
 
-	return sdk.NewDecCoinFromDec(tokenOutDenom, tokenAmountOut), nil
+	// We ignore the decimal component, as we round down the token amount out.
+	tokenAmountOutInt := tokenAmountOut.TruncateInt()
+	if !tokenAmountOutInt.IsPositive() {
+		return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")
+	}
+
+	return sdk.NewCoin(tokenOutDenom, tokenAmountOutInt), nil
 }
 
 // SwapOutAmtGivenIn is a mutative method for CalcOutAmtGivenIn, which includes the actual swap.
@@ -79,14 +85,10 @@ func (p *Pool) SwapOutAmtGivenIn(
 ) (
 	tokenOut sdk.Coin, err error,
 ) {
-	tokenOutDecCoin, err := p.CalcOutAmtGivenIn(ctx, tokensIn, tokenOutDenom, swapFee)
+	tokenOutCoin, err := p.CalcOutAmtGivenIn(ctx, tokensIn, tokenOutDenom, swapFee)
 	if err != nil {
 		return sdk.Coin{}, err
 	}
-	tokenOutCoin, _ := tokenOutDecCoin.TruncateDecimal()
-	if !tokenOutCoin.Amount.IsPositive() {
-		return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")
-	}
 
 	err = p.applySwap(ctx, tokensIn, sdk.Coins{tokenOutCoin})
 	if err != nil {
@@ -99,11 +101,11 @@ func (p *Pool) SwapOutAmtGivenIn(
 // given the swapped out amount, using solveConstantFunctionInvariant.
 func (p Pool) CalcInAmtGivenOut(
 	ctx sdk.Context, tokensOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (
-	tokenIn sdk.DecCoin, err error,
+	tokenIn sdk.Coin, err error,
 ) {
 	tokenOut, poolAssetOut, poolAssetIn, err := p.parsePoolAssets(tokensOut, tokenInDenom)
 	if err != nil {
-		return sdk.DecCoin{}, err
+		return sdk.Coin{}, err
 	}
 
 	// delta balanceOut is positive(tokens inside the pool decreases)
@@ -119,27 +121,25 @@ func (p Pool) CalcInAmtGivenOut(
 	// Thus in order to give X amount out, we solve the invariant for the invariant input. However invariant input = (1 - swapfee) * trade input.
 	// Therefore we divide by (1 - swapfee) here
 	tokenAmountInBeforeFee := tokenAmountIn.Quo(sdk.OneDec().Sub(swapFee))
-	// TODO: Once we make Calc methods return integers
-	// if tokenInDecimal is non-zero, we add 1 to the tokenInCoin
-	// if tokenInDecimal.Amount.IsPositive() {
-	// 	tokenInCoin.Amount = tokenInCoin.Amount.AddRaw(1)
-	// }
-	return sdk.NewDecCoinFromDec(tokenInDenom, tokenAmountInBeforeFee), nil
+
+	// We round up tokenInAmt, as this is whats charged for the swap, for the precise amount out.
+	// Otherwise, the pool would under-charge by this rounding error.
+	tokenInAmt := tokenAmountInBeforeFee.Ceil().TruncateInt()
+
+	if !tokenInAmt.IsPositive() {
+		return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")
+	}
+	return sdk.NewCoin(tokenInDenom, tokenInAmt), nil
 }
 
 // SwapInAmtGivenOut is a mutative method for CalcOutAmtGivenIn, which includes the actual swap.
 func (p *Pool) SwapInAmtGivenOut(
 	ctx sdk.Context, tokensOut sdk.Coins, tokenInDenom string, swapFee sdk.Dec) (
 	tokenIn sdk.Coin, err error,
 ) {
-	tokenInDecCoin, err := p.CalcInAmtGivenOut(ctx, tokensOut, tokenInDenom, swapFee)
+	tokenInCoin, err := p.CalcInAmtGivenOut(ctx, tokensOut, tokenInDenom, swapFee)
 	if err != nil {
-		return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount is zero or negative")
-	}
-	tokenInCoin, _ := tokenInDecCoin.TruncateDecimal()
-
-	if !tokenInCoin.Amount.IsPositive() {
-		return sdk.Coin{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount must be positive")
+		return sdk.Coin{}, err
 	}
 
 	err = p.applySwap(ctx, sdk.Coins{tokenInCoin}, tokensOut)