Skip to content

Commit

Permalink
Add nil checks on the controller module for underlying app (#2102)
Browse files Browse the repository at this point in the history 
(cherry picked from commit dcd616c)

# Conflicts:
#	CHANGELOG.md
  • Loading branch information
@chatton @mergify
chatton authored and mergify[bot] committed Aug 26, 2022
1 parent cc7148a commit e7b49fd
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 9 deletions.
58 changes: 58 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,64 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

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

* (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.

### 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

>>>>>>> dcd616c (Add nil checks on the controller module for underlying app (#2102))
* (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
24 changes: 19 additions & 5 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,10 @@ 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.
if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
return "", err
if im.app != nil {
if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
return "", err
}
}

return version, nil
Expand Down Expand Up @@ -101,7 +103,11 @@ func (im IBCMiddleware) OnChanOpenAck(
}

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

return nil
}

// OnChanOpenAck implements the IBCMiddleware interface
Expand Down Expand Up @@ -156,7 +162,11 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
}

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

return nil
}

// OnTimeoutPacket implements the IBCMiddleware interface
Expand All @@ -173,7 +183,11 @@ func (im IBCMiddleware) OnTimeoutPacket(
return err
}

return im.app.OnTimeoutPacket(ctx, packet, relayer)
if im.app != nil {
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

return nil
}

// SendPacket implements the ICS4 Wrapper interface
Expand Down
61 changes: 57 additions & 4 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/stretchr/testify/suite"
"github.com/tendermint/tendermint/crypto"

"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller"
"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"
fee "github.com/cosmos/ibc-go/v5/modules/apps/29-fee"
Expand Down Expand Up @@ -120,7 +121,10 @@ func SetupICAPath(path *ibctesting.Path, owner string) error {
}

func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
var channel *channeltypes.Channel
var (
channel *channeltypes.Channel
isNilApp bool
)

testCases := []struct {
name string
Expand Down Expand Up @@ -162,13 +166,19 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
}
}, false,
},
{
"nil underlying app", func() {
isNilApp = true
}, 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)
Expand Down Expand Up @@ -207,6 +217,10 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
}

version, err := cbs.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(),
)
Expand Down Expand Up @@ -271,7 +285,10 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenTry() {
}

func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
var path *ibctesting.Path
var (
path *ibctesting.Path
isNilApp bool
)

testCases := []struct {
name string
Expand Down Expand Up @@ -300,13 +317,19 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
}
}, false,
},
{
"nil underlying app", func() {
isNilApp = true
}, 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)
Expand All @@ -327,6 +350,10 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {

err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelID, path.EndpointB.ChannelConfig.Version)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
}

if tc.expPass {
suite.Require().NoError(err)
} else {
Expand Down Expand Up @@ -497,7 +524,10 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
}

func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
var path *ibctesting.Path
var (
path *ibctesting.Path
isNilApp bool
)

testCases := []struct {
msg string
Expand All @@ -523,11 +553,17 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
}
}, false,
},
{
"nil underlying app", func() {
isNilApp = true
}, true,
},
}

for _, tc := range testCases {
suite.Run(tc.msg, func() {
suite.SetupTest() // reset
isNilApp = false

path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)
Expand All @@ -554,6 +590,10 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
}

err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, []byte("ack"), nil)

if tc.expPass {
Expand All @@ -566,7 +606,10 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
}

func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
var path *ibctesting.Path
var (
path *ibctesting.Path
isNilApp bool
)

testCases := []struct {
msg string
Expand All @@ -592,11 +635,17 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
}
}, false,
},
{
"nil underlying app", func() {
isNilApp = true
}, true,
},
}

for _, tc := range testCases {
suite.Run(tc.msg, func() {
suite.SetupTest() // reset
isNilApp = false

path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)
Expand All @@ -623,6 +672,10 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

if isNilApp {
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
}

err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, nil)

if tc.expPass {
Expand Down

0 comments on commit e7b49fd

Please sign in to comment.