Skip to content

Commit

Permalink
refactor: use explicit checks in ica controller upgrade handlers (#5472…
Browse files Browse the repository at this point in the history 
…) (#5518)

* refactor: explicit checks in upgrade init and upgrade ack for ica controller

Updated UpgradeInit logic
Updated UpgradeAck logic
Refactored UpgradeInit tests

* test: complete upgrade ack tests

* test: add code cov

* lint appease

* nit: use correct error type

* nit comment change

* fix typo

---------

Co-authored-by: Cian Hatton <cian@interchain.io>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 4a021a3)

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
  • Loading branch information
@mergify @colin-axner
mergify[bot] and colin-axner authored Jan 5, 2024
1 parent c8973a0 commit 5a9db30
Show file tree
Hide file tree
Showing 2 changed files with 198 additions and 51 deletions.
95 changes: 71 additions & 24 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,17 +142,42 @@ func (Keeper) OnChanCloseConfirm(
}

// OnChanUpgradeInit performs the upgrade init step of the channel upgrade handshake.
// The upgrade init callback must verify the proposed changes to the order, connectionHops, and version.
// Within the version we have the tx type, encoding, interchain account address, host/controller connectionID's
// and the ICS27 protocol version.
//
// The following may be changed:
// - tx type (must be supported)
// - encoding (must be supported)
//
// The following may not be changed:
// - order
// - connectionHops (and subsequently host/controller connectionIDs)
// - interchain account address
// - ICS27 protocol version
func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) {
if strings.TrimSpace(version) == "" {
return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
}

// verify order has not changed
// 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)
}

metadata, err := icatypes.MetadataFromVersion(version)
// verify connection hops has not changed
connectionID, err := k.GetConnectionID(ctx, portID, channelID)
if err != nil {
return "", err
}

if len(connectionHops) != 1 || connectionHops[0] != connectionID {
return "", errorsmod.Wrapf(channeltypes.ErrInvalidUpgrade, "expected connection hops %s, got %s", []string{connectionID}, connectionHops)
}

// verify proposed version only modifies tx type or encoding
if strings.TrimSpace(version) == "" {
return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
}

proposedMetadata, err := icatypes.MetadataFromVersion(version)
if err != nil {
return "", err
}
Expand All @@ -162,30 +187,49 @@ func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, ord
return "", err
}

if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
return "", errorsmod.Wrap(err, "invalid metadata")
// ValidateControllerMetadata will ensure the ICS27 protocol version has not changed and that the
// tx type and encoding are supported
if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, proposedMetadata); err != nil {
return "", errorsmod.Wrap(err, "invalid upgrade metadata")
}

// the interchain account address on the host chain
// must remain the same after the upgrade.
if currentMetadata.Address != metadata.Address {
if currentMetadata.Address != proposedMetadata.Address {
return "", errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "interchain account address cannot be changed")
}

if currentMetadata.ControllerConnectionId != connectionHops[0] {
return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed connection hop must not change")
if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed controller connection ID must not change")
}

if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed host connection ID must not change")
}

return version, nil
}

// OnChanUpgradeAck implements the ack setup of the channel upgrade handshake.
// The upgrade ack callback must verify the proposed changes to the channel version.
// Within the channel version we have the tx type, encoding, interchain account address, host/controller connectionID's
// and the ICS27 protocol version.
//
// The following may be changed:
// - tx type (must be supported)
// - encoding (must be supported)
//
// The following may not be changed:
// - controller connectionID
// - host connectionID
// - interchain account address
// - ICS27 protocol version
func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
if strings.TrimSpace(counterpartyVersion) == "" {
return errorsmod.Wrap(channeltypes.ErrInvalidChannelVersion, "counterparty version cannot be empty")
}

metadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
proposedMetadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
if err != nil {
return err
}
Expand All @@ -195,27 +239,30 @@ func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpart
return err
}

// the interchain account address on the host chain
// must remain the same after the upgrade.
if currentMetadata.Address != metadata.Address {
return errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "address cannot be changed")
channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
}

if currentMetadata.ControllerConnectionId != metadata.ControllerConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed controller connection ID must not change")
// ValidateControllerMetadata will ensure the ICS27 protocol version has not changed and that the
// tx type and encoding are supported. Note, we pass in the current channel connection hops. The upgrade init
// step will verify that the proposed connection hops will not change.
if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, proposedMetadata); err != nil {
return errorsmod.Wrap(err, "invalid upgrade metadata")
}

if currentMetadata.HostConnectionId != metadata.HostConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed host connection ID must not change")
// the interchain account address on the host chain
// must remain the same after the upgrade.
if currentMetadata.Address != proposedMetadata.Address {
return errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "address cannot be changed")
}

channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed controller connection ID must not change")
}

if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, metadata); err != nil {
return err
if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
return errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed host connection ID must not change")
}

return nil
Expand Down
154 changes: 127 additions & 27 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
connectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
ibctesting "github.com/cosmos/ibc-go/v8/testing"
)

Expand Down Expand Up @@ -478,12 +479,26 @@ func (suite *KeeperTestSuite) TestOnChanCloseConfirm() {
}

func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
const (
invalidVersion = "invalid-version"
)

var (
path *ibctesting.Path
metadata icatypes.Metadata
version string
order channeltypes.Order
)

// updateMetadata is a helper function which modifies the metadata stored in the channel version
// and marshals it into a string to pass to OnChanUpgradeInit as the version string.
updateMetadata := func(modificationFn func(*icatypes.Metadata)) {
metadata, err := icatypes.MetadataFromVersion(path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version)
suite.Require().NoError(err)
modificationFn(&metadata)
version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
}

testCases := []struct {
name string
malleate func()
Expand All @@ -495,48 +510,104 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
nil,
},
{
name: "failure: change ICA address",
name: "failure: invalid order",
malleate: func() {
metadata.Address = TestOwnerAddress
order = channeltypes.UNORDERED
},
expError: icatypes.ErrInvalidAccountAddress,
expError: channeltypes.ErrInvalidChannelOrdering,
},
{
name: "failure: change controller connection id",
name: "failure: connectionID not found",
malleate: func() {
metadata.ControllerConnectionId = differentConnectionID
// channelID is provided via the endpoint channelID
path.EndpointA.ChannelID = "invalid channel"
},
expError: connectiontypes.ErrInvalidConnection,
expError: channeltypes.ErrChannelNotFound,
},
{
name: "failure: change host connection id",
name: "failure: invalid proposed connectionHops",
malleate: func() {
metadata.HostConnectionId = differentConnectionID
// connection hops is provided via endpoint connectionID
path.EndpointA.ConnectionID = differentConnectionID
},
expError: connectiontypes.ErrInvalidConnection,
expError: channeltypes.ErrInvalidUpgrade,
},
{
name: "failure: empty version",
malleate: func() {
version = ""
},
expError: icatypes.ErrInvalidVersion,
},
{
name: "failure: cannot decode version string",
malleate: func() {
channel := path.EndpointA.GetChannel()
channel.Version = "invalid-metadata-string"
path.EndpointA.SetChannel(channel)
version = invalidVersion
},
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: invalid connection hops",
name: "failure: cannot decode self version string",
malleate: func() {
path.EndpointA.ConnectionID = differentConnectionID
ch := path.EndpointA.GetChannel()
ch.Version = invalidVersion
path.EndpointA.SetChannel(ch)
},
expError: connectiontypes.ErrConnectionNotFound,
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: invalid order",
name: "failure: failed controller metadata validation, invalid encoding",
malleate: func() {
order = channeltypes.UNORDERED
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Encoding = "invalid-encoding"
})
},
expError: channeltypes.ErrInvalidChannelOrdering,
expError: icatypes.ErrInvalidCodec,
},
{
name: "failure: failed controller metadata validation, invalid tx type",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.TxType = "invalid-tx-type"
})
},
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: failed controller metadata validation, invalid interchain account version",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Version = "invalid-interchain-account-version"
})
},
expError: icatypes.ErrInvalidVersion,
},
{
name: "failure: interchain account address changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Address = TestOwnerAddress // use valid address
})
},
expError: icatypes.ErrInvalidAccountAddress,
},
{
name: "failure: controller connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.ControllerConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the controller connection identifier are unreachable
},
{
name: "failure: host connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.HostConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the host connection identifier are unreachable
},
}

Expand All @@ -563,9 +634,12 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
// this is the actual change to the version.
metadata.Encoding = icatypes.EncodingProto3JSON

tc.malleate() // malleate mutates test data
path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))

version := string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
version = path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version

tc.malleate() // malleate mutates test data

upgradeVersion, err := path.EndpointA.Chain.GetSimApp().ICAControllerKeeper.OnChanUpgradeInit(
path.EndpointA.Chain.GetContext(),
Expand Down Expand Up @@ -642,7 +716,24 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: invalid tx type",
name: "failure: channel not found",
malleate: func() {
// channelID is provided via the endpoint channelID
path.EndpointA.ChannelID = "invalid channel"
},
expError: ibcerrors.ErrNotFound, // GetChannel error is unreachable
},
{
name: "failure: failed controller metadata validation, invalid encoding",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Encoding = "invalid-encoding"
})
},
expError: icatypes.ErrInvalidCodec,
},
{
name: "failure: failed controller metadata validation, invalid tx type",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.TxType = "invalid-tx-type"
Expand All @@ -651,10 +742,19 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
expError: icatypes.ErrUnknownDataType,
},
{
name: "failure: interchain account address has changed",
name: "failure: failed controller metadata validation, invalid interchain account version",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Version = "invalid-interchain-account-version"
})
},
expError: icatypes.ErrInvalidVersion,
},
{
name: "failure: interchain account address changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.Address = "different-address"
metadata.Address = TestOwnerAddress // use valid address
})
},
expError: icatypes.ErrInvalidAccountAddress,
Expand All @@ -663,19 +763,19 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
name: "failure: controller connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.ControllerConnectionId = "different-connection-id"
metadata.ControllerConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnectionIdentifier,
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the controller identifier are unreachable
},
{
name: "failure: host connection ID has changed",
malleate: func() {
updateMetadata(func(metadata *icatypes.Metadata) {
metadata.HostConnectionId = "different-host-id"
metadata.HostConnectionId = differentConnectionID
})
},
expError: connectiontypes.ErrInvalidConnectionIdentifier,
expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the host identifier are unreachable
},
}

Expand Down

0 comments on commit 5a9db30

Please sign in to comment.