Skip to content

Commit

Permalink
chore: ics27 middleware callback routing (#2157)
Browse files Browse the repository at this point in the history
* [WIP] add middleware enabled flags and conditional logic

* adapting private registerInterchainAccount func to accept portID in favour of owner

* updating tests

* cleaning up tests

* adding changelog

* updating tests: adding cbs with unreachable error returns for safety

* Update modules/apps/27-interchain-accounts/controller/keeper/keeper.go

Co-authored-by: colin axnér <[email protected]>

Co-authored-by: colin axnér <[email protected]>
(cherry picked from commit dda9f98)

# Conflicts:
#	CHANGELOG.md
#	modules/apps/27-interchain-accounts/controller/ibc_middleware.go
  • Loading branch information
damiannolan authored and mergify[bot] committed Aug 31, 2022
1 parent 80b4e70 commit 034deee
Show file tree
Hide file tree
Showing 8 changed files with 176 additions and 14 deletions.
63 changes: 63 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,69 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

<<<<<<< HEAD
=======
### Improvements

* (27-interchain-accounts) [\#1352](https://github.com/cosmos/ibc-go/issues/1352) Add support for Cosmos-SDK simulation to ics27 module.
* (apps/27-interchain-accounts) [\#2133](https://github.com/cosmos/ibc-go/pull/2133) Generates genesis protos in a separate directory to avoid circular import errors. The protobuf package name has changed for the genesis types.
* (linting) [\#1418](https://github.com/cosmos/ibc-go/pull/1418) Fix linting errors, resulting compatiblity with go1.18 linting style, golangci-lint 1.46.2 and the revivie linter. This caused breaking changes in core/04-channel, core/ante, and the testing library.
* (modules/light-clients/07-tendermint) [\#1713](https://github.com/cosmos/ibc-go/pull/1713) Allow client upgrade proposals to update `TrustingPeriod`. See ADR-026 for context.
* (modules/core/02-client) [\#1188](https://github.com/cosmos/ibc-go/pull/1188/files) Routing `MsgSubmitMisbehaviour` to `UpdateClient` keeper function. Deprecating `SubmitMisbehaviour` endpoint.
* (modules/core/02-client) [\#1208](https://github.com/cosmos/ibc-go/pull/1208) Replace `CheckHeaderAndUpdateState` usage in 02-client with calls to `VerifyClientMessage`, `CheckForMisbehaviour`, `UpdateStateOnMisbehaviour` and `UpdateState`.
* (modules/light-clients/09-localhost) [\#1187](https://github.com/cosmos/ibc-go/pull/1187/) Removing localhost light client implementation as it is not functional. An upgrade handler is provided in `modules/migrations/v5` to prune `09-localhost` clients and consensus states from the store.
* [\#1186](https://github.com/cosmos/ibc-go/pull/1186/files) Removing `GetRoot` function from ConsensusState interface in `02-client`. `GetRoot` is unused by core IBC.
* (modules/core/02-client) [\#1196](https://github.com/cosmos/ibc-go/pull/1196) Adding VerifyClientMessage to ClientState interface.
* (modules/core/02-client) [\#1198](https://github.com/cosmos/ibc-go/pull/1198) Adding UpdateStateOnMisbehaviour to ClientState interface.
* (modules/core/02-client) [\#1170](https://github.com/cosmos/ibc-go/pull/1170) Updating `ClientUpdateProposal` to set client state in lightclient implementations `CheckSubstituteAndUpdateState` methods.
* (modules/core/02-client) [\#1197](https://github.com/cosmos/ibc-go/pull/1197) Adding `CheckForMisbehaviour` to `ClientState` interface.
* (modules/core/02-client) [\#1195](https://github.com/cosmos/ibc-go/pull/1210) Removing `CheckHeaderAndUpdateState` from `ClientState` interface & associated light client implementations.
* (modules/core/02-client) [\#1189](https://github.com/cosmos/ibc-go/pull/1212) Removing `CheckMisbehaviourAndUpdateState` from `ClientState` interface & associated light client implementations.
* (modules/core/exported) [\#1206](https://github.com/cosmos/ibc-go/pull/1206) Adding new method `UpdateState` to `ClientState` interface.
* (modules/core/02-client) [\#1741](https://github.com/cosmos/ibc-go/pull/1741) Emitting a new `upgrade_chain` event upon setting upgrade consensus state.
* (client) [\#724](https://github.com/cosmos/ibc-go/pull/724) `IsRevisionFormat` and `IsClientIDFormat` have been updated to disallow newlines before the dash used to separate the chainID and revision number, and the client type and client sequence.
* (02-client/cli) [\#897](https://github.com/cosmos/ibc-go/pull/897) Remove `GetClientID()` from `Misbehaviour` interface. Submit client misbehaviour cli command requires an explicit client id now.
* (06-solomachine) [\#1972](https://github.com/cosmos/ibc-go/pull/1972) Solo machine implementation of `ZeroCustomFields` fn now panics as the fn is only used for upgrades which solo machine does not support.
* (apps/27-interchain-accounts) [\#2102](https://github.com/cosmos/ibc-go/pull/2102) ICS27 controller middleware now supports a nil underlying application. This allows chains to make use of interchain accounts with existing auth mechanisms such as x/group and x/gov.
* (apps/27-interchain-accounts) [\#2146](https://github.com/cosmos/ibc-go/pull/2146) ICS27 controller now claims the channel capability passed via ibc core, and passes `nil` to the underlying app callback. The channel capability arg in `SendTx` is now ignored and looked up internally.
* (apps/27-interchain-accounts) [\#2134](https://github.com/cosmos/ibc-go/pull/2134) Adding upgrade handler to ICS27 `controller` submodule for migration of channel capabilities. This upgrade handler migrates ownership of channel capabilities from the underlying application to the ICS27 `controller` submodule.
* (apps/27-interchain-accounts) [\#2157](https://github.com/cosmos/ibc-go/pull/2157) Adding `IsMiddlewareEnabled` functionality to enforce calls to ICS27 msg server to *not* route to the underlying application.

### Features

### Bug Fixes

* (makefile) [\#1785](https://github.com/cosmos/ibc-go/pull/1785) Fetch the correct versions of protocol buffers dependencies from tendermint, cosmos-sdk, and ics23.
* (light-clients/solomachine) [#1839](https://github.com/cosmos/ibc-go/issues/1839) Fixed usage of the new diversifier in validation of changing diversifiers for the solo machine. The current diversifier must sign over the new diversifier.
* (light-clients/07-tendermint) [\#1674](https://github.com/cosmos/ibc-go/pull/1674) Submitted ClientState is zeroed out before checking the proof in order to prevent the proposal from containing information governance is not actually voting on.
* (modules/core/02-client)[\#1676](https://github.com/cosmos/ibc-go/pull/1676) ClientState must be zeroed out for `UpgradeProposals` to pass validation. This prevents a proposal containing information governance is not actually voting on.

## [v4.0.0](https://github.com/cosmos/ibc-go/releases/tag/v4.0.0) - 2022-08-12

### Dependencies

* [\#1627](https://github.com/cosmos/ibc-go/pull/1627) Bump Go version to 1.18
* [\#1905](https://github.com/cosmos/ibc-go/pull/1905) Bump SDK version to v0.45.7

### API Breaking

* (core/04-channel) [\#1792](https://github.com/cosmos/ibc-go/pull/1792) Remove `PreviousChannelID` from `NewMsgChannelOpenTry` arguments. `MsgChannelOpenTry.ValidateBasic()` returns error if the deprecated `PreviousChannelID` is not empty.
* (core/03-connection) [\#1797](https://github.com/cosmos/ibc-go/pull/1797) Remove `PreviousConnectionID` from `NewMsgConnectionOpenTry` arguments. `MsgConnectionOpenTry.ValidateBasic()` returns error if the deprecated `PreviousConnectionID` is not empty.
* (modules/core/03-connection) [\#1672](https://github.com/cosmos/ibc-go/pull/1672) Remove crossing hellos from connection handshakes. The `PreviousConnectionId` in `MsgConnectionOpenTry` has been deprecated.
* (modules/core/04-channel) [\#1317](https://github.com/cosmos/ibc-go/pull/1317) Remove crossing hellos from channel handshakes. The `PreviousChannelId` in `MsgChannelOpenTry` has been deprecated.
* (transfer) [\#1250](https://github.com/cosmos/ibc-go/pull/1250) Deprecate `GetTransferAccount` since the `transfer` module account is never used.
* (channel) [\#1283](https://github.com/cosmos/ibc-go/pull/1283) The `OnChanOpenInit` application callback now returns a version string in line with the latest [spec changes](https://github.com/cosmos/ibc/pull/629).
* (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`.
* (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`.
* (apps/27-interchain-accounts)[\#1432](https://github.com/cosmos/ibc-go/pull/1432) Updating `RegisterInterchainAccount` to include an additional `version` argument, supporting ICS29 fee middleware functionality in ICS27 interchain accounts.
* (apps/27-interchain-accounts)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`.
* (transfer)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`.
* (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of non-deterministic writes to application state.
* (core/04-channel)[\#1636](https://github.com/cosmos/ibc-go/pull/1636) Removing `SplitChannelVersion` and `MergeChannelVersions` functions since they are not used.

### State Machine Breaking

>>>>>>> dda9f98 (chore: ics27 middleware callback routing (#2157))
* (apps/transfer) [\#1907](https://github.com/cosmos/ibc-go/pull/1907) Blocked module account addresses are no longer allowed to send IBC transfers.
* (apps/27-interchain-accounts) [\#1882](https://github.com/cosmos/ibc-go/pull/1882) Explicitly check length of interchain account packet data in favour of nil check.

Expand Down
11 changes: 8 additions & 3 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,13 @@ func (im IBCMiddleware) OnChanOpenInit(
// call underlying app's OnChanOpenInit callback with the passed in version
// the version returned is discarded as the ica-auth module does not have permission to edit the version string.
// ics27 will always return the version string containing the Metadata struct which is created during the `RegisterInterchainAccount` call.
<<<<<<< HEAD
if im.app != nil {
if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
=======
if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, channelID) {
if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, nil, counterparty, version); err != nil {
>>>>>>> dda9f98 (chore: ics27 middleware callback routing (#2157))
return "", err
}
}
Expand Down Expand Up @@ -103,7 +108,7 @@ func (im IBCMiddleware) OnChanOpenAck(
}

// call underlying app's OnChanOpenAck callback with the counterparty app version.
if im.app != nil {
if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, channelID) {
return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
}

Expand Down Expand Up @@ -162,7 +167,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
}

// call underlying app's OnAcknowledgementPacket callback.
if im.app != nil {
if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
}

Expand All @@ -183,7 +188,7 @@ func (im IBCMiddleware) OnTimeoutPacket(
return err
}

if im.app != nil {
if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
var (
channel *channeltypes.Channel
isNilApp bool
path *ibctesting.Path
)

testCases := []struct {
Expand Down Expand Up @@ -171,6 +172,18 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
isNilApp = true
}, true,
},
{
"middleware disabled", func() {
suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty, version string,
) (string, error) {
return "", fmt.Errorf("error should be unreachable")
}
}, true,
},
}

for _, tc := range testCases {
Expand All @@ -180,7 +193,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
suite.SetupTest() // reset
isNilApp = false

path := NewICAPath(suite.chainA, suite.chainB)
path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

// mock init interchain account
Expand All @@ -193,6 +206,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
path.EndpointA.ChannelConfig.PortID = portID
path.EndpointA.ChannelID = ibctesting.FirstChannelID

suite.chainA.GetSimApp().ICAControllerKeeper.SetMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

// default values
counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
channel = &channeltypes.Channel{
Expand Down Expand Up @@ -322,6 +337,17 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
isNilApp = true
}, true,
},
{
"middleware disabled", func() {
suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenAck = func(
ctx sdk.Context, portID, channelID string, counterpartyChannelID string, counterpartyVersion string,
) error {
return fmt.Errorf("error should be unreachable")
}
}, true,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -558,6 +584,17 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
isNilApp = true
}, true,
},
{
"middleware disabled", func() {
suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnAcknowledgementPacket = func(
ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress,
) error {
return fmt.Errorf("error should be unreachable")
}
}, true,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -640,6 +677,17 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
isNilApp = true
}, true,
},
{
"middleware disabled", func() {
suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnTimeoutPacket = func(
ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress,
) error {
return fmt.Errorf("error should be unreachable")
}
}, true,
},
}

for _, tc := range testCases {
Expand Down
24 changes: 15 additions & 9 deletions modules/apps/27-interchain-accounts/controller/keeper/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,28 @@ import (
// - An error is returned if the port identifier is already in use. Gaining access to interchain accounts whose channels
// have closed cannot be done with this function. A regular MsgChannelOpenInit must be used.
func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner, version string) error {
_, err := k.registerInterchainAccount(ctx, connectionID, owner, version)
return err
}

// registerInterchainAccount registers an interchain account, returning the channel id of the MsgChannelOpenInitResponse
// and an error if one occurred.
func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, owner, version string) (string, error) {
portID, err := icatypes.NewControllerPortID(owner)
if err != nil {
return "", err
return err
}

channelID, err := k.registerInterchainAccount(ctx, connectionID, portID, version)
if err != nil {
return err
}

k.SetMiddlewareEnabled(ctx, portID, channelID)

return nil
}

// registerInterchainAccount registers an interchain account, returning the channel id of the MsgChannelOpenInitResponse
// and an error if one occurred.
func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, portID, version string) (string, error) {
// if there is an active channel for this portID / connectionID return an error
activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionID, portID)
if found {
return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s for owner %s", activeChannelID, portID, connectionID, owner)
return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s", activeChannelID, portID, connectionID)
}

switch {
Expand Down
18 changes: 18 additions & 0 deletions modules/apps/27-interchain-accounts/controller/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,21 @@ func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, connectionID, portI
store := ctx.KVStore(k.storeKey)
store.Set(icatypes.KeyOwnerAccount(portID, connectionID), []byte(address))
}

// IsMiddlewareEnabled returns true if the underlying application callbacks are enabled for given port and channel identifier pair, otherwise false
func (k Keeper) IsMiddlewareEnabled(ctx sdk.Context, portID, channelID string) bool {
store := ctx.KVStore(k.storeKey)
return store.Has(icatypes.KeyIsMiddlewareEnabled(portID, channelID))
}

// SetMiddlewareEnabled stores a flag to indicate that the underlying application callbacks should be enabled for the given port and channel identifier pair
func (k Keeper) SetMiddlewareEnabled(ctx sdk.Context, portID, channelID string) {
store := ctx.KVStore(k.storeKey)
store.Set(icatypes.KeyIsMiddlewareEnabled(portID, channelID), []byte{byte(1)})
}

// DeleteMiddlewareEnabled deletes the middleware enabled flag stored in state
func (k Keeper) DeleteMiddlewareEnabled(ctx sdk.Context, portID, channelID string) {
store := ctx.KVStore(k.storeKey)
store.Delete(icatypes.KeyIsMiddlewareEnabled(portID, channelID))
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types"
)

var _ types.MsgServer = Keeper{}
Expand All @@ -14,7 +15,12 @@ var _ types.MsgServer = Keeper{}
func (k Keeper) RegisterAccount(goCtx context.Context, msg *types.MsgRegisterAccount) (*types.MsgRegisterAccountResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

channelID, err := k.registerInterchainAccount(ctx, msg.ConnectionId, msg.Owner, msg.Version)
portID, err := icatypes.NewControllerPortID(msg.Owner)
if err != nil {
return nil, err
}

channelID, err := k.registerInterchainAccount(ctx, msg.ConnectionId, portID, msg.Version)
if err != nil {
return nil, err
}
Expand Down
8 changes: 8 additions & 0 deletions modules/apps/27-interchain-accounts/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ var (

// PortKeyPrefix defines the key prefix used to store ports
PortKeyPrefix = "port"

// IsMiddlewareEnabledPrefix defines the key prefix used to store a flag for legacy API callback routing via ibc middleware
IsMiddlewareEnabledPrefix = "isMiddlewareEnabled"
)

// KeyActiveChannel creates and returns a new key used for active channels store operations
Expand All @@ -52,3 +55,8 @@ func KeyOwnerAccount(portID, connectionID string) []byte {
func KeyPort(portID string) []byte {
return []byte(fmt.Sprintf("%s/%s", PortKeyPrefix, portID))
}

// KeyIsMiddlewareEnabled creates and returns a new key used for signaling legacy API callback routing via ibc middleware
func KeyIsMiddlewareEnabled(portID, channelID string) []byte {
return []byte(fmt.Sprintf("%s/%s/%s", IsMiddlewareEnabledPrefix, portID, channelID))
}
8 changes: 8 additions & 0 deletions modules/apps/27-interchain-accounts/types/keys_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
package types_test

import (
fmt "fmt"

"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types"
ibctesting "github.com/cosmos/ibc-go/v5/testing"
)

func (suite *TypesTestSuite) TestKeyActiveChannel() {
Expand All @@ -13,3 +16,8 @@ func (suite *TypesTestSuite) TestKeyOwnerAccount() {
key := types.KeyOwnerAccount("port-id", "connection-id")
suite.Require().Equal("owner/port-id/connection-id", string(key))
}

func (suite *TypesTestSuite) TestKeyIsMiddlewareEnabled() {
key := types.KeyIsMiddlewareEnabled(ibctesting.MockPort, ibctesting.FirstChannelID)
suite.Require().Equal(fmt.Sprintf("%s/%s/%s", types.IsMiddlewareEnabledPrefix, ibctesting.MockPort, ibctesting.FirstChannelID), string(key))
}

0 comments on commit 034deee

Please sign in to comment.