refactor: condense v1 denom parsing to single function (#6424)

* refactor: reduce v1 denom parsing to single function in types package * fix: bug
cosmos · May 30, 2024 · 2a555af · 2a555af
1 parent 4816ae1
commit 2a555af
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 108 deletions.
diff --git a/modules/apps/transfer/internal/convert/convert.go b/modules/apps/transfer/internal/convert/convert.go
@@ -1,9 +1,6 @@
 package convert
 
 import (
-	"errors"
-	"strings"
-
 	errorsmod "cosmossdk.io/errors"
 
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
@@ -16,14 +13,11 @@ func PacketDataV1ToV2(packetData types.FungibleTokenPacketData) (types.FungibleT
 		return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(err, "invalid packet data")
 	}
 
-	v2Denom, trace := ExtractDenomAndTraceFromV1Denom(packetData.Denom)
+	denom := types.ExtractDenomFromFullPath(packetData.Denom)
 	return types.FungibleTokenPacketDataV2{
 		Tokens: []types.Token{
 			{
-				Denom: types.Denom{
-					Base:  v2Denom,
-					Trace: trace,
-				},
+				Denom:  denom,
 				Amount: packetData.Amount,
 			},
 		},
@@ -32,29 +26,3 @@ func PacketDataV1ToV2(packetData types.FungibleTokenPacketData) (types.FungibleT
 		Memo:     packetData.Memo,
 	}, nil
 }
-
-// extractDenomAndTraceFromV1Denom extracts the base denom and remaining trace from a v1 IBC denom.
-func ExtractDenomAndTraceFromV1Denom(v1Denom string) (string, []string) {
-	v1DenomTrace := types.ParseDenomTrace(v1Denom)
-
-	// if the path string is empty, then the base denom is the full native denom.
-	if v1DenomTrace.Path == "" {
-		return v1DenomTrace.BaseDenom, nil
-	}
-
-	splitPath := strings.Split(v1DenomTrace.Path, "/")
-
-	// this condition should never be reached.
-	if len(splitPath)%2 != 0 {
-		panic(errors.New("path slice length is not even"))
-	}
-
-	// the path slices consists of entries of ports and channel ids separately,
-	// we need to combine them to form the trace.
-	var trace []string
-	for i := 0; i < len(splitPath); i += 2 {
-		trace = append(trace, strings.Join(splitPath[i:i+2], "/"))
-	}
-
-	return v1DenomTrace.BaseDenom, trace
-}
diff --git a/modules/apps/transfer/keeper/mbt_relay_test.go b/modules/apps/transfer/keeper/mbt_relay_test.go
@@ -19,7 +19,6 @@ import (
 
 	"github.com/cometbft/cometbft/crypto"
 
-	convertinternal "github.com/cosmos/ibc-go/v8/modules/apps/transfer/internal/convert"
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
@@ -144,7 +143,7 @@ func BalancesFromTla(tla []TlaBalance) []Balance {
 }
 
 func FungibleTokenPacketFromTla(packet TlaFungibleTokenPacket) FungibleTokenPacket {
-	denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(DenomFromTla(packet.Data.Denom))
+	denom := types.ExtractDenomFromFullPath(DenomFromTla(packet.Data.Denom))
 	return FungibleTokenPacket{
 		SourceChannel: packet.SourceChannel,
 		SourcePort:    packet.SourcePort,
@@ -153,10 +152,7 @@ func FungibleTokenPacketFromTla(packet TlaFungibleTokenPacket) FungibleTokenPack
 		Data: types.NewFungibleTokenPacketDataV2(
 			[]types.Token{
 				{
-					Denom: types.Denom{
-						Base:  denom,
-						Trace: trace,
-					},
+					Denom:  denom,
 					Amount: packet.Data.Amount,
 				},
 			},

diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go
@@ -12,7 +12,6 @@ import (
 	banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil"
 	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
 
-	convertinternal "github.com/cosmos/ibc-go/v8/modules/apps/transfer/internal/convert"
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
@@ -524,14 +523,11 @@ func (suite *KeeperTestSuite) TestOnRecvPacket_ReceiverIsSource() {
 
 			tc.malleate()
 
-			denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(denomTrace.GetFullDenomPath())
+			denom := types.ExtractDenomFromFullPath(denomTrace.GetFullDenomPath())
 			data := types.NewFungibleTokenPacketDataV2(
 				[]types.Token{
 					{
-						Denom: types.Denom{
-							Base:  denom,
-							Trace: trace,
-						},
+						Denom:  denom,
 						Amount: amount.String(),
 					},
 				}, suite.chainA.SenderAccount.GetAddress().String(), receiver, memo)
@@ -605,14 +601,11 @@ func (suite *KeeperTestSuite) TestOnRecvPacketSetsTotalEscrowAmountForSourceIBCT
 		Path:      fmt.Sprintf("%s/%s/%s/%s", path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID),
 	}
 
-	denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(denomTrace.GetFullDenomPath())
+	denom := types.ExtractDenomFromFullPath(denomTrace.GetFullDenomPath())
 	data := types.NewFungibleTokenPacketDataV2(
 		[]types.Token{
 			{
-				Denom: types.Denom{
-					Base:  denom,
-					Trace: trace,
-				},
+				Denom:  denom,
 				Amount: amount.String(),
 			},
 		}, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), "")
@@ -739,14 +732,11 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() {
 
 			tc.malleate()
 
-			denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(denomTrace.GetFullDenomPath())
+			denom := types.ExtractDenomFromFullPath(denomTrace.GetFullDenomPath())
 			data := types.NewFungibleTokenPacketDataV2(
 				[]types.Token{
 					{
-						Denom: types.Denom{
-							Base:  denom,
-							Trace: trace,
-						},
+						Denom:  denom,
 						Amount: amount.String(),
 					},
 				}, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), "")
@@ -836,14 +826,11 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacketSetsTotalEscrowAmountFo
 		),
 	)
 
-	denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(denomTrace.GetFullDenomPath())
+	denom := types.ExtractDenomFromFullPath(denomTrace.GetFullDenomPath())
 	data := types.NewFungibleTokenPacketDataV2(
 		[]types.Token{
 			{
-				Denom: types.Denom{
-					Base:  denom,
-					Trace: trace,
-				},
+				Denom:  denom,
 				Amount: amount.String(),
 			},
 		},
@@ -967,14 +954,11 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() {
 
 			tc.malleate()
 
-			denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(denomTrace.GetFullDenomPath())
+			denom := types.ExtractDenomFromFullPath(denomTrace.GetFullDenomPath())
 			data := types.NewFungibleTokenPacketDataV2(
 				[]types.Token{
 					{
-						Denom: types.Denom{
-							Base:  denom,
-							Trace: trace,
-						},
+						Denom:  denom,
 						Amount: amount.String(),
 					},
 				}, sender, suite.chainB.SenderAccount.GetAddress().String(), "")
@@ -1057,14 +1041,11 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketSetsTotalEscrowAmountForSourceI
 		),
 	)
 
-	denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(denomTrace.GetFullDenomPath())
+	denom := types.ExtractDenomFromFullPath(denomTrace.GetFullDenomPath())
 	data := types.NewFungibleTokenPacketDataV2(
 		[]types.Token{
 			{
-				Denom: types.Denom{
-					Base:  denom,
-					Trace: trace,
-				},
+				Denom:  denom,
 				Amount: amount.String(),
 			},
 		}, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), "")

diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go
@@ -28,19 +28,10 @@ import (
 // - "gamm/pool/1" => DenomTrace{Path: "", BaseDenom: "gamm/pool/1"}
 // - "uatom" => DenomTrace{Path: "", BaseDenom: "uatom"}
 func ParseDenomTrace(rawDenom string) DenomTrace {
-	denomSplit := strings.Split(rawDenom, "/")
-
-	if denomSplit[0] == rawDenom {
-		return DenomTrace{
-			Path:      "",
-			BaseDenom: rawDenom,
-		}
-	}
-
-	pathSlice, baseDenom := extractPathAndBaseFromFullDenom(denomSplit)
+	denom := ExtractDenomFromFullPath(rawDenom)
 	return DenomTrace{
-		Path:      strings.Join(pathSlice, "/"),
-		BaseDenom: baseDenom,
+		Path:      strings.Join(denom.Trace, "/"),
+		BaseDenom: denom.Base,
 	}
 }
 
@@ -76,15 +67,23 @@ func (dt DenomTrace) GetFullDenomPath() string {
 	return dt.GetPrefix() + dt.BaseDenom
 }
 
-// extractPathAndBaseFromFullDenom returns the trace path and the base denom from
-// the elements that constitute the complete denom.
-func extractPathAndBaseFromFullDenom(fullDenomItems []string) ([]string, string) {
+// ExtractDenomFromFullPath returns the denom from the full path.
+// Used to support v1 denoms.
+func ExtractDenomFromFullPath(fullPath string) Denom {
+	denomSplit := strings.Split(fullPath, "/")
+
+	if denomSplit[0] == fullPath {
+		return Denom{
+			Base: fullPath,
+		}
+	}
+
 	var (
-		pathSlice      []string
+		trace          []string
 		baseDenomSlice []string
 	)
 
-	length := len(fullDenomItems)
+	length := len(denomSplit)
 	for i := 0; i < length; i += 2 {
 		// The IBC specification does not guarantee the expected format of the
 		// destination port or destination channel identifier. A short term solution
@@ -95,17 +94,20 @@ func extractPathAndBaseFromFullDenom(fullDenomItems []string) ([]string, string)
 		// will be incorrectly parsed, but the token will continue to be treated correctly
 		// as an IBC denomination. The hash used to store the token internally on our chain
 		// will be the same value as the base denomination being correctly parsed.
-		if i < length-1 && length > 2 && channeltypes.IsValidChannelID(fullDenomItems[i+1]) {
-			pathSlice = append(pathSlice, fullDenomItems[i], fullDenomItems[i+1])
+		if i < length-1 && length > 2 && channeltypes.IsValidChannelID(denomSplit[i+1]) {
+			trace = append(trace, denomSplit[i]+"/"+denomSplit[i+1])
 		} else {
-			baseDenomSlice = fullDenomItems[i:]
+			baseDenomSlice = denomSplit[i:]
 			break
 		}
 	}
 
-	baseDenom := strings.Join(baseDenomSlice, "/")
+	base := strings.Join(baseDenomSlice, "/")
 
-	return pathSlice, baseDenom
+	return Denom{
+		Base:  base,
+		Trace: trace,
+	}
 }
 
 // validateTraceIdentifiers validates the correctness of the trace associated with a particular base denom.
@@ -149,26 +151,20 @@ func (dt DenomTrace) Validate() error {
 //   - Unprefixed denomination: 'baseDenom'
 //
 // 'baseDenom' may or may not contain '/'s
-func ValidatePrefixedDenom(denom string) error {
-	denomSplit := strings.Split(denom, "/")
-	if denomSplit[0] == denom && strings.TrimSpace(denom) != "" {
-		// NOTE: no base denomination validation
-		return nil
-	}
-
-	pathSlice, baseDenom := extractPathAndBaseFromFullDenom(denomSplit)
-	if strings.TrimSpace(baseDenom) == "" {
+func ValidatePrefixedDenom(fullPath string) error {
+	denom := ExtractDenomFromFullPath(fullPath)
+	if strings.TrimSpace(denom.Base) == "" {
 		return errorsmod.Wrap(ErrInvalidDenomForTransfer, "base denomination cannot be blank")
 	}
 
-	path := strings.Join(pathSlice, "/")
-	if path == "" {
-		// NOTE: base denom contains slashes, so no base denomination validation
-		return nil
+	path := strings.Join(denom.Trace, "/")
+	if len(denom.Trace) != 0 {
+		identifiers := strings.Split(path, "/")
+		return validateTraceIdentifiers(identifiers)
+
 	}
 
-	identifiers := strings.Split(path, "/")
-	return validateTraceIdentifiers(identifiers)
+	return nil
 }
 
 // validateIBCDenom validates that the given denomination is either: