nits: more ics29 nits (#741)

* nits: remove capital from error + add godoc

* nit: add Wrapf

* nit: use strings.TrimSpace

* nit: add err type for MsgPayPacketFee

* refactor: app version + add comment (#750)

* chore: remove error

* test: add test for whitespaced empty string

modules/apps/29-fee/ibc_module.go

@@ -56,6 +56,8 @@ func (im IBCModule) OnChanOpenInit(
 }
 
 // OnChanOpenTry implements the IBCModule interface
+// If the channel is not fee enabled the underlying application version will be returned
+// If the channel is fee enabled we merge the underlying application version with the ics29 version
 func (im IBCModule) OnChanOpenTry(
 	ctx sdk.Context,
 	order channeltypes.Order,
@@ -86,11 +88,11 @@ func (im IBCModule) OnChanOpenTry(
 		return "", err
 	}
 
-	if im.keeper.IsFeeEnabled(ctx, portID, channelID) {
-		return channeltypes.MergeChannelVersions(types.Version, appVersion), nil
+	if !im.keeper.IsFeeEnabled(ctx, portID, channelID) {
+		return appVersion, nil
 	}
 
-	return appVersion, nil
+	return channeltypes.MergeChannelVersions(types.Version, appVersion), nil
 }
 
 // OnChanOpenAck implements the IBCModule interface

modules/apps/29-fee/keeper/escrow.go

@@ -23,7 +23,7 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee types.IdentifiedP
 
 	hasRefundAcc := k.authKeeper.GetAccount(ctx, refundAcc)
 	if hasRefundAcc == nil {
-		return sdkerrors.Wrap(types.ErrRefundAccNotFound, fmt.Sprintf("Account with address: %s not found", refundAcc))
+		return sdkerrors.Wrapf(types.ErrRefundAccNotFound, "account with address: %s not found", refundAcc)
 	}
 
 	coins := identifiedFee.Fee.RecvFee
@@ -122,6 +122,7 @@ func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) e
 			refundErr = err
 			return true
 		}
+
 		err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.AckFee)
 		if err != nil {
 			refundErr = err

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

@@ -1,6 +1,8 @@
 package types
 
 import (
+	"strings"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 
@@ -28,7 +30,7 @@ func (msg MsgRegisterCounterpartyAddress) ValidateBasic() error {
 		return sdkerrors.Wrap(err, "failed to convert msg.Address into sdk.AccAddress")
 	}
 
-	if msg.CounterpartyAddress == "" {
+	if strings.TrimSpace(msg.CounterpartyAddress) == "" {
 		return ErrCounterpartyAddressEmpty
 	}
 
@@ -82,12 +84,12 @@ func (msg MsgPayPacketFee) ValidateBasic() error {
 
 	// 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.ErrInvalidCoins
+		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.ErrInvalidCoins
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, "contains one or more invalid fees")
 	}
 
 	return nil
@@ -119,7 +121,7 @@ func (msg MsgPayPacketFeeAsync) ValidateBasic() error {
 
 	err = msg.IdentifiedPacketFee.Validate()
 	if err != nil {
-		return sdkerrors.Wrap(err, "Invalid IdentifiedPacketFee")
+		return sdkerrors.Wrap(err, "invalid IdentifiedPacketFee")
 	}
 
 	return nil
@@ -144,11 +146,12 @@ func NewIdentifiedPacketFee(packetId channeltypes.PacketId, fee Fee, refundAddr
 	}
 }
 
+// Validate performs a stateless check of the IdentifiedPacketFee fields
 func (fee IdentifiedPacketFee) Validate() error {
 	// validate PacketId
 	err := fee.PacketId.Validate()
 	if err != nil {
-		return sdkerrors.Wrap(err, "Invalid PacketId")
+		return sdkerrors.Wrap(err, "invalid PacketId")
 	}
 
 	_, err = sdk.AccAddressFromBech32(fee.RefundAddress)

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

@@ -29,6 +29,7 @@ func TestMsgRegisterCountepartyAddressValidation(t *testing.T) {
 		{"validate with correct sdk.AccAddress", NewMsgRegisterCounterpartyAddress(validAddr, validAddr), true},
 		{"validate with incorrect destination relayer address", NewMsgRegisterCounterpartyAddress(invalidAddr, validAddr), false},
 		{"invalid counterparty address", NewMsgRegisterCounterpartyAddress(validAddr, ""), false},
+		{"invalid counterparty address: whitespaced empty string", NewMsgRegisterCounterpartyAddress(validAddr, " "), false},
 	}
 
 	for i, tc := range testCases {