chore: correctly set/delete active channels (#463)

* correctly set active channels, implement delete OnChanCloseConfirm callback

* removing active channel on packet timeout

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

@@ -29,7 +29,7 @@ func NewIBCModule(k keeper.Keeper, app porttypes.IBCModule) IBCModule {
 	}
 }
 
-// Implement IBCModule callbacks
+// OnChanOpenInit implements the IBCModule interface
 func (im IBCModule) OnChanOpenInit(
 	ctx sdk.Context,
 	order channeltypes.Order,
@@ -43,6 +43,7 @@ func (im IBCModule) OnChanOpenInit(
 	return im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version)
 }
 
+// OnChanOpenTry implements the IBCModule interface
 func (im IBCModule) OnChanOpenTry(
 	ctx sdk.Context,
 	order channeltypes.Order,
@@ -57,6 +58,7 @@ func (im IBCModule) OnChanOpenTry(
 	return im.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion)
 }
 
+// OnChanOpenAck implements the IBCModule interface
 func (im IBCModule) OnChanOpenAck(
 	ctx sdk.Context,
 	portID,
@@ -66,6 +68,7 @@ func (im IBCModule) OnChanOpenAck(
 	return im.keeper.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion)
 }
 
+// OnChanOpenConfirm implements the IBCModule interface
 func (im IBCModule) OnChanOpenConfirm(
 	ctx sdk.Context,
 	portID,
@@ -74,6 +77,7 @@ func (im IBCModule) OnChanOpenConfirm(
 	return im.keeper.OnChanOpenConfirm(ctx, portID, channelID)
 }
 
+// OnChanCloseInit implements the IBCModule interface
 func (im IBCModule) OnChanCloseInit(
 	ctx sdk.Context,
 	portID,
@@ -83,14 +87,16 @@ func (im IBCModule) OnChanCloseInit(
 	return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel")
 }
 
+// OnChanCloseConfirm implements the IBCModule interface
 func (im IBCModule) OnChanCloseConfirm(
 	ctx sdk.Context,
 	portID,
 	channelID string,
 ) error {
-	return nil
+	return im.keeper.OnChanCloseConfirm(ctx, portID, channelID)
 }
 
+// OnRecvPacket implements the IBCModule interface
 func (im IBCModule) OnRecvPacket(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
@@ -116,6 +122,7 @@ func (im IBCModule) OnRecvPacket(
 	return ack
 }
 
+// OnAcknowledgementPacket implements the IBCModule interface
 func (im IBCModule) OnAcknowledgementPacket(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
@@ -139,13 +146,13 @@ func (im IBCModule) OnAcknowledgementPacket(
 	return nil
 }
 
+// OnTimeoutPacket implements the IBCModule interface
 func (im IBCModule) OnTimeoutPacket(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
 ) error {
-	// TODO
-	return nil
+	return im.keeper.OnTimeoutPacket(ctx, packet)
 }
 
 // NegotiateAppVersion implements the IBCModule interface

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

@@ -164,12 +164,29 @@ func (k Keeper) OnChanOpenAck(
 	return nil
 }
 
-// Set active channel
+// OnChanOpenConfirm completes the handshake process by setting the active channel in state on the host chain
+//
+// Host Chain
 func (k Keeper) OnChanOpenConfirm(
 	ctx sdk.Context,
 	portID,
 	channelID string,
 ) error {
+
+	k.SetActiveChannel(ctx, portID, channelID)
+
+	return nil
+}
+
+// OnChanCloseConfirm removes the active channel stored in state
+func (k Keeper) OnChanCloseConfirm(
+	ctx sdk.Context,
+	portID,
+	channelID string,
+) error {
+
+	k.DeleteActiveChannel(ctx, portID)
+
 	return nil
 }
 

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

@@ -428,3 +428,47 @@ func (suite *KeeperTestSuite) TestOnChanOpenConfirm() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestOnChanCloseConfirm() {
+	var (
+		path *ibctesting.Path
+	)
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expPass  bool
+	}{
+
+		{
+			"success", func() {}, true,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(tc.name, func() {
+			suite.SetupTest() // reset
+			path = NewICAPath(suite.chainA, suite.chainB)
+			suite.coordinator.SetupConnections(path)
+
+			err := SetupICAPath(path, TestOwnerAddress)
+			suite.Require().NoError(err)
+
+			tc.malleate() // explicitly change fields in channel and testChannel
+
+			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)
+
+			if tc.expPass {
+				suite.Require().NoError(err)
+				suite.Require().False(found)
+				suite.Require().Empty(activeChannel)
+			} else {
+				suite.Require().Error(err)
+			}
+
+		})
+	}
+}

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

@@ -134,6 +134,12 @@ func (k Keeper) SetActiveChannel(ctx sdk.Context, portID, channelID string) {
 	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) {
+	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)

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

@@ -169,8 +169,7 @@ func (suite *KeeperTestSuite) SetupICAPath(path *ibctesting.Path, owner string)
 		return err
 	}
 
-	if err := suite.chainB.GetSimApp().ICAKeeper.OnChanOpenConfirm(suite.chainA.GetContext(),
-		path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID); err != nil {
+	if err := path.EndpointB.ChanOpenConfirm(); err != nil {
 		return err
 	}
 

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

@@ -233,10 +233,10 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac
 	}
 }
 
-func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, data types.InterchainAccountPacketData) error {
-	if k.hook != nil {
-		k.hook.OnTxFailed(ctx, packet.SourcePort, packet.SourceChannel, packet.Data, data.Data)
-	}
+// 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)
 
 	return nil
 }

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

@@ -220,3 +220,45 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestOnTimeoutPacket() {
+	var (
+		path *ibctesting.Path
+	)
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expPass  bool
+	}{
+		{
+			"success", func() {}, true,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(tc.name, func() {
+			suite.SetupTest() // reset
+			path = NewICAPath(suite.chainA, suite.chainB)
+			suite.coordinator.SetupConnections(path)
+
+			err := suite.SetupICAPath(path, TestOwnerAddress)
+			suite.Require().NoError(err)
+
+			tc.malleate() // malleate mutates test data
+
+			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)
+
+			if tc.expPass {
+				suite.Require().NoError(err)
+				suite.Require().Empty(channel)
+				suite.Require().False(found)
+			} else {
+				suite.Require().Error(err)
+			}
+		})
+	}
+}