Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disallow ICA upgrades that change channel order + increase test coverage #5415

Merged
merged 8 commits into from
Dec 18, 2023
Merged

disallow ICA upgrades that change channel order + increase test coverage #5415

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6fda0c2
disallow changing ordering in ICA upgrade and increase test coverage
crodriguezvega Dec 14, 2023
c85485c
linting
crodriguezvega Dec 14, 2023
765896b
Merge branch '04-channel-upgrades' into carlos/disallow-ica-upgrades-…
crodriguezvega Dec 14, 2023
e0dfa47
Merge branch '04-channel-upgrades' into carlos/disallow-ica-upgrades-…
crodriguezvega Dec 14, 2023
9fdbd17
Merge branch '04-channel-upgrades' into carlos/disallow-ica-upgrades-…
crodriguezvega Dec 14, 2023
68de128
review comments
crodriguezvega Dec 14, 2023
53b7093
Merge branch '04-channel-upgrades' into carlos/disallow-ica-upgrades-…
crodriguezvega Dec 18, 2023
5d3325d
Merge branch '04-channel-upgrades' into carlos/disallow-ica-upgrades-…
crodriguezvega Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
199 changes: 198 additions & 1 deletion modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
ibctesting "github.com/cosmos/ibc-go/v8/testing"
)

const invalidVersion = "invalid|version"
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

var (
// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
Expand Down Expand Up @@ -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,
},
{
Expand Down Expand Up @@ -762,6 +764,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 = invalidVersion
}, 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)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
} 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)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
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 = invalidVersion
}, 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
Expand Down
5 changes: 5 additions & 0 deletions modules/apps/27-interchain-accounts/host/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
77 changes: 77 additions & 0 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
Loading