From c8e03dfb61b0b6c47d1c148380aff7ed5b9a4ce9 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Tue, 19 Dec 2023 19:59:13 +0200 Subject: [PATCH 1/2] Add passthrough implementations for OnChanUpgradeOpen and OnChanUpgradeRestore for controller. --- .../controller/ibc_middleware.go | 31 +++- .../controller/ibc_middleware_test.go | 148 ++++++++++++++++++ 2 files changed, 177 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index 0e56b2af529..042aecb60f7 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -298,11 +298,38 @@ func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, cou } // OnChanUpgradeOpen implements the IBCModule interface -func (IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) { +func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) { + connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID) + if err != nil { + panic(err) + } + + if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionID) { + // Only cast to UpgradableModule if the application is set. + cbs, ok := im.app.(porttypes.UpgradableModule) + if !ok { + panic(errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")) + } + cbs.OnChanUpgradeOpen(ctx, portID, channelID, order, connectionHops, version) + } } // OnChanUpgradeRestore implements the IBCModule interface -func (IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {} +func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) { + connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID) + if err != nil { + panic(err) + } + + if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionID) { + // Only cast to UpgradableModule if the application is set. + cbs, ok := im.app.(porttypes.UpgradableModule) + if !ok { + panic(errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")) + } + cbs.OnChanUpgradeRestore(ctx, portID, channelID) + } +} // SendPacket implements the ICS4 Wrapper interface func (IBCMiddleware) SendPacket( diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go index 8cd161c3792..aea074ca886 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -960,6 +960,154 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { } } +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() { + var ( + path *ibctesting.Path + isNilApp bool + counterpartyVersion string + ) + + testCases := []struct { + name string + malleate func() + expError error + }{ + { + "success", func() {}, nil, + }, + { + "success: nil app", + func() { + isNilApp = true + }, + nil, + }, + { + "middleware disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID) + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID string, counterpartyVersion string) error { + return ibcmock.MockApplicationCallbackError + } + }, nil, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + isNilApp = false + + path = NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + counterpartyVersion = path.EndpointB.GetChannel().Version + + tc.malleate() // malleate mutates test data + + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + app, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + cbs, ok := app.(porttypes.UpgradableModule) + suite.Require().True(ok) + + if isNilApp { + cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper) + } + + cbs.OnChanUpgradeOpen( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + channeltypes.ORDERED, + []string{path.EndpointA.ConnectionID}, + counterpartyVersion, + ) + + if tc.expError == nil { + suite.Require().NoError(err) + } else { + suite.Require().ErrorIs(err, tc.expError) + } + }) + } +} + +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeRestore() { + var ( + path *ibctesting.Path + isNilApp bool + ) + + testCases := []struct { + name string + malleate func() + expError error + }{ + { + "success", func() {}, nil, + }, + { + "success: nil app", + func() { + isNilApp = true + }, + nil, + }, + { + "middleware disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID) + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID string, counterpartyVersion string) error { + return ibcmock.MockApplicationCallbackError + } + }, nil, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + suite.SetupTest() // reset + isNilApp = false + + path = NewICAPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + err := SetupICAPath(path, TestOwnerAddress) + suite.Require().NoError(err) + + tc.malleate() // malleate mutates test data + + module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + app, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + cbs, ok := app.(porttypes.UpgradableModule) + suite.Require().True(ok) + + if isNilApp { + cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper) + } + + cbs.OnChanUpgradeRestore(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + if tc.expError == nil { + suite.Require().NoError(err) + } else { + suite.Require().ErrorIs(err, tc.expError) + } + }) + } +} + func (suite *InterchainAccountsTestSuite) TestSingleHostMultipleControllers() { var ( pathAToB *ibctesting.Path From a2c32ed7f24d18b056261fac5bd8a0c40180be22 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 20 Dec 2023 09:55:23 +0200 Subject: [PATCH 2/2] Drop unused expError from test functions. --- .../controller/ibc_middleware_test.go | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go index b595eba52a0..568edb2b031 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -967,17 +967,16 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() { testCases := []struct { name string malleate func() - expError error }{ { - "success", func() {}, nil, + "success", + func() {}, }, { "success: nil app", func() { isNilApp = true }, - nil, }, { "middleware disabled", func() { @@ -985,7 +984,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() { suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID string, counterpartyVersion string) error { return ibcmock.MockApplicationCallbackError } - }, nil, + }, }, } @@ -1026,12 +1025,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() { []string{path.EndpointA.ConnectionID}, counterpartyVersion, ) - - if tc.expError == nil { - suite.Require().NoError(err) - } else { - suite.Require().ErrorIs(err, tc.expError) - } }) } } @@ -1045,17 +1038,15 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeRestore() { testCases := []struct { name string malleate func() - expError error }{ { - "success", func() {}, nil, + "success", func() {}, }, { "success: nil app", func() { isNilApp = true }, - nil, }, { "middleware disabled", func() { @@ -1063,7 +1054,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeRestore() { suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID string, counterpartyVersion string) error { return ibcmock.MockApplicationCallbackError } - }, nil, + }, }, } @@ -1095,12 +1086,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeRestore() { } cbs.OnChanUpgradeRestore(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - - if tc.expError == nil { - suite.Require().NoError(err) - } else { - suite.Require().ErrorIs(err, tc.expError) - } }) } }