ica: TrySendTx error handling nits (#491)

* updating error handling and msgs for TrySendTx flow

* renaming active channel ID getter/setters, adding comment re indeterminate errs

* renaming DeleteActiveChannel -> DeleteActiveChannelID

modules/apps/27-interchain-accounts/keeper/account_test.go

@@ -39,7 +39,7 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
 			func() {
 				portID, err := types.GeneratePortID(owner, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
 				suite.Require().NoError(err)
-				suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, path.EndpointA.ChannelID)
+				suite.chainA.GetSimApp().ICAKeeper.SetActiveChannelID(suite.chainA.GetContext(), portID, path.EndpointA.ChannelID)
 			},
 			false,
 		},

modules/apps/27-interchain-accounts/keeper/handshake.go

@@ -57,9 +57,9 @@ func (k Keeper) OnChanOpenInit(
 		return sdkerrors.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.VersionPrefix, version)
 	}
 
-	existingChannelID, found := k.GetActiveChannel(ctx, portID)
+	activeChannelID, found := k.GetActiveChannelID(ctx, portID)
 	if found {
-		return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "existing active channel %s for portID %s", existingChannelID, portID)
+		return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "existing active channel %s for portID %s", activeChannelID, portID)
 	}
 
 	// Claim channel capability passed back by IBC module
@@ -152,7 +152,7 @@ func (k Keeper) OnChanOpenAck(
 		return sdkerrors.Wrap(err, "counterparty version validation failed")
 	}
 
-	k.SetActiveChannel(ctx, portID, channelID)
+	k.SetActiveChannelID(ctx, portID, channelID)
 
 	accAddr, err := types.ParseAddressFromVersion(counterpartyVersion)
 	if err != nil {
@@ -173,7 +173,7 @@ func (k Keeper) OnChanOpenConfirm(
 	channelID string,
 ) error {
 
-	k.SetActiveChannel(ctx, portID, channelID)
+	k.SetActiveChannelID(ctx, portID, channelID)
 
 	return nil
 }
@@ -185,7 +185,7 @@ func (k Keeper) OnChanCloseConfirm(
 	channelID string,
 ) error {
 
-	k.DeleteActiveChannel(ctx, portID)
+	k.DeleteActiveChannelID(ctx, portID)
 
 	return nil
 }

modules/apps/27-interchain-accounts/keeper/handshake_test.go

@@ -99,7 +99,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
 		{
 			"channel is already active",
 			func() {
-				suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+				suite.chainA.GetSimApp().ICAKeeper.SetActiveChannelID(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
 			},
 			false,
 		},
@@ -325,7 +325,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 func (suite *KeeperTestSuite) TestOnChanOpenAck() {
 	var (
 		path                *ibctesting.Path
-		expectedChannel     string
+		expectedChannelID   string
 		counterpartyVersion string
 	)
 
@@ -339,7 +339,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() {
 		},
 		{
 			"invalid counterparty version", func() {
-				expectedChannel = ""
+				expectedChannelID = ""
 				counterpartyVersion = "version"
 			}, false,
 		},
@@ -359,17 +359,17 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() {
 
 			err = path.EndpointB.ChanOpenTry()
 			suite.Require().NoError(err)
-			expectedChannel = path.EndpointA.ChannelID
+			expectedChannelID = path.EndpointA.ChannelID
 
 			tc.malleate() // explicitly change fields in channel and testChannel
 
 			err = suite.chainA.GetSimApp().ICAKeeper.OnChanOpenAck(suite.chainA.GetContext(),
 				path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, counterpartyVersion,
 			)
 
-			activeChannel, _ := suite.chainA.GetSimApp().ICAKeeper.GetActiveChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
+			activeChannelID, _ := suite.chainA.GetSimApp().ICAKeeper.GetActiveChannelID(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
 
-			suite.Require().Equal(activeChannel, expectedChannel)
+			suite.Require().Equal(activeChannelID, expectedChannelID)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
@@ -459,12 +459,12 @@ func (suite *KeeperTestSuite) TestOnChanCloseConfirm() {
 			err = suite.chainB.GetSimApp().ICAKeeper.OnChanCloseConfirm(suite.chainB.GetContext(),
 				path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
 
-			activeChannel, found := suite.chainB.GetSimApp().ICAKeeper.GetActiveChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
+			activeChannelID, found := suite.chainB.GetSimApp().ICAKeeper.GetActiveChannelID(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
 				suite.Require().False(found)
-				suite.Require().Empty(activeChannel)
+				suite.Require().Empty(activeChannelID)
 			} else {
 				suite.Require().Error(err)
 			}

modules/apps/27-interchain-accounts/keeper/keeper.go

@@ -99,10 +99,10 @@ func (k Keeper) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability
 	return k.scopedKeeper.ClaimCapability(ctx, cap, name)
 }
 
-// GetActiveChannel retrieves the active channelID from the store keyed by the provided portID
-func (k Keeper) GetActiveChannel(ctx sdk.Context, portId string) (string, bool) {
+// GetActiveChannelID retrieves the active channelID from the store keyed by the provided portID
+func (k Keeper) GetActiveChannelID(ctx sdk.Context, portID string) (string, bool) {
 	store := ctx.KVStore(k.storeKey)
-	key := types.KeyActiveChannel(portId)
+	key := types.KeyActiveChannel(portID)
 
 	if !store.Has(key) {
 		return "", false
@@ -111,21 +111,21 @@ func (k Keeper) GetActiveChannel(ctx sdk.Context, portId string) (string, bool)
 	return string(store.Get(key)), true
 }
 
-// SetActiveChannel stores the active channelID, keyed by the provided portID
-func (k Keeper) SetActiveChannel(ctx sdk.Context, portID, channelID string) {
+// SetActiveChannelID stores the active channelID, keyed by the provided portID
+func (k Keeper) SetActiveChannelID(ctx sdk.Context, portID, channelID string) {
 	store := ctx.KVStore(k.storeKey)
 	store.Set(types.KeyActiveChannel(portID), []byte(channelID))
 }
 
-// DeleteActiveChannel removes the active channel keyed by the provided portID stored in state
-func (k Keeper) DeleteActiveChannel(ctx sdk.Context, portID string) {
+// DeleteActiveChannelID removes the active channel keyed by the provided portID stored in state
+func (k Keeper) DeleteActiveChannelID(ctx sdk.Context, portID string) {
 	store := ctx.KVStore(k.storeKey)
 	store.Delete(types.KeyActiveChannel(portID))
 }
 
 // IsActiveChannel returns true if there exists an active channel for the provided portID, otherwise false
 func (k Keeper) IsActiveChannel(ctx sdk.Context, portID string) bool {
-	_, ok := k.GetActiveChannel(ctx, portID)
+	_, ok := k.GetActiveChannelID(ctx, portID)
 	return ok
 }
 

modules/apps/27-interchain-accounts/keeper/relay.go

@@ -14,20 +14,20 @@ import (
 // manage helper module access to owner addresses they do not have capabilities for
 func (k Keeper) TrySendTx(ctx sdk.Context, portID string, icaPacketData types.InterchainAccountPacketData) (uint64, error) {
 	// Check for the active channel
-	activeChannelId, found := k.GetActiveChannel(ctx, portID)
+	activeChannelID, found := k.GetActiveChannelID(ctx, portID)
 	if !found {
-		return 0, types.ErrActiveChannelNotFound
+		return 0, sdkerrors.Wrapf(types.ErrActiveChannelNotFound, "failed to retrieve active channel for port %s", portID)
 	}
 
-	sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelId)
+	sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID)
 	if !found {
-		return 0, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelId)
+		return 0, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelID)
 	}
 
 	destinationPort := sourceChannelEnd.GetCounterparty().GetPortID()
 	destinationChannel := sourceChannelEnd.GetCounterparty().GetChannelID()
 
-	return k.createOutgoingPacket(ctx, portID, activeChannelId, destinationPort, destinationChannel, icaPacketData)
+	return k.createOutgoingPacket(ctx, portID, activeChannelID, destinationPort, destinationChannel, icaPacketData)
 }
 
 func (k Keeper) createOutgoingPacket(
@@ -50,7 +50,7 @@ func (k Keeper) createOutgoingPacket(
 	// get the next sequence
 	sequence, found := k.channelKeeper.GetNextSequenceSend(ctx, sourcePort, sourceChannel)
 	if !found {
-		return 0, channeltypes.ErrSequenceSendNotFound
+		return 0, sdkerrors.Wrapf(channeltypes.ErrSequenceSendNotFound, "failed to retrieve next sequence send for channel %s on port %s", sourceChannel, sourcePort)
 	}
 
 	// timeoutTimestamp is set to be a max number here so that we never recieve a timeout
@@ -147,7 +147,8 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error
 	var data types.InterchainAccountPacketData
 
 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
-		return sdkerrors.Wrapf(types.ErrUnknownPacketData, "cannot unmarshal ICS-27 interchain account packet data")
+		// UnmarshalJSON errors are indeterminate and therefore are not wrapped and included in failed acks
+		return sdkerrors.Wrapf(types.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain account packet data")
 	}
 
 	switch data.Type {
@@ -164,7 +165,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error
 
 		return nil
 	default:
-		return types.ErrUnknownPacketData
+		return types.ErrUnknownDataType
 	}
 }
 
@@ -175,7 +176,7 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac
 // OnTimeoutPacket removes the active channel associated with the provided packet, the underlying channel end is closed
 // due to the semantics of ORDERED channels
 func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet) error {
-	k.DeleteActiveChannel(ctx, packet.SourcePort)
+	k.DeleteActiveChannelID(ctx, packet.SourcePort)
 
 	return nil
 }

modules/apps/27-interchain-accounts/keeper/relay_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types"
 	clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
-
 	ibctesting "github.com/cosmos/ibc-go/v2/testing"
 )
 
@@ -52,7 +51,7 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
 		},
 		{
 			"channel does not exist", func() {
-				suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, "channel-100")
+				suite.chainA.GetSimApp().ICAKeeper.SetActiveChannelID(suite.chainA.GetContext(), portID, "channel-100")
 			}, false,
 		},
 		{
@@ -255,11 +254,11 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() {
 			packet := channeltypes.NewPacket([]byte{}, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(0, 100), 0)
 			err = suite.chainA.GetSimApp().ICAKeeper.OnTimeoutPacket(suite.chainA.GetContext(), packet)
 
-			channel, found := suite.chainA.GetSimApp().ICAKeeper.GetActiveChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
+			activeChannelID, found := suite.chainA.GetSimApp().ICAKeeper.GetActiveChannelID(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
-				suite.Require().Empty(channel)
+				suite.Require().Empty(activeChannelID)
 				suite.Require().False(found)
 			} else {
 				suite.Require().Error(err)

modules/apps/27-interchain-accounts/types/errors.go

@@ -5,7 +5,7 @@ import (
 )
 
 var (
-	ErrUnknownPacketData           = sdkerrors.Register(ModuleName, 2, "unknown packet data")
+	ErrUnknownDataType             = sdkerrors.Register(ModuleName, 2, "unknown data type")
 	ErrAccountAlreadyExist         = sdkerrors.Register(ModuleName, 3, "account already exist")
 	ErrPortAlreadyBound            = sdkerrors.Register(ModuleName, 4, "port is already bound")
 	ErrUnsupportedChain            = sdkerrors.Register(ModuleName, 5, "unsupported chain")