From b618f02ad5b880bed2a8cc2da01f3898bd854528 Mon Sep 17 00:00:00 2001 From: Sean King Date: Fri, 21 Jan 2022 16:46:29 +0100 Subject: [PATCH] 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 * fix: error message * test: move Validate to fee.go & abstract out test * chore: remove test cases Co-authored-by: Damian Nolan --- modules/apps/29-fee/types/fee.go | 32 +++++++++ modules/apps/29-fee/types/fee_test.go | 96 ++++++++++++++++++++++++++ modules/apps/29-fee/types/msgs.go | 24 ++----- modules/apps/29-fee/types/msgs_test.go | 48 ------------- 4 files changed, 134 insertions(+), 66 deletions(-) create mode 100644 modules/apps/29-fee/types/fee.go create mode 100644 modules/apps/29-fee/types/fee_test.go diff --git a/modules/apps/29-fee/types/fee.go b/modules/apps/29-fee/types/fee.go new file mode 100644 index 00000000000..841a1a3a0c1 --- /dev/null +++ b/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 +} diff --git a/modules/apps/29-fee/types/fee_test.go b/modules/apps/29-fee/types/fee_test.go new file mode 100644 index 00000000000..0dbd9f29c4e --- /dev/null +++ b/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) + } + } +} diff --git a/modules/apps/29-fee/types/msgs.go b/modules/apps/29-fee/types/msgs.go index 2d06232313a..f3abe737472 100644 --- a/modules/apps/29-fee/types/msgs.go +++ b/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 } diff --git a/modules/apps/29-fee/types/msgs_test.go b/modules/apps/29-fee/types/msgs_test.go index 15a3b45dbde..57b38a07086 100644 --- a/modules/apps/29-fee/types/msgs_test.go +++ b/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 {