chore: pull out hop validation and consolidate for transfer+packet (#…

…6695) * chore: pull out hop validation and consolidate transfer+packet * Update modules/apps/transfer/types/forwarding_test.go Co-authored-by: Carlos Rodriguez <[email protected]> * cr fixes --------- Co-authored-by: Carlos Rodriguez <[email protected]>
cosmos · Jun 26, 2024 · 74397f2 · 74397f2
1 parent a8904da
commit 74397f2
Show file tree

Hide file tree

Showing 4 changed files with 122 additions and 36 deletions.
diff --git a/modules/apps/transfer/types/forwarding.go b/modules/apps/transfer/types/forwarding.go
@@ -18,17 +18,8 @@ func NewForwarding(unwind bool, hops ...Hop) Forwarding {
 
 // Validate performs a basic validation of the Forwarding fields.
 func (fi Forwarding) Validate() error {
- if len(fi.Hops) > MaximumNumberOfForwardingHops {
- return errorsmod.Wrapf(ErrInvalidForwarding, "number of hops in forwarding cannot exceed %d", MaximumNumberOfForwardingHops)
- }
-
- for _, hop := range fi.Hops {
- if err := host.PortIdentifierValidator(hop.PortId); err != nil {
- return errorsmod.Wrapf(err, "invalid hop source port ID %s", hop.PortId)
- }
- if err := host.ChannelIdentifierValidator(hop.ChannelId); err != nil {
- return errorsmod.Wrapf(err, "invalid source channel ID %s", hop.ChannelId)
- }
+ if err := validateHops(fi.Hops); err != nil {
+ return errorsmod.Wrapf(ErrInvalidForwarding, "invalid hops in forwarding")
  }
 
  return nil
@@ -44,8 +35,8 @@ func NewForwardingPacketData(destinationMemo string, hops ...Hop) ForwardingPack
 
 // Validate performs a basic validation of the ForwardingPacketData fields.
 func (fi ForwardingPacketData) Validate() error {
- if len(fi.Hops) > MaximumNumberOfForwardingHops {
- return errorsmod.Wrapf(ErrInvalidForwarding, "number of hops in forwarding packet data cannot exceed %d", MaximumNumberOfForwardingHops)
+ if err := validateHops(fi.Hops); err != nil {
+ return errorsmod.Wrapf(ErrInvalidForwarding, "invalid hops in forwarding packet data")
  }
 
  if len(fi.DestinationMemo) > MaximumMemoLength {
@@ -56,12 +47,32 @@ func (fi ForwardingPacketData) Validate() error {
  return errorsmod.Wrap(ErrInvalidForwarding, "memo specified when forwarding packet data hops is empty")
  }
 
- for _, hop := range fi.Hops {
- if err := host.PortIdentifierValidator(hop.PortId); err != nil {
- return errorsmod.Wrapf(err, "invalid source port ID %s", hop.PortId)
- }
- if err := host.ChannelIdentifierValidator(hop.ChannelId); err != nil {
- return errorsmod.Wrapf(err, "invalid source channel ID %s", hop.ChannelId)
+ return nil
+}
+
+// Validate performs a basic validation of the Hop fields.
+func (h Hop) Validate() error {
+ if err := host.PortIdentifierValidator(h.PortId); err != nil {
+ return errorsmod.Wrapf(err, "invalid hop source port ID %s", h.PortId)
+ }
+ if err := host.ChannelIdentifierValidator(h.ChannelId); err != nil {
+ return errorsmod.Wrapf(err, "invalid source channel ID %s", h.ChannelId)
+ }
+
+ return nil
+}
+
+// validateHops performs a basic validation of the hops.
+// It checks that the number of hops does not exceed the maximum allowed and that each hop is valid.
+// It will not return any errors if hops is empty.
+func validateHops(hops []Hop) error {
+ if len(hops) > MaximumNumberOfForwardingHops {
+ return errorsmod.Wrapf(ErrInvalidForwarding, "number of hops cannot exceed %d", MaximumNumberOfForwardingHops)
+ }
+
+ for _, hop := range hops {
+ if err := hop.Validate(); err != nil {
+ return err
  }
  }
 

diff --git a/modules/apps/transfer/types/forwarding_test.go b/modules/apps/transfer/types/forwarding_test.go
@@ -50,7 +50,7 @@ func TestForwarding_Validate(t *testing.T) {
  ChannelId: ibctesting.FirstChannelID,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with too long hop port ID",
@@ -61,7 +61,7 @@ func TestForwarding_Validate(t *testing.T) {
  ChannelId: ibctesting.FirstChannelID,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with non-alpha hop port ID",
@@ -72,7 +72,7 @@ func TestForwarding_Validate(t *testing.T) {
  ChannelId: ibctesting.FirstChannelID,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with too long hop channel ID",
@@ -83,7 +83,7 @@ func TestForwarding_Validate(t *testing.T) {
  ChannelId: invalidLongChannel,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with too short hop channel ID",
@@ -94,7 +94,7 @@ func TestForwarding_Validate(t *testing.T) {
  ChannelId: invalidShortChannel,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with non-alpha hop channel ID",
@@ -105,7 +105,7 @@ func TestForwarding_Validate(t *testing.T) {
  ChannelId: invalidChannel,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  }
  for _, tc := range tests {
@@ -179,7 +179,7 @@ func TestForwardingPacketData_Validate(t *testing.T) {
  ChannelId: ibctesting.FirstChannelID,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with too long hop port ID",
@@ -190,7 +190,7 @@ func TestForwardingPacketData_Validate(t *testing.T) {
  ChannelId: ibctesting.FirstChannelID,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with non-alpha hop port ID",
@@ -201,7 +201,7 @@ func TestForwardingPacketData_Validate(t *testing.T) {
  ChannelId: ibctesting.FirstChannelID,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with too long hop channel ID",
@@ -212,7 +212,7 @@ func TestForwardingPacketData_Validate(t *testing.T) {
  ChannelId: invalidLongChannel,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with too short hop channel ID",
@@ -223,7 +223,7 @@ func TestForwardingPacketData_Validate(t *testing.T) {
  ChannelId: invalidShortChannel,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "invalid forwarding with non-alpha hop channel ID",
@@ -234,7 +234,7 @@ func TestForwardingPacketData_Validate(t *testing.T) {
  ChannelId: invalidChannel,
  },
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  }
  for _, tc := range tests {
@@ -253,6 +253,82 @@ func TestForwardingPacketData_Validate(t *testing.T) {
  }
 }
 
+func TestValidateHop(t *testing.T) {
+ tests := []struct {
+ name string
+ hop types.Hop
+ expError error
+ }{
+ {
+ "valid hop",
+ validHop,
+ nil,
+ },
+ {
+ "invalid hop with too short port ID",
+ types.Hop{
+ PortId: invalidShortPort,
+ ChannelId: ibctesting.FirstChannelID,
+ },
+ host.ErrInvalidID,
+ },
+ {
+ "invalid hop with too long port ID",
+ types.Hop{
+ PortId: invalidLongPort,
+ ChannelId: ibctesting.FirstChannelID,
+ },
+ host.ErrInvalidID,
+ },
+ {
+ "invalid hop with non-alpha port ID",
+ types.Hop{
+ PortId: invalidPort,
+ ChannelId: ibctesting.FirstChannelID,
+ },
+ host.ErrInvalidID,
+ },
+ {
+ "invalid hop with too long channel ID",
+ types.Hop{
+ PortId: types.PortID,
+ ChannelId: invalidLongChannel,
+ },
+ host.ErrInvalidID,
+ },
+ {
+ "invalid hop with too short channel ID",
+ types.Hop{
+ PortId: types.PortID,
+ ChannelId: invalidShortChannel,
+ },
+ host.ErrInvalidID,
+ },
+ {
+ "invalid hop with non-alpha channel ID",
+ types.Hop{
+ PortId: types.PortID,
+ ChannelId: invalidChannel,
+ },
+ host.ErrInvalidID,
+ },
+ }
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ tc := tc
+
+ err := tc.hop.Validate()
+
+ expPass := tc.expError == nil
+ if expPass {
+ require.NoError(t, err)
+ } else {
+ require.ErrorIs(t, err, tc.expError)
+ }
+ })
+ }
+}
+
 // generateHops generates a slice of n correctly initialized hops.
 func generateHops(n int) []types.Hop {
  hops := make([]types.Hop, n)

diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go
@@ -84,8 +84,8 @@ func TestMsgTransferValidation(t *testing.T) {
  {"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
  {"multidenom: both token and tokens are set", &types.MsgTransfer{validPort, validChannel, coin, sender, receiver, clienttypes.ZeroHeight(), 100, "", coins, emptyForwarding}, ibcerrors.ErrInvalidCoins},
  {"timeout height must be zero if forwarding path hops is not empty", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 100, "memo", types.NewForwarding(false, validHop)), types.ErrInvalidPacketTimeout},
- {"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 port", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(false, types.Hop{PortId: invalidPort, ChannelId: validChannel})), types.ErrInvalidForwarding},
+ {"invalid forwarding info channel", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(false, types.Hop{PortId: validPort, ChannelId: invalidChannel})), types.ErrInvalidForwarding},
  {"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},
  {"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},

diff --git a/modules/apps/transfer/types/packet_test.go b/modules/apps/transfer/types/packet_test.go
@@ -8,7 +8,6 @@ import (
  "github.com/stretchr/testify/require"
 
  "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
- host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
  ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
  ibctesting "github.com/cosmos/ibc-go/v8/testing"
 )
@@ -428,7 +427,7 @@ func TestFungibleTokenPacketDataV2ValidateBasic(t *testing.T) {
  },
  ),
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "failure: invalid forwarding path channel ID",
@@ -450,7 +449,7 @@ func TestFungibleTokenPacketDataV2ValidateBasic(t *testing.T) {
  },
  ),
  ),
- host.ErrInvalidID,
+ types.ErrInvalidForwarding,
  },
  {
  "failure: invalid forwarding path too many hops",