From baa0e8b546eddc75e5b6548f240fd8e0e526d801 Mon Sep 17 00:00:00 2001 From: bznein Date: Tue, 25 Jun 2024 16:25:15 +0100 Subject: [PATCH 1/6] Refactor validation --- modules/apps/transfer/types/msgs.go | 71 ++++++++++++++--------------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index eafb6abd59e..f36592cc118 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -69,10 +69,23 @@ func NewMsgTransfer( // NOTE: The recipient addresses format is not validated as the format defined by // the chain is not known to IBC. func (msg MsgTransfer) ValidateBasic() error { - if err := validateSourcePortAndChannel(msg); err != nil { - return err // The actual error and its message are already wrapped in the called function. - } + if msg.ShouldBeForwarded() { + if err := msg.validateForwarding(); err != nil { + return err + } + } else { + // We verify that portID and channelID are valid IDs only if + // we are not setting unwind to true. + // In that case, validation that they are empty is performed in + // validateForwarding(). + if err := host.PortIdentifierValidator(msg.SourcePort); err != nil { + return errorsmod.Wrap(err, "invalid source port ID") + } + if err := host.ChannelIdentifierValidator(msg.SourceChannel); err != nil { + return errorsmod.Wrap(err, "invalid source channel ID") + } + } if len(msg.Tokens) == 0 && !isValidIBCCoin(msg.Token) { return errorsmod.Wrap(ibcerrors.ErrInvalidCoins, "either token or token array must be filled") } @@ -99,30 +112,38 @@ func (msg MsgTransfer) ValidateBasic() error { return errorsmod.Wrapf(ErrInvalidMemo, "memo must not exceed %d bytes", MaximumMemoLength) } + for _, coin := range msg.GetCoins() { + if err := validateIBCCoin(coin); err != nil { + return errorsmod.Wrapf(ibcerrors.ErrInvalidCoins, "%s: %s", err.Error(), coin.String()) + } + } + + return nil +} + +func (msg MsgTransfer) validateForwarding() error { if err := msg.Forwarding.Validate(); err != nil { return err } - if msg.ShouldBeForwarded() { + if !msg.TimeoutHeight.IsZero() { // when forwarding, the timeout height must not be set - if !msg.TimeoutHeight.IsZero() { - return errorsmod.Wrapf(ErrInvalidPacketTimeout, "timeout height must not be set if forwarding path hops is not empty: %s, %s", msg.TimeoutHeight, msg.Forwarding.Hops) - } + return errorsmod.Wrapf(ErrInvalidPacketTimeout, "timeout height must not be set if forwarding path hops is not empty: %s, %s", msg.TimeoutHeight, msg.Forwarding.Hops) } if msg.Forwarding.Unwind { - // When unwinding, we must have at most one token. + if msg.SourcePort != "" { + return errorsmod.Wrapf(ErrInvalidForwarding, "source port must be empty when unwind is set, got %s instead", msg.SourcePort) + } + if msg.SourceChannel != "" { + return errorsmod.Wrapf(ErrInvalidForwarding, "source channel must be empty when unwind is set, got %s instead", msg.SourceChannel) + } if len(msg.GetCoins()) > 1 { + // When unwinding, we must have at most one token. return errorsmod.Wrap(ibcerrors.ErrInvalidCoins, "cannot unwind more than one token") } } - for _, coin := range msg.GetCoins() { - if err := validateIBCCoin(coin); err != nil { - return errorsmod.Wrapf(ibcerrors.ErrInvalidCoins, "%s: %s", err.Error(), coin.String()) - } - } - return nil } @@ -164,25 +185,3 @@ func validateIBCCoin(coin sdk.Coin) error { return nil } - -func validateSourcePortAndChannel(msg MsgTransfer) error { - // If unwind is set, we want to ensure that port and channel are empty. - if msg.Forwarding.Unwind { - if msg.SourcePort != "" { - return errorsmod.Wrapf(ErrInvalidForwarding, "source port must be empty when unwind is set, got %s instead", msg.SourcePort) - } - if msg.SourceChannel != "" { - return errorsmod.Wrapf(ErrInvalidForwarding, "source channel must be empty when unwind is set, got %s instead", msg.SourceChannel) - } - return nil - } - - // Otherwise, we just do the usual validation of the port and channel identifiers. - if err := host.PortIdentifierValidator(msg.SourcePort); err != nil { - return errorsmod.Wrap(err, "invalid source port ID") - } - if err := host.ChannelIdentifierValidator(msg.SourceChannel); err != nil { - return errorsmod.Wrap(err, "invalid source channel ID") - } - return nil -} From 2121f42692e92549958fa5caf744be0bb4d58bef Mon Sep 17 00:00:00 2001 From: bznein Date: Tue, 25 Jun 2024 16:29:16 +0100 Subject: [PATCH 2/6] Fixed verification logic, added two tests --- modules/apps/transfer/types/msgs.go | 2 +- modules/apps/transfer/types/msgs_test.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index f36592cc118..7b43666bef9 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -73,7 +73,7 @@ func (msg MsgTransfer) ValidateBasic() error { if err := msg.validateForwarding(); err != nil { return err } - } else { + } else if !msg.Forwarding.Unwind { // We verify that portID and channelID are valid IDs only if // we are not setting unwind to true. // In that case, validation that they are empty is performed in diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index a7d837ae741..0f4890aa63f 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -87,6 +87,8 @@ func TestMsgTransferValidation(t *testing.T) { {"invalid forwarding info port", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(false, types.Hop{PortId: invalidPort, ChannelId: validChannel})), host.ErrInvalidID}, {"invalid forwarding info channel", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(false, types.Hop{PortId: validPort, ChannelId: invalidChannel})), host.ErrInvalidID}, {"invalid forwarding info too many hops", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(false, generateHops(types.MaximumNumberOfForwardingHops+1)...)), types.ErrInvalidForwarding}, + {"invalid portID when forwarding is set but unwind is not", types.NewMsgTransfer("", validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(false, validHop)), host.ErrInvalidID}, + {"invalid channelID when forwarding is set but unwind is not", types.NewMsgTransfer(validPort, "", coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(false, validHop)), host.ErrInvalidID}, {"unwind specified but source port is not empty", types.NewMsgTransfer(validPort, "", coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(true)), types.ErrInvalidForwarding}, {"unwind specified but source channel is not empty", types.NewMsgTransfer("", validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(true)), types.ErrInvalidForwarding}, {"unwind specified but more than one coin in the message", types.NewMsgTransfer("", "", coins.Add(sdk.NewCoin("atom", ibctesting.TestCoin.Amount)), sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(true)), ibcerrors.ErrInvalidCoins}, From 05b64e2a2c325bb0783eb06c5a8d2b0dfbcba018 Mon Sep 17 00:00:00 2001 From: bznein Date: Tue, 25 Jun 2024 16:33:47 +0100 Subject: [PATCH 3/6] Fix check for unwind --- modules/apps/transfer/types/msgs.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 7b43666bef9..28b569fb7bc 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -73,7 +73,11 @@ func (msg MsgTransfer) ValidateBasic() error { if err := msg.validateForwarding(); err != nil { return err } - } else if !msg.Forwarding.Unwind { + } + // We might still enter here if len(hops) > 0 + // but unwind is false. In that case we need to validate + // source port and channel IDs. + if !msg.Forwarding.Unwind { // We verify that portID and channelID are valid IDs only if // we are not setting unwind to true. // In that case, validation that they are empty is performed in From 2944cae24356715adaaf50a459f0b3cfeeb3bdcf Mon Sep 17 00:00:00 2001 From: bznein Date: Tue, 25 Jun 2024 16:35:00 +0100 Subject: [PATCH 4/6] removed unneeded indirection --- modules/apps/transfer/types/msgs.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 28b569fb7bc..838021c1782 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -69,14 +69,9 @@ func NewMsgTransfer( // NOTE: The recipient addresses format is not validated as the format defined by // the chain is not known to IBC. func (msg MsgTransfer) ValidateBasic() error { - if msg.ShouldBeForwarded() { - if err := msg.validateForwarding(); err != nil { - return err - } + if err := msg.validateForwarding(); err != nil { + return err } - // We might still enter here if len(hops) > 0 - // but unwind is false. In that case we need to validate - // source port and channel IDs. if !msg.Forwarding.Unwind { // We verify that portID and channelID are valid IDs only if // we are not setting unwind to true. @@ -126,6 +121,9 @@ func (msg MsgTransfer) ValidateBasic() error { } func (msg MsgTransfer) validateForwarding() error { + if !msg.ShouldBeForwarded() { + return nil + } if err := msg.Forwarding.Validate(); err != nil { return err } From d89d44786af14b1b436b77e6731727d978a02abe Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Wed, 26 Jun 2024 10:48:14 +0100 Subject: [PATCH 5/6] Update modules/apps/transfer/types/msgs.go Co-authored-by: DimitrisJim --- modules/apps/transfer/types/msgs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 838021c1782..c2e6dcd5087 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -130,7 +130,7 @@ func (msg MsgTransfer) validateForwarding() error { if !msg.TimeoutHeight.IsZero() { // when forwarding, the timeout height must not be set - return errorsmod.Wrapf(ErrInvalidPacketTimeout, "timeout height must not be set if forwarding path hops is not empty: %s, %s", msg.TimeoutHeight, msg.Forwarding.Hops) + return errorsmod.Wrapf(ErrInvalidPacketTimeout, "timeout height must be zero if forwarding path hops is not empty: %s, %s", msg.TimeoutHeight, msg.Forwarding.Hops) } if msg.Forwarding.Unwind { From 6f4bc871d853c4db38627f1af888e4f994605064 Mon Sep 17 00:00:00 2001 From: bznein Date: Wed, 26 Jun 2024 10:51:48 +0100 Subject: [PATCH 6/6] Add docstring. --- modules/apps/transfer/types/msgs.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index c2e6dcd5087..be3420d233f 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -120,6 +120,7 @@ func (msg MsgTransfer) ValidateBasic() error { return nil } +// validateForwarding ensures that forwarding is set up correctly. func (msg MsgTransfer) validateForwarding() error { if !msg.ShouldBeForwarded() { return nil