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

Jump to bottom

Add implementations for OnChanUpgradeOpen and OnChanUpgradeRestore for controller. #5467

Merged
merged 4 commits into from
Dec 20, 2023
Merged

Add implementations for OnChanUpgradeOpen and OnChanUpgradeRestore for controller. #5467

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8e03df
Add passthrough implementations for OnChanUpgradeOpen and OnChanUpgra…
DimitrisJim Dec 19, 2023
05cad88
Merge branch '04-channel-upgrades' into jim/5457-impl-missing-callbac…
DimitrisJim Dec 20, 2023
a2c32ed
Drop unused expError from test functions.
DimitrisJim Dec 20, 2023
fc29a10
Merge branch '04-channel-upgrades' into jim/5457-impl-missing-callbac…
DimitrisJim Dec 20, 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
31 changes: 29 additions & 2 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 @@ -293,11 +293,38 @@ func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, cou
}

// OnChanUpgradeOpen implements the IBCModule interface
func (IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) {
func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) {
connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID)
if err != nil {
panic(err)
}

if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionID) {
// Only cast to UpgradableModule if the application is set.
cbs, ok := im.app.(porttypes.UpgradableModule)
if !ok {
panic(errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack"))
}
cbs.OnChanUpgradeOpen(ctx, portID, channelID, order, connectionHops, version)
}
}

// OnChanUpgradeRestore implements the IBCModule interface
func (IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {}
func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {
connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID)
if err != nil {
panic(err)
}

if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, connectionID) {
// Only cast to UpgradableModule if the application is set.
cbs, ok := im.app.(porttypes.UpgradableModule)
if !ok {
panic(errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack"))
}
cbs.OnChanUpgradeRestore(ctx, portID, channelID)
}
}

// SendPacket implements the ICS4 Wrapper interface
func (IBCMiddleware) SendPacket(
Expand Down
148 changes: 148 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 @@ -957,6 +957,154 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {
}
}

func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() {
var (
path *ibctesting.Path
isNilApp bool
counterpartyVersion string
)

testCases := []struct {
name string
malleate func()
expError error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose there will never an expError because the func can only panic in one case.

It should be possible to set some mock app or middleware as the app which doesn't implement the interface. Maybe could reuse something from #5406 with expPanics, where instead of using nil we use a mock that doesn't implement the interface:

if expPanics {
    cbs = controller.NewIBCMiddleware(ibcmock.NewBlockUpgradeMiddleware(), suite.chainA.GetSimApp().ICAControllerKeeper)
}

Happy to do it in a follow up if we go with the mock upgrade middleware

Copy link
Contributor Author

@DimitrisJim DimitrisJim Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to do it in a follow up if we go with the mock upgrade middleware

Sounds perfect to me, I'll open a quick issue for it. (I'll clean up expError for now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5471

}{
{
"success", func() {}, nil,
},
{
"success: nil app",
func() {
isNilApp = true
},
nil,
},
{
"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 ibcmock.MockApplicationCallbackError
}
}, nil,
},
}

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)
}

cbs.OnChanUpgradeOpen(
suite.chainA.GetContext(),
path.EndpointA.ChannelConfig.PortID,
path.EndpointA.ChannelID,
channeltypes.ORDERED,
[]string{path.EndpointA.ConnectionID},
counterpartyVersion,
)

if tc.expError == nil {
suite.Require().NoError(err)
} else {
suite.Require().ErrorIs(err, tc.expError)
}
})
}
}

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

testCases := []struct {
name string
malleate func()
expError error
}{
{
"success", func() {}, nil,
},
{
"success: nil app",
func() {
isNilApp = true
},
nil,
},
{
"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 ibcmock.MockApplicationCallbackError
}
}, nil,
},
}

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)

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)
}

cbs.OnChanUpgradeRestore(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

if tc.expError == nil {
suite.Require().NoError(err)
} else {
suite.Require().ErrorIs(err, tc.expError)
}
})
}
}

func (suite *InterchainAccountsTestSuite) TestSingleHostMultipleControllers() {
var (
pathAToB *ibctesting.Path
Expand Down
Loading