Add more validation to order placement (#175)

* Add more validation to order placement

* gofumpt

* Rm account and contract addr validation

* Add fund check

* Add nil check

* fix lint

* Catch all panics in DEX EndBlock (#176)

* Catch all panics in DEX EndBlock

* update metric key

* address comments

* Add catch-all for all custom modules (#178)

* Refactor `dex` `Endblock` logic per contract (#174)

* Refactor dex EndBlock

* tests

* Refactor `dex` EndBlock logic

* fix order pointer

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

Author: philipsu522

x/dex/keeper/msgserver/msg_server_place_orders.go

@@ -3,6 +3,7 @@ package msgserver
 import (
 	"context"
 	"errors"
+	"fmt"
 	"math/big"
 
 	conversion "github.com/sei-protocol/sei-chain/utils"
@@ -32,6 +33,9 @@ func (k msgServer) transferFunds(goCtx context.Context, msg *types.MsgPlaceOrder
 	}
 
 	for _, fund := range msg.Funds {
+		if fund.Amount.IsNil() || fund.IsNegative() {
+			return errors.New("fund deposits cannot be nil or negative")
+		}
 		k.MemState.GetDepositInfo(typesutils.ContractAddress(msg.GetContractAddr())).AddDeposit(dexcache.DepositInfoEntry{
 			Creator: msg.Creator,
 			Denom:   fund.Denom,
@@ -99,11 +103,17 @@ func (k msgServer) PlaceOrders(goCtx context.Context, msg *types.MsgPlaceOrders)
 }
 
 func (k msgServer) validateOrder(order *types.Order) error {
-	if order.Quantity.IsNil() {
-		return errors.New("quantity cannot be empty")
+	if order.Quantity.IsNil() || order.Quantity.IsNegative() {
+		return fmt.Errorf("invalid order quantity: %s", order.Quantity)
+	}
+	if order.Price.IsNil() || order.Price.IsNegative() {
+		return fmt.Errorf("invalid order price: %s", order.Price)
+	}
+	if len(order.AssetDenom) == 0 {
+		return fmt.Errorf("invalid order, asset denom is empty")
 	}
-	if order.Price.IsNil() {
-		return errors.New("price cannot be empty")
+	if len(order.PriceDenom) == 0 {
+		return fmt.Errorf("invalid order, price denom is empty")
 	}
 	return nil
 }

x/dex/keeper/msgserver/msg_server_place_orders_test.go

@@ -10,8 +10,10 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-const TestCreator = "sei1ewxvf5a9wq9zk5nurtl6m9yfxpnhyp7s7uk5sl"
-const TestContract = "sei14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9sh9m79m"
+const (
+	TestCreator  = "sei1ewxvf5a9wq9zk5nurtl6m9yfxpnhyp7s7uk5sl"
+	TestContract = "sei14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9sh9m79m"
+)
 
 func TestPlaceOrder(t *testing.T) {
 	msg := &types.MsgPlaceOrders{
@@ -48,6 +50,9 @@ func TestPlaceOrder(t *testing.T) {
 	require.Equal(t, 2, len(res.OrderIds))
 	require.Equal(t, uint64(0), res.OrderIds[0])
 	require.Equal(t, uint64(1), res.OrderIds[1])
+	// Ensure that contract addr and account is set in the order
+	require.Equal(t, msg.Orders[0].ContractAddr, TestContract)
+	require.Equal(t, msg.Orders[0].Account, TestCreator)
 }
 
 func TestPlaceInvalidOrder(t *testing.T) {
@@ -56,6 +61,7 @@ func TestPlaceInvalidOrder(t *testing.T) {
 	keeper.SetTickSizeForPair(ctx, TestContract, keepertest.TestPair, *keepertest.TestPair.Ticksize)
 	wctx := sdk.WrapSDKContext(ctx)
 
+	// Empty quantity
 	msg := &types.MsgPlaceOrders{
 		Creator:      TestCreator,
 		ContractAddr: TestContract,
@@ -75,6 +81,7 @@ func TestPlaceInvalidOrder(t *testing.T) {
 	_, err := server.PlaceOrders(wctx, msg)
 	require.NotNil(t, err)
 
+	// Empty price
 	msg = &types.MsgPlaceOrders{
 		Creator:      TestCreator,
 		ContractAddr: TestContract,
@@ -93,4 +100,86 @@ func TestPlaceInvalidOrder(t *testing.T) {
 	server = msgserver.NewMsgServerImpl(*keeper, nil)
 	_, err = server.PlaceOrders(wctx, msg)
 	require.NotNil(t, err)
+
+	// Negative quantity
+	msg = &types.MsgPlaceOrders{
+		Creator:      TestCreator,
+		ContractAddr: TestContract,
+		Orders: []*types.Order{
+			{
+				Price:             sdk.MustNewDecFromStr("10"),
+				Quantity:          sdk.MustNewDecFromStr("-1"),
+				Data:              "",
+				PositionDirection: types.PositionDirection_LONG,
+				OrderType:         types.OrderType_LIMIT,
+				PriceDenom:        keepertest.TestPriceDenom,
+				AssetDenom:        keepertest.TestAssetDenom,
+			},
+		},
+	}
+	server = msgserver.NewMsgServerImpl(*keeper, nil)
+	_, err = server.PlaceOrders(wctx, msg)
+	require.NotNil(t, err)
+
+	// Negative price
+	msg = &types.MsgPlaceOrders{
+		Creator:      TestCreator,
+		ContractAddr: TestContract,
+		Orders: []*types.Order{
+			{
+				Price:             sdk.MustNewDecFromStr("-1"),
+				Quantity:          sdk.MustNewDecFromStr("10"),
+				Data:              "",
+				PositionDirection: types.PositionDirection_LONG,
+				OrderType:         types.OrderType_LIMIT,
+				PriceDenom:        keepertest.TestPriceDenom,
+				AssetDenom:        keepertest.TestAssetDenom,
+				ContractAddr:      TestContract,
+				Account:           "testaccount",
+			},
+		},
+	}
+	server = msgserver.NewMsgServerImpl(*keeper, nil)
+	_, err = server.PlaceOrders(wctx, msg)
+	require.NotNil(t, err)
+
+	// Missing contract
+	msg = &types.MsgPlaceOrders{
+		Creator:      TestCreator,
+		ContractAddr: TestContract,
+		Orders: []*types.Order{
+			{
+				Price:             sdk.MustNewDecFromStr("-1"),
+				Quantity:          sdk.MustNewDecFromStr("10"),
+				Data:              "",
+				PositionDirection: types.PositionDirection_LONG,
+				OrderType:         types.OrderType_LIMIT,
+				PriceDenom:        keepertest.TestPriceDenom,
+				AssetDenom:        keepertest.TestAssetDenom,
+			},
+		},
+	}
+	server = msgserver.NewMsgServerImpl(*keeper, nil)
+	_, err = server.PlaceOrders(wctx, msg)
+	require.NotNil(t, err)
+
+	// Missing account
+	msg = &types.MsgPlaceOrders{
+		Creator:      TestCreator,
+		ContractAddr: TestContract,
+		Orders: []*types.Order{
+			{
+				Price:             sdk.MustNewDecFromStr("-1"),
+				Quantity:          sdk.MustNewDecFromStr("10"),
+				Data:              "",
+				PositionDirection: types.PositionDirection_LONG,
+				OrderType:         types.OrderType_LIMIT,
+				PriceDenom:        keepertest.TestPriceDenom,
+				AssetDenom:        keepertest.TestAssetDenom,
+			},
+		},
+	}
+	server = msgserver.NewMsgServerImpl(*keeper, nil)
+	_, err = server.PlaceOrders(wctx, msg)
+	require.NotNil(t, err)
 }

x/dex/module_test.go

@@ -336,7 +336,8 @@ func TestEndBlockPanicHandling(t *testing.T) {
 	dexkeeper.SetContract(ctx, &types.ContractInfo{CodeId: 123, ContractAddr: contractAddr.String(), NeedHook: true, NeedOrderMatching: true})
 	dexkeeper.AddRegisteredPair(ctx, contractAddr.String(), pair)
 	dexkeeper.MemState.GetBlockOrders(utils.ContractAddress(contractAddr.String()), utils.GetPairString(&pair)).AddOrder(
-		types.Order{
+		&types.Order{
+
 			Id:                1,
 			Account:           testAccount.String(),
 			ContractAddr:      contractAddr.String(),