From bd28658acbd8e8601c015a84955f98795a6cbea8 Mon Sep 17 00:00:00 2001 From: Sean King Date: Mon, 17 Jan 2022 14:51:39 +0100 Subject: [PATCH 1/7] nits: remove capital from error + add godoc --- modules/apps/29-fee/keeper/escrow.go | 3 ++- modules/apps/29-fee/types/msgs.go | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 9ba1709a8dc..c03c008f929 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -23,7 +23,7 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee *types.Identified 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.Wrap(types.ErrRefundAccNotFound, fmt.Sprintf("account with address: %s not found", refundAcc)) } coins := identifiedFee.Fee.ReceiveFee @@ -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 diff --git a/modules/apps/29-fee/types/msgs.go b/modules/apps/29-fee/types/msgs.go index 7138706cf80..ebde8748692 100644 --- a/modules/apps/29-fee/types/msgs.go +++ b/modules/apps/29-fee/types/msgs.go @@ -119,7 +119,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 +144,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) From 7227885d95795ac41eedc5b9594733c7a7fea8ee Mon Sep 17 00:00:00 2001 From: Sean King Date: Mon, 17 Jan 2022 14:55:04 +0100 Subject: [PATCH 2/7] nit: add Wrapf --- modules/apps/29-fee/keeper/escrow.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index c03c008f929..4187c791703 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -23,7 +23,7 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee *types.Identified 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.ReceiveFee From 4d3708225636fb50b87a49bad102c02a9b8998a4 Mon Sep 17 00:00:00 2001 From: Sean King Date: Mon, 17 Jan 2022 14:59:09 +0100 Subject: [PATCH 3/7] nit: use strings.TrimSpace --- modules/apps/29-fee/types/msgs.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/apps/29-fee/types/msgs.go b/modules/apps/29-fee/types/msgs.go index ebde8748692..b60d5416bd5 100644 --- a/modules/apps/29-fee/types/msgs.go +++ b/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 } From bd9aae1321da6c8af4b70aa080cbf1504df1a48b Mon Sep 17 00:00:00 2001 From: Sean King Date: Mon, 17 Jan 2022 15:04:33 +0100 Subject: [PATCH 4/7] nit: add err type for MsgPayPacketFee --- modules/apps/29-fee/types/errors.go | 1 + modules/apps/29-fee/types/msgs.go | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/types/errors.go b/modules/apps/29-fee/types/errors.go index 55ce843e92c..db6df1cd478 100644 --- a/modules/apps/29-fee/types/errors.go +++ b/modules/apps/29-fee/types/errors.go @@ -14,4 +14,5 @@ var ( ErrCounterpartyAddressEmpty = sdkerrors.Register(ModuleName, 7, "counterparty address must not be empty") ErrForwardRelayerAddressNotFound = sdkerrors.Register(ModuleName, 8, "forward relayer address not found") ErrFeeNotEnabled = sdkerrors.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled") + ErrInvalidFee = sdkerrors.Register(ModuleName, 10, "one or more fees are invalid") ) diff --git a/modules/apps/29-fee/types/msgs.go b/modules/apps/29-fee/types/msgs.go index b60d5416bd5..a54f3c54d81 100644 --- a/modules/apps/29-fee/types/msgs.go +++ b/modules/apps/29-fee/types/msgs.go @@ -84,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.ReceiveFee.IsValid() || !msg.Fee.TimeoutFee.IsValid() { - return sdkerrors.ErrInvalidCoins + return ErrInvalidFee } // if all three fee's are zero or empty return an error if msg.Fee.AckFee.IsZero() && msg.Fee.ReceiveFee.IsZero() && msg.Fee.TimeoutFee.IsZero() { - return sdkerrors.ErrInvalidCoins + return ErrInvalidFee } return nil From f72e34dcfc609ab12fd5ae83f2944d0485907268 Mon Sep 17 00:00:00 2001 From: Sean King Date: Tue, 18 Jan 2022 16:39:56 +0100 Subject: [PATCH 5/7] refactor: app version + add comment (#750) --- modules/apps/29-fee/ibc_module.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index 1b54981d7ea..ac4ea21ca52 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/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 From 1a851ec24bd256f42bab975ab5e1eb8c40827f21 Mon Sep 17 00:00:00 2001 From: Sean King Date: Thu, 20 Jan 2022 15:49:12 +0100 Subject: [PATCH 6/7] chore: remove error --- modules/apps/29-fee/types/errors.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/29-fee/types/errors.go b/modules/apps/29-fee/types/errors.go index db6df1cd478..55ce843e92c 100644 --- a/modules/apps/29-fee/types/errors.go +++ b/modules/apps/29-fee/types/errors.go @@ -14,5 +14,4 @@ var ( ErrCounterpartyAddressEmpty = sdkerrors.Register(ModuleName, 7, "counterparty address must not be empty") ErrForwardRelayerAddressNotFound = sdkerrors.Register(ModuleName, 8, "forward relayer address not found") ErrFeeNotEnabled = sdkerrors.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled") - ErrInvalidFee = sdkerrors.Register(ModuleName, 10, "one or more fees are invalid") ) From 45a161ce79314bf7cb92bfac92edb087fb30c90c Mon Sep 17 00:00:00 2001 From: Sean King Date: Thu, 20 Jan 2022 15:51:20 +0100 Subject: [PATCH 7/7] test: add test for whitespaced empty string --- modules/apps/29-fee/types/msgs_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/apps/29-fee/types/msgs_test.go b/modules/apps/29-fee/types/msgs_test.go index 8671694bca0..8f7a0dc4cd7 100644 --- a/modules/apps/29-fee/types/msgs_test.go +++ b/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 {