From 6fda0c22385ed63f6d2d8d0a584646fbd87f21bb Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 14 Dec 2023 13:16:54 +0100 Subject: [PATCH 1/3] disallow changing ordering in ICA upgrade and increase test coverage --- .../controller/ibc_middleware.go | 5 + .../controller/ibc_middleware_test.go | 195 ++++++++++++++++++ .../27-interchain-accounts/host/ibc_module.go | 5 + .../host/ibc_module_test.go | 77 +++++++ 4 files changed, 282 insertions(+) diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index 8828f6314bc..0e56b2af529 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -243,6 +243,11 @@ func (im IBCMiddleware) OnChanUpgradeInit(ctx sdk.Context, portID, channelID str return "", types.ErrControllerSubModuleDisabled } + // support for unordered ICA channels is not implemented yet + if order != channeltypes.ORDERED { + return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order) + } + version, err := im.keeper.OnChanUpgradeInit(ctx, portID, channelID, connectionHops, version) if err != nil { return "", err 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 3394c1af332..53ec187be81 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -762,6 +762,201 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() { } } +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() { + var ( + path *ibctesting.Path + isNilApp bool + order channeltypes.Order + version string + ) + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", func() {}, true, + }, + { + "controller submodule disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false)) + }, false, + }, + { + "unordered channels not supported", func() { + order = channeltypes.UNORDERED + }, false, + }, + { + "ICA OnChanUpgradeInit fails - invalid version", func() { + version = "invalid|version" + }, false, + }, + { + "ICA auth module callback fails", func() { + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeInit = func(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) { + return "", fmt.Errorf("mock ica auth fails") + } + }, false, + }, + { + "nil underlying app", func() { + isNilApp = true + }, true, + }, + { + "middleware disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID) + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeInit = func(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) { + return "", fmt.Errorf("error should be unreachable") + } + }, true, + }, + } + + 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 := RegisterInterchainAccount(path.EndpointA, TestOwnerAddress) + suite.Require().NoError(err) + + order = channeltypes.ORDERED + metadata := icatypes.NewDefaultMetadata(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) + version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata)) + + 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) + } else { + version, err = cbs.OnChanUpgradeInit( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + order, + []string{path.EndpointA.ConnectionID}, + version, + ) + } + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + suite.Require().Empty(version) + } + }) + } +} + +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { + var ( + path *ibctesting.Path + isNilApp bool + counterpartyVersion string + ) + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", func() {}, true, + }, + { + "controller submodule disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false)) + }, false, + }, + { + "ICA OnChanUpgradeAck fails - invalid version", func() { + counterpartyVersion = "invalid|version" + }, false, + }, + { + "ICA auth module callback fails", func() { + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID string, counterpartyVersion string) error { + return fmt.Errorf("mock ica auth fails") + } + }, false, + }, + { + "nil underlying app", func() { + isNilApp = true + }, true, + }, + { + "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 fmt.Errorf("error should be unreachable") + } + }, true, + }, + } + + 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) + } else { + err = cbs.OnChanUpgradeAck( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + counterpartyVersion, + ) + } + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } +} + func (suite *InterchainAccountsTestSuite) TestSingleHostMultipleControllers() { var ( pathAToB *ibctesting.Path diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index bbc68506fc4..2c210f24296 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -168,6 +168,11 @@ func (im IBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, return "", types.ErrHostSubModuleDisabled } + // support for unordered ICA channels is not implemented yet + if order != channeltypes.ORDERED { + return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order) + } + return im.keeper.OnChanUpgradeTry(ctx, portID, channelID, order, connectionHops, counterpartyVersion) } diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index a5bb9381e66..19df715aa45 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -20,6 +20,7 @@ import ( feetypes "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/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" + porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" "github.com/cosmos/ibc-go/v8/modules/core/exported" ibctesting "github.com/cosmos/ibc-go/v8/testing" @@ -603,6 +604,82 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() { } } +func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { + var order channeltypes.Order + + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", func() {}, true, + }, + { + "host submodule disabled", func() { + suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{})) + }, false, + }, + { + "unordered channels not supported", func() { + order = channeltypes.UNORDERED + }, false, + }, + } + + for _, tc := range testCases { + tc := tc + + 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) + + interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID) + suite.Require().True(found) + + order = channeltypes.ORDERED + metadata := icatypes.NewDefaultMetadata(path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) + metadata.Address = interchainAccountAddr + metadata.Encoding = icatypes.EncodingProto3JSON // this is the actual change to the version + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata)) + + err = path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) + + tc.malleate() // malleate mutates test data + + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID) + suite.Require().NoError(err) + + app, ok := suite.chainB.App.GetIBCKeeper().Router.GetRoute(module) + suite.Require().True(ok) + cbs, ok := app.(porttypes.UpgradableModule) + suite.Require().True(ok) + + version, err := cbs.OnChanUpgradeTry( + suite.chainB.GetContext(), + path.EndpointB.ChannelConfig.PortID, + path.EndpointB.ChannelID, + order, + []string{path.EndpointB.ConnectionID}, + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version, + ) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + suite.Require().Empty(version) + } + }) + } +} + func (suite *InterchainAccountsTestSuite) fundICAWallet(ctx sdk.Context, portID string, amount sdk.Coins) { interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(ctx, ibctesting.FirstConnectionID, portID) suite.Require().True(found) From c85485c801fdc8be60f7f35943529107b8638e65 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 14 Dec 2023 13:39:03 +0100 Subject: [PATCH 2/3] linting --- .../controller/ibc_middleware_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 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 53ec187be81..4a872d1d295 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -20,6 +20,8 @@ import ( ibctesting "github.com/cosmos/ibc-go/v8/testing" ) +const invalidVersion = "invalid|version" + var ( // TestOwnerAddress defines a reusable bech32 address for testing purposes TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs" @@ -327,7 +329,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { }, { "ICA OnChanOpenACK fails - invalid version", func() { - path.EndpointB.ChannelConfig.Version = "invalid|version" + path.EndpointB.ChannelConfig.Version = invalidVersion }, false, }, { @@ -790,7 +792,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() { }, { "ICA OnChanUpgradeInit fails - invalid version", func() { - version = "invalid|version" + version = invalidVersion }, false, }, { @@ -887,7 +889,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { }, { "ICA OnChanUpgradeAck fails - invalid version", func() { - counterpartyVersion = "invalid|version" + counterpartyVersion = invalidVersion }, false, }, { From 68de128b9f16b59f364f532ea66f9c1acec8aa49 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 14 Dec 2023 22:18:59 +0100 Subject: [PATCH 3/3] review comments --- .../controller/ibc_middleware_test.go | 79 ++++++++++--------- .../host/ibc_module_test.go | 10 +-- 2 files changed, 45 insertions(+), 44 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 4a872d1d295..8cd161c3792 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -18,6 +18,7 @@ import ( porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v8/testing" + ibcmock "github.com/cosmos/ibc-go/v8/testing/mock" ) const invalidVersion = "invalid|version" @@ -775,45 +776,45 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() { testCases := []struct { name string malleate func() - expPass bool + expError error }{ { - "success", func() {}, true, + "success", func() {}, nil, }, { "controller submodule disabled", func() { suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false)) - }, false, + }, types.ErrControllerSubModuleDisabled, }, { "unordered channels not supported", func() { order = channeltypes.UNORDERED - }, false, + }, channeltypes.ErrInvalidChannelOrdering, }, { "ICA OnChanUpgradeInit fails - invalid version", func() { version = invalidVersion - }, false, + }, icatypes.ErrUnknownDataType, }, { "ICA auth module callback fails", func() { suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeInit = func(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) { - return "", fmt.Errorf("mock ica auth fails") + return "", ibcmock.MockApplicationCallbackError } - }, false, + }, ibcmock.MockApplicationCallbackError, }, { "nil underlying app", func() { isNilApp = true - }, true, + }, porttypes.ErrInvalidRoute, }, { "middleware disabled", func() { suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID) suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeInit = func(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) { - return "", fmt.Errorf("error should be unreachable") + return "", ibcmock.MockApplicationCallbackError } - }, true, + }, nil, }, } @@ -846,21 +847,21 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() { if isNilApp { cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper) - } else { - version, err = cbs.OnChanUpgradeInit( - suite.chainA.GetContext(), - path.EndpointA.ChannelConfig.PortID, - path.EndpointA.ChannelID, - order, - []string{path.EndpointA.ConnectionID}, - version, - ) } - if tc.expPass { + version, err = cbs.OnChanUpgradeInit( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + order, + []string{path.EndpointA.ConnectionID}, + version, + ) + + if tc.expError == nil { suite.Require().NoError(err) } else { - suite.Require().Error(err) + suite.Require().ErrorIs(err, tc.expError) suite.Require().Empty(version) } }) @@ -877,40 +878,40 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { testCases := []struct { name string malleate func() - expPass bool + expError error }{ { - "success", func() {}, true, + "success", func() {}, nil, }, { "controller submodule disabled", func() { suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false)) - }, false, + }, types.ErrControllerSubModuleDisabled, }, { "ICA OnChanUpgradeAck fails - invalid version", func() { counterpartyVersion = invalidVersion - }, false, + }, icatypes.ErrUnknownDataType, }, { "ICA auth module callback fails", func() { suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanUpgradeAck = func(ctx sdk.Context, portID, channelID string, counterpartyVersion string) error { - return fmt.Errorf("mock ica auth fails") + return ibcmock.MockApplicationCallbackError } - }, false, + }, ibcmock.MockApplicationCallbackError, }, { "nil underlying app", func() { isNilApp = true - }, true, + }, porttypes.ErrInvalidRoute, }, { "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 fmt.Errorf("error should be unreachable") + return ibcmock.MockApplicationCallbackError } - }, true, + }, nil, }, } @@ -941,19 +942,19 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() { if isNilApp { cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper) - } else { - err = cbs.OnChanUpgradeAck( - suite.chainA.GetContext(), - path.EndpointA.ChannelConfig.PortID, - path.EndpointA.ChannelID, - counterpartyVersion, - ) } - if tc.expPass { + err = cbs.OnChanUpgradeAck( + suite.chainA.GetContext(), + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + counterpartyVersion, + ) + + if tc.expError == nil { suite.Require().NoError(err) } else { - suite.Require().Error(err) + suite.Require().ErrorIs(err, tc.expError) } }) } diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index 19df715aa45..84de1b00364 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module_test.go @@ -610,20 +610,20 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { testCases := []struct { name string malleate func() - expPass bool + expError error }{ { - "success", func() {}, true, + "success", func() {}, nil, }, { "host submodule disabled", func() { suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{})) - }, false, + }, types.ErrHostSubModuleDisabled, }, { "unordered channels not supported", func() { order = channeltypes.UNORDERED - }, false, + }, channeltypes.ErrInvalidChannelOrdering, }, } @@ -670,7 +670,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() { path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version, ) - if tc.expPass { + if tc.expError == nil { suite.Require().NoError(err) } else { suite.Require().Error(err)