hygiene: add validate fn for Fee (#748)

* hygiene: add validate fn for Fee

* Update modules/apps/29-fee/types/msgs.go

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

* fix: error message

* test: move Validate to fee.go & abstract out test

* chore: remove test cases

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

modules/apps/29-fee/types/fee.go

@@ -0,0 +1,32 @@
+package types
+
+import (
+	"strings"
+
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+)
+
+// Validate asserts that each Fee is valid and all three Fees are not empty or zero
+func (fee Fee) Validate() error {
+	var errFees []string
+	if !fee.AckFee.IsValid() {
+		errFees = append(errFees, "ack fee invalid")
+	}
+	if !fee.RecvFee.IsValid() {
+		errFees = append(errFees, "recv fee invalid")
+	}
+	if !fee.TimeoutFee.IsValid() {
+		errFees = append(errFees, "timeout fee invalid")
+	}
+
+	if len(errFees) > 0 {
+		return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "contains invalid fees: %s", strings.Join(errFees, " , "))
+	}
+
+	// if all three fee's are zero or empty return an error
+	if fee.AckFee.IsZero() && fee.RecvFee.IsZero() && fee.TimeoutFee.IsZero() {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, "all fees are zero")
+	}
+
+	return nil
+}

modules/apps/29-fee/types/fee_test.go

@@ -0,0 +1,96 @@
+package types
+
+import (
+	"testing"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/stretchr/testify/require"
+)
+
+// TestFeeValidation tests Validate
+func TestFeeValidation(t *testing.T) {
+	var (
+		fee        Fee
+		ackFee     sdk.Coins
+		receiveFee sdk.Coins
+		timeoutFee sdk.Coins
+	)
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expPass  bool
+	}{
+		{
+			"success",
+			func() {},
+			true,
+		},
+		{
+			"should fail when all fees are invalid",
+			func() {
+				ackFee = invalidCoins
+				receiveFee = invalidCoins
+				timeoutFee = invalidCoins
+			},
+			false,
+		},
+		{
+			"should fail with single invalid fee",
+			func() {
+				ackFee = invalidCoins
+			},
+			false,
+		},
+		{
+			"should fail with two invalid fees",
+			func() {
+				timeoutFee = invalidCoins
+				ackFee = invalidCoins
+			},
+			false,
+		},
+		{
+			"should pass with two empty fees",
+			func() {
+				timeoutFee = sdk.Coins{}
+				ackFee = sdk.Coins{}
+			},
+			true,
+		},
+		{
+			"should pass with one empty fee",
+			func() {
+				timeoutFee = sdk.Coins{}
+			},
+			true,
+		},
+		{
+			"should fail if all fees are empty",
+			func() {
+				ackFee = sdk.Coins{}
+				receiveFee = sdk.Coins{}
+				timeoutFee = sdk.Coins{}
+			},
+			false,
+		},
+	}
+
+	for _, tc := range testCases {
+		// build message
+		ackFee = validCoins
+		receiveFee = validCoins
+		timeoutFee = validCoins
+
+		// malleate
+		tc.malleate()
+		fee = Fee{receiveFee, ackFee, timeoutFee}
+		err := fee.Validate()
+
+		if tc.expPass {
+			require.NoError(t, err)
+		} else {
+			require.Error(t, err)
+		}
+	}
+}

modules/apps/29-fee/types/msgs.go

@@ -80,14 +80,8 @@ func (msg MsgPayPacketFee) ValidateBasic() error {
 		return ErrRelayersNotNil
 	}
 
-	// if any of the fee's are invalid return an error
-	if !msg.Fee.AckFee.IsValid() || !msg.Fee.RecvFee.IsValid() || !msg.Fee.TimeoutFee.IsValid() {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, "contains one or more invalid fees")
-	}
-
-	// if all three fee's are zero or empty return an error
-	if msg.Fee.AckFee.IsZero() && msg.Fee.RecvFee.IsZero() && msg.Fee.TimeoutFee.IsZero() {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, "contains one or more invalid fees")
+	if err := msg.Fee.Validate(); err != nil {
+		return err
 	}
 
 	return nil
@@ -185,20 +179,14 @@ func (fee IdentifiedPacketFee) Validate() error {
 		return sdkerrors.Wrap(err, "failed to convert RefundAddress into sdk.AccAddress")
 	}
 
-	// if any of the fee's are invalid return an error
-	if !fee.Fee.AckFee.IsValid() || !fee.Fee.RecvFee.IsValid() || !fee.Fee.TimeoutFee.IsValid() {
-		return sdkerrors.ErrInvalidCoins
-	}
-
-	// if all three fee's are zero or empty return an error
-	if fee.Fee.AckFee.IsZero() && fee.Fee.RecvFee.IsZero() && fee.Fee.TimeoutFee.IsZero() {
-		return sdkerrors.ErrInvalidCoins
-	}
-
 	// enforce relayer is nil
 	if fee.Relayers != nil {
 		return ErrRelayersNotNil
 	}
 
+	if err := fee.Fee.Validate(); err != nil {
+		return err
+	}
+
 	return nil
 }

modules/apps/29-fee/types/msgs_test.go

@@ -105,54 +105,6 @@ func TestMsgPayPacketFeeValidation(t *testing.T) {
 			},
 			false,
 		},
-		{
-			"should fail when all fees are invalid",
-			func() {
-				ackFee = invalidCoins
-				receiveFee = invalidCoins
-				timeoutFee = invalidCoins
-			},
-			false,
-		},
-		{
-			"should fail with single invalid fee",
-			func() {
-				ackFee = invalidCoins
-			},
-			false,
-		},
-		{
-			"should fail with two invalid fees",
-			func() {
-				timeoutFee = invalidCoins
-				ackFee = invalidCoins
-			},
-			false,
-		},
-		{
-			"should pass with two empty fees",
-			func() {
-				timeoutFee = sdk.Coins{}
-				ackFee = sdk.Coins{}
-			},
-			true,
-		},
-		{
-			"should pass with one empty fee",
-			func() {
-				timeoutFee = sdk.Coins{}
-			},
-			true,
-		},
-		{
-			"should fail if all fees are empty",
-			func() {
-				ackFee = sdk.Coins{}
-				receiveFee = sdk.Coins{}
-				timeoutFee = sdk.Coins{}
-			},
-			false,
-		},
 	}
 
 	for _, tc := range testCases {