fix: validation swap msg and offerCoinFee ceiling (#436)

* fix: validation swap msg and offerCoinFee ceiling

* docs: update description and swagger to 2.3.1

* fix: consume all ReservedOfferCoinFee when full matched

* fix: apply suggestions of the PR

* fix: revert da7d52e pointer argument

* fix: reserveOfferCoinFee residual Issue

* fix: update comments and revert fee logic

client/docs/config.json

@@ -3,7 +3,7 @@
   "info": {
     "title": "Cosmos SDK Liquidity Module - REST and gRPC Gateway docs",
     "description": "A REST interface for state queries, transactions",
-    "version": "2.3.0"
+    "version": "2.3.1"
   },
   "apis": [
     {

client/docs/swagger-ui/swagger.yaml

@@ -2,7 +2,7 @@ swagger: '2.0'
 info:
   title: Cosmos SDK Liquidity Module - REST and gRPC Gateway docs
   description: 'A REST interface for state queries, transactions'
-  version: 2.3.0
+  version: 2.3.1
 paths:
   /cosmos/liquidity/v1beta1/params:
     get:
@@ -1345,8 +1345,8 @@ paths:
                         offering
 
                         and `offer_coin_fee` must be half of offer coin amount *
-                        current `params.swap_fee_rate` for reservation to pay
-                        fees.
+                        current `params.swap_fee_rate` and ceil for reservation
+                        to pay fees.
 
                         This request is stacked in the batch of the liquidity
                         pool, is not processed 
@@ -1693,8 +1693,8 @@ paths:
                       offering
 
                       and `offer_coin_fee` must be half of offer coin amount *
-                      current `params.swap_fee_rate` for reservation to pay
-                      fees.
+                      current `params.swap_fee_rate` and ceil for reservation to
+                      pay fees.
 
                       This request is stacked in the batch of the liquidity
                       pool, is not processed 
@@ -2502,7 +2502,7 @@ definitions:
       `demand_coin_denom` with the coin and the price you're offering
 
       and `offer_coin_fee` must be half of offer coin amount * current
-      `params.swap_fee_rate` for reservation to pay fees.
+      `params.swap_fee_rate` and ceil for reservation to pay fees.
 
       This request is stacked in the batch of the liquidity pool, is not
       processed 
@@ -3455,7 +3455,7 @@ definitions:
               `demand_coin_denom` with the coin and the price you're offering
 
               and `offer_coin_fee` must be half of offer coin amount * current
-              `params.swap_fee_rate` for reservation to pay fees.
+              `params.swap_fee_rate` and ceil for reservation to pay fees.
 
               This request is stacked in the batch of the liquidity pool, is not
               processed 
@@ -3665,7 +3665,7 @@ definitions:
                 `demand_coin_denom` with the coin and the price you're offering
 
                 and `offer_coin_fee` must be half of offer coin amount * current
-                `params.swap_fee_rate` for reservation to pay fees.
+                `params.swap_fee_rate` and ceil for reservation to pay fees.
 
                 This request is stacked in the batch of the liquidity pool, is
                 not processed 
@@ -4086,7 +4086,7 @@ definitions:
           `demand_coin_denom` with the coin and the price you're offering
 
           and `offer_coin_fee` must be half of offer coin amount * current
-          `params.swap_fee_rate` for reservation to pay fees.
+          `params.swap_fee_rate` and ceil for reservation to pay fees.
 
           This request is stacked in the batch of the liquidity pool, is not
           processed 

go.sum

@@ -613,7 +613,6 @@ github.com/tendermint/go-amino v0.16.0/go.mod h1:TQU0M1i/ImAo+tYpZi73AU3V/dKeCoM
 github.com/tendermint/tendermint v0.34.0-rc4/go.mod h1:yotsojf2C1QBOw4dZrTcxbyxmPUrT4hNuOQWX9XUwB4=
 github.com/tendermint/tendermint v0.34.0-rc6/go.mod h1:ugzyZO5foutZImv0Iyx/gOFCX6mjJTgbLHTwi17VDVg=
 github.com/tendermint/tendermint v0.34.0/go.mod h1:Aj3PIipBFSNO21r+Lq3TtzQ+uKESxkbA3yo/INM4QwQ=
-github.com/tendermint/tendermint v0.34.10 h1:wBOc/It8sh/pVH9np2V5fBvRmIyFN/bUrGPx+eAHexs=
 github.com/tendermint/tendermint v0.34.10/go.mod h1:aeHL7alPh4uTBIJQ8mgFEE8VwJLXI1VD3rVOmH2Mcy0=
 github.com/tendermint/tendermint v0.34.11 h1:q1Yh76oG4QbS07xhmIJh5iAE0fYpJ8P8YKYtjnWfJRY=
 github.com/tendermint/tendermint v0.34.11/go.mod h1:aeHL7alPh4uTBIJQ8mgFEE8VwJLXI1VD3rVOmH2Mcy0=

proto/tendermint/liquidity/v1beta1/tx.proto

@@ -137,7 +137,7 @@ message MsgWithdrawWithinBatchResponse {}
 // `MsgSwapWithinBatch` defines an sdk.Msg type that supports submitting a swap offer request to the batch of the liquidity pool.
 // Submit swap offer to the liquidity pool batch with the specified the `pool_id`, `swap_type_id`,
 // `demand_coin_denom` with the coin and the price you're offering
-// and `offer_coin_fee` must be half of offer coin amount * current `params.swap_fee_rate` for reservation to pay fees.
+// and `offer_coin_fee` must be half of offer coin amount * current `params.swap_fee_rate` and ceil for reservation to pay fees.
 // This request is stacked in the batch of the liquidity pool, is not processed 
 // immediately, and is processed in the `endblock` at the same time as other requests.
 // You must request the same fields as the pool.
@@ -184,7 +184,7 @@ message MsgSwapWithinBatch {
       example: "\"denomB\"",
     }];
 
-  // half of offer coin amount * params.swap_fee_rate for reservation to pay fees.
+  // half of offer coin amount * params.swap_fee_rate and ceil for reservation to pay fees.
   cosmos.base.v1beta1.Coin offer_coin_fee = 6 [
     (gogoproto.nullable)     = false,
     (gogoproto.moretags)     = "yaml:\"offer_coin_fee\"",

x/liquidity/keeper/liquidity_pool.go

@@ -660,7 +660,7 @@ func (k Keeper) TransactAndRefundSwapLiquidityPool(ctx sdk.Context, swapMsgState
 			sendCoin(poolReserveAcc, sms.Msg.GetSwapRequester(), sdk.NewCoin(sms.Msg.DemandCoinDenom, receiveAmt))
 			sendCoin(batchEscrowAcc, poolReserveAcc, sdk.NewCoin(sms.Msg.OfferCoin.Denom, offerCoinFeeAmt))
 
-			if sms.RemainingOfferCoin.IsPositive() && sms.OrderExpiryHeight == ctx.BlockHeight() {
+			if sms.RemainingOfferCoin.Add(sms.ReservedOfferCoinFee).IsPositive() && sms.OrderExpiryHeight == ctx.BlockHeight() {
 				sendCoin(batchEscrowAcc, sms.Msg.GetSwapRequester(), sms.RemainingOfferCoin.Add(sms.ReservedOfferCoinFee))
 			}
 

x/liquidity/keeper/msg_server.go

@@ -138,19 +138,14 @@ func (k msgServer) Swap(goCtx context.Context, msg *types.MsgSwapWithinBatch) (*
 		return nil, types.ErrCircuitBreakerEnabled
 	}
 
-	params := k.GetParams(ctx)
-	if msg.OfferCoinFee.IsZero() {
-		msg.OfferCoinFee = types.GetOfferCoinFee(msg.OfferCoin, params.SwapFeeRate)
-	}
-
 	poolBatch, found := k.GetPoolBatch(ctx, msg.PoolId)
 	if !found {
 		return nil, types.ErrPoolBatchNotExists
 	}
 
 	batchMsg, err := k.Keeper.SwapWithinBatch(ctx, msg, types.CancelOrderLifeSpan)
 	if err != nil {
-		return &types.MsgSwapWithinBatchResponse{}, err
+		return nil, err
 	}
 
 	ctx.EventManager().EmitEvents(sdk.Events{

x/liquidity/keeper/msg_server_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/tendermint/liquidity/app"
+	"github.com/tendermint/liquidity/x/liquidity"
 	"github.com/tendermint/liquidity/x/liquidity/types"
 )
 
@@ -219,3 +220,213 @@ func TestMsgGetLiquidityPoolMetadata(t *testing.T) {
 	require.Equal(t, totalSupply, metaData.PoolCoinTotalSupply)
 	require.Equal(t, creatorBalance, metaData.PoolCoinTotalSupply)
 }
+
+func TestMsgSwapWithinBatch(t *testing.T) {
+	simapp, ctx := app.CreateTestInput()
+	params := simapp.LiquidityKeeper.GetParams(ctx)
+
+	depositCoins := sdk.NewCoins(sdk.NewCoin(DenomX, sdk.NewInt(1_000_000_000)), sdk.NewCoin(DenomY, sdk.NewInt(1_000_000_000)))
+	depositorAddr := app.AddRandomTestAddr(simapp, ctx, depositCoins.Add(params.PoolCreationFee...))
+	user := app.AddRandomTestAddr(simapp, ctx, sdk.NewCoins(sdk.NewCoin(DenomX, sdk.NewInt(1_000_000_000)), sdk.NewCoin(DenomY, sdk.NewInt(1_000_000_000))))
+
+	pool, err := simapp.LiquidityKeeper.CreatePool(ctx, &types.MsgCreatePool{
+		PoolCreatorAddress: depositorAddr.String(),
+		PoolTypeId:         types.DefaultPoolTypeID,
+		DepositCoins:       depositCoins,
+	})
+	require.NoError(t, err)
+
+	cases := []struct {
+		expectedErr  string // empty means no error expected
+		swapFeeRate  sdk.Dec
+		msg          *types.MsgSwapWithinBatch
+		afterBalance sdk.Coins
+	}{
+		{
+			"",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(10000)),
+				OfferCoinFee:         types.GetOfferCoinFee(sdk.NewCoin(DenomX, sdk.NewInt(10000)), params.SwapFeeRate),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("999989985denomX,1000009984denomY"),
+		},
+		{
+			// bad offer coin denom
+			"bad offer coin fee",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(10000)),
+				OfferCoinFee:         sdk.NewCoin(DenomY, sdk.NewInt(15)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("1000000000denomX,1000000000denomY"),
+		},
+		{
+			"bad offer coin fee",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(10000)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.NewInt(14)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("1000000000denomX,1000000000denomY"),
+		},
+		{
+			"bad offer coin fee",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(10000)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.NewInt(16)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("1000000000denomX,1000000000denomY"),
+		},
+		{
+			"",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(10001)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.NewInt(16)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("999989983denomX,1000009984denomY"),
+		},
+		{
+			"",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(100)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.NewInt(1)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("999999899denomX,1000000098denomY"),
+		},
+		{
+			"bad offer coin fee",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(100)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.NewInt(0)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("1000000000denomX,1000000000denomY"),
+		},
+		{
+			"",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(1000)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.NewInt(2)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("999998998denomX,1000000997denomY"),
+		},
+		{
+			"bad offer coin fee",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(1000)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.NewInt(1)),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("1000000000denomX,1000000000denomY"),
+		},
+		{
+			"",
+			sdk.ZeroDec(),
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(1000)),
+				OfferCoinFee:         sdk.NewCoin(DenomX, sdk.ZeroInt()),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("999999000denomX,1000000999denomY"),
+		},
+		{
+			"does not match the reserve coin of the pool",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(10000)),
+				OfferCoinFee:         types.GetOfferCoinFee(sdk.NewCoin(DenomX, sdk.NewInt(10000)), params.SwapFeeRate),
+				DemandCoinDenom:      DenomA,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("1000000000denomX,1000000000denomY"),
+		},
+		{
+			"can not exceed max order ratio of reserve coins that can be ordered at a order",
+			types.DefaultSwapFeeRate,
+			&types.MsgSwapWithinBatch{
+				SwapRequesterAddress: user.String(),
+				PoolId:               pool.Id,
+				SwapTypeId:           pool.TypeId,
+				OfferCoin:            sdk.NewCoin(DenomX, sdk.NewInt(100_000_001)),
+				OfferCoinFee:         types.GetOfferCoinFee(sdk.NewCoin(DenomX, sdk.NewInt(100_000_001)), params.SwapFeeRate),
+				DemandCoinDenom:      DenomY,
+				OrderPrice:           sdk.MustNewDecFromStr("1.00002"),
+			},
+			types.MustParseCoinsNormalized("1000000000denomX,1000000000denomY"),
+		},
+	}
+
+	for _, tc := range cases {
+		cacheCtx, _ := ctx.CacheContext()
+		cacheCtx = cacheCtx.WithBlockHeight(1)
+		params.SwapFeeRate = tc.swapFeeRate
+		simapp.LiquidityKeeper.SetParams(cacheCtx, params)
+		_, err = simapp.LiquidityKeeper.SwapWithinBatch(cacheCtx, tc.msg, types.CancelOrderLifeSpan)
+		if tc.expectedErr == "" {
+			require.NoError(t, err)
+			liquidity.EndBlocker(cacheCtx, simapp.LiquidityKeeper)
+		} else {
+			require.EqualError(t, err, tc.expectedErr)
+		}
+		moduleAccAddress := simapp.AccountKeeper.GetModuleAddress(types.ModuleName)
+		require.True(t, simapp.BankKeeper.GetAllBalances(cacheCtx, moduleAccAddress).IsZero())
+		require.Equal(t, tc.afterBalance, simapp.BankKeeper.GetAllBalances(cacheCtx, user))
+	}
+}

x/liquidity/spec/03_state_transitions.md

@@ -221,12 +221,25 @@ Variables:
 
 ### Swap Fee Payment
 
-- Swap fees are calculated after the swap price is calculated.
-- Swap fees are proportional to the coins received from matched swap orders.
-
-  - `SwapFee` = `ReceivedMatchedCoin` * `SwapFeeRate`
+Rather than taking fee solely from `OfferCoin`, liquidity module is designed to take fees half from `OfferCoin`, and the other half from `ExchangedCoin`. This smooths out an impact of the fee payment process.
+- **OfferCoin Fee Reservation ( fee before batch process, in OfferCoin )**
+    - when user orders 100 Xcoin, the swap message demands
+        - `OfferCoin`(in Xcoin) : 100
+        - `ReservedOfferCoinFeeAmount`(in Xcoin) = `OfferCoin`*(`SwapFeeRate`/2)
+    - user needs to have at least 100+100*(`SwapFeeRate`/2) amount of Xcoin to successfully commit this swap message
+        - the message fails when user's balance is below this amount
+- **Actual Fee Payment**
+    - if 10 Xcoin is executed
+        - **OfferCoin Fee Payment from Reserved OfferCoin Fee**
+            - `OfferCoinFeeAmount`(in Xcoin) = (10/100)*`ReservedOfferCoinFeeAmount`
+            - `ReservedOfferCoinFeeAmount` is reduced from this fee payment
+        - **ExchangedCoin Fee Payment ( fee after batch process, in ExchangedCoin )**
+            - `ExchangedCoinFeeAmount`(in Ycoin) = `OfferCoinFeeAmount` / `SwapPrice`
+                - this is exactly equal value compared to advance fee payment assuming the current SwapPrice, to minimize the pool price impact from fee payment process
 
-- Swap fees are sent to the liquidity pool
+- Swap fees are proportional to the coins received from matched swap orders.
+- Swap fees are sent to the liquidity pool.
+- The decimal points of the swap fees are rounded up.
 
 ## Cancel unexecuted swap orders with expired CancelHeight
 

x/liquidity/spec/04_messages.md

@@ -97,5 +97,5 @@ The MsgWithdrawWithinBatch message performs validity checks. The transaction tha
 - Denoms of `OfferCoin` or `DemandCoin` do not exist in `bank` module
 - The balance of `SwapRequester` does not have enough coins for `OfferCoin`
 - `OrderPrice` <= zero
-- `OfferCoinFee` equals `OfferCoin` _`params.SwapFeeRate`_ `0.5` with truncating Int
+- `OfferCoinFee` equals `OfferCoin` * `params.SwapFeeRate` * `0.5` with ceiling
 - Has sufficient balance `OfferCoinFee` to reserve offer coin fee