fix: check coins are positive in lockup ValidateBasic (#2180)

* add check to lockup validate basic

* add table driven test for ValidateBasic

* Update msgs_test.go

x/lockup/types/msgs.go

@@ -5,6 +5,7 @@ import (
 	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
 // constants.
@@ -29,6 +30,11 @@ func NewMsgLockTokens(owner sdk.AccAddress, duration time.Duration, coins sdk.Co
 func (m MsgLockTokens) Route() string { return RouterKey }
 func (m MsgLockTokens) Type() string  { return TypeMsgLockTokens }
 func (m MsgLockTokens) ValidateBasic() error {
+	_, err := sdk.AccAddressFromBech32(m.Owner)
+	if err != nil {
+		return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "Invalid owner address (%s)", err)
+	}
+
 	if m.Duration <= 0 {
 		return fmt.Errorf("duration should be positive: %d < 0", m.Duration)
 	}
@@ -38,6 +44,10 @@ func (m MsgLockTokens) ValidateBasic() error {
 		return fmt.Errorf("lockups can only have one denom per lock ID, got %v", m.Coins)
 	}
 
+	if !m.Coins.IsAllPositive() {
+		return fmt.Errorf("cannot lock up a zero or negative amount")
+	}
+
 	return nil
 }
 

x/lockup/types/msgs_test.go

@@ -0,0 +1,104 @@
+package types
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	appParams "github.com/osmosis-labs/osmosis/v10/app/params"
+)
+
+func TestMsgLockTokens(t *testing.T) {
+	appParams.SetAddressPrefixes()
+	pk1 := ed25519.GenPrivKey().PubKey()
+	addr1 := sdk.AccAddress(pk1.Address()).String()
+	invalidAddr := sdk.AccAddress("invalid")
+
+	createMsg := func(after func(msg MsgLockTokens) MsgLockTokens) MsgLockTokens {
+		properMsg := MsgLockTokens{
+			Owner:    addr1,
+			Duration: time.Hour,
+			Coins:    sdk.NewCoins(sdk.NewCoin("test", sdk.NewInt(100))),
+		}
+
+		return after(properMsg)
+	}
+
+	msg := createMsg(func(msg MsgLockTokens) MsgLockTokens {
+		// Do nothing
+		return msg
+	})
+
+	require.Equal(t, msg.Route(), RouterKey)
+	require.Equal(t, msg.Type(), "lock_tokens")
+	signers := msg.GetSigners()
+	require.Equal(t, len(signers), 1)
+	require.Equal(t, signers[0].String(), addr1)
+
+	tests := []struct {
+		name       string
+		msg        MsgLockTokens
+		expectPass bool
+	}{
+		{
+			name: "proper msg",
+			msg: createMsg(func(msg MsgLockTokens) MsgLockTokens {
+				// Do nothing
+				return msg
+			}),
+			expectPass: true,
+		},
+		{
+			name: "invalid owner",
+			msg: createMsg(func(msg MsgLockTokens) MsgLockTokens {
+				msg.Owner = invalidAddr.String()
+				return msg
+			}),
+			expectPass: false,
+		},
+		{
+			name: "invalid duration",
+			msg: createMsg(func(msg MsgLockTokens) MsgLockTokens {
+				msg.Duration = -1
+				return msg
+			}),
+			expectPass: false,
+		},
+		{
+			name: "invalid coin length",
+			msg: createMsg(func(msg MsgLockTokens) MsgLockTokens {
+				msg.Coins = sdk.NewCoins(sdk.NewCoin("test1", sdk.NewInt(100000)), sdk.NewCoin("test2", sdk.NewInt(100000)))
+				return msg
+			}),
+			expectPass: false,
+		},
+		{
+			name: "zero token amount",
+			msg: createMsg(func(msg MsgLockTokens) MsgLockTokens {
+				msg.Coins = sdk.NewCoins(sdk.NewCoin("test1", sdk.NewInt(0)))
+				return msg
+			}),
+			expectPass: false,
+		},
+	}
+
+	for _, test := range tests {
+		if test.expectPass {
+			require.NoError(t, test.msg.ValidateBasic(), "test: %v", test.name)
+		} else {
+			require.Error(t, test.msg.ValidateBasic(), "test: %v", test.name)
+		}
+	}
+}
+
+// TODO: Complete table driven tests for the remaining messages
+
+// MsgBeginUnlockingAll
+
+// MsgBeginUnlocking
+
+// MsgExtendLockup