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

Add nil checks on the controller module for underlying app #2102

Merged
merged 16 commits into from
Aug 26, 2022
Merged

Add nil checks on the controller module for underlying app #2102

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0d0de7a
chore: adding nil checks when accessing underlying app
chatton Aug 23, 2022
8264c93
chore: updating tests to add nil auth scenario
chatton Aug 24, 2022
e8e94af
chore: adding test cases for nil module in all relevant cases
chatton Aug 24, 2022
20db6d4
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 24, 2022
248f449
chore: changed message to nil underlying app
chatton Aug 24, 2022
964df98
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 24, 2022
df3a616
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 24, 2022
a0f66f1
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 25, 2022
e519082
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 25, 2022
7db37aa
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 25, 2022
d4c5b04
chore: addressing formatting issues from PR feedback
chatton Aug 25, 2022
24753e6
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 25, 2022
58b7b67
chore: adding changelog.md entry
chatton Aug 25, 2022
e9dacf9
chore: updating changelog message
chatton Aug 25, 2022
4c3eb83
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 26, 2022
cc1d9cc
Merge branch 'main' into cian/issue#2040-add-nil-checks-on-the-contro…
chatton Aug 26, 2022
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
21 changes: 16 additions & 5 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 @@ -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,10 @@ 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
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// OnChanOpenAck implements the IBCMiddleware interface
Expand Down Expand Up @@ -156,7 +161,10 @@ 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 +181,10 @@ 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
44 changes: 44 additions & 0 deletions 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 @@ -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 @@ -121,6 +122,7 @@ func SetupICAPath(path *ibctesting.Path, owner string) error {

func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
var channel *channeltypes.Channel
var isNilApp bool
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

testCases := []struct {
name string
Expand Down Expand Up @@ -162,6 +164,11 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
}
}, false,
},
{
"nil underlying app", func() {
isNilApp = true
}, true,
},
}

for _, tc := range testCases {
Expand All @@ -170,6 +177,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset

isNilApp = false
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

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

Expand Down Expand Up @@ -207,6 +216,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 @@ -272,6 +285,7 @@ func (suite *InterchainAccountsTestSuite) TestChanOpenTry() {

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

testCases := []struct {
name string
Expand Down Expand Up @@ -300,6 +314,11 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
}
}, false,
},
{
"nil underlying app", func() {
isNilApp = true
}, true,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -327,6 +346,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 {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
}

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

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

testCases := []struct {
msg string
Expand All @@ -523,11 +547,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 +584,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 @@ -567,6 +601,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {

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

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

for _, tc := range testCases {
Expand Down Expand Up @@ -623,6 +663,10 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

if isNilApp {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
}

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

if tc.expPass {
Expand Down