From 19aaf3f82550dd96bbbe6ff6f6b7fd1a5260d366 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 4 Oct 2021 11:07:10 +0200 Subject: [PATCH 1/2] correctly set active channels, implement delete OnChanCloseConfirm callback --- .../apps/27-interchain-accounts/ibc_module.go | 12 ++++- .../keeper/handshake.go | 19 +++++++- .../keeper/handshake_test.go | 44 +++++++++++++++++++ .../27-interchain-accounts/keeper/keeper.go | 6 +++ .../keeper/keeper_test.go | 3 +- 5 files changed, 79 insertions(+), 5 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module.go b/modules/apps/27-interchain-accounts/ibc_module.go index 80e798af826..076be284feb 100644 --- a/modules/apps/27-interchain-accounts/ibc_module.go +++ b/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,6 +146,7 @@ func (im IBCModule) OnAcknowledgementPacket( return nil } +// OnTimeoutPacket implements the IBCModule interface func (im IBCModule) OnTimeoutPacket( ctx sdk.Context, packet channeltypes.Packet, diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index 6b929f547d9..792cfd2f5d8 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -122,11 +122,28 @@ 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 } diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index ba2a1eea9ca..35cbac15a62 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -292,3 +292,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) + } + + }) + } +} diff --git a/modules/apps/27-interchain-accounts/keeper/keeper.go b/modules/apps/27-interchain-accounts/keeper/keeper.go index 832755817b5..c9d16d957f0 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper.go @@ -148,6 +148,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) diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index 1930f327bd9..18552547c7c 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -170,8 +170,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 } From 1c62d9976097785333bac1c6829a3887940f53c9 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 6 Oct 2021 18:09:15 +0200 Subject: [PATCH 2/2] removing active channel on packet timeout --- .../apps/27-interchain-accounts/ibc_module.go | 3 +- .../27-interchain-accounts/keeper/relay.go | 8 ++-- .../keeper/relay_test.go | 42 +++++++++++++++++++ 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/modules/apps/27-interchain-accounts/ibc_module.go b/modules/apps/27-interchain-accounts/ibc_module.go index 076be284feb..1c90abcc1e6 100644 --- a/modules/apps/27-interchain-accounts/ibc_module.go +++ b/modules/apps/27-interchain-accounts/ibc_module.go @@ -152,8 +152,7 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, _ sdk.AccAddress, ) error { - // TODO - return nil + return im.keeper.OnTimeoutPacket(ctx, packet) } // NegotiateAppVersion implements the IBCModule interface diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 09175043dec..ba63d165734 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -232,10 +232,10 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac } } -func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, data types.IBCAccountPacketData) error { - if k.hook != nil { - k.hook.OnTxFailed(ctx, packet.SourcePort, packet.SourceChannel, k.ComputeVirtualTxHash(data.Data, packet.Sequence), 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 } diff --git a/modules/apps/27-interchain-accounts/keeper/relay_test.go b/modules/apps/27-interchain-accounts/keeper/relay_test.go index 78d407934d8..11658981f4d 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/keeper/relay_test.go @@ -225,3 +225,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) + } + }) + } +}