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.

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

Use new port router for OnChanUpgradeAck #7094

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -287,25 +287,7 @@ func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, cou
return types.ErrControllerSubModuleDisabled
}

if err := im.keeper.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion); err != nil {
return err
}

connectionID, err := im.keeper.GetConnectionID(ctx, portID, channelID)
if err != nil {
return 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 {
return errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")
}
return cbs.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion)
}

return nil
return im.keeper.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion)
}

// OnChanUpgradeOpen implements the IBCModule interface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() {
func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {
var (
path *ibctesting.Path
isNilApp bool
counterpartyVersion string
)

Expand All @@ -842,9 +841,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {
},
{
"success: nil underlying app",
func() {
isNilApp = true
},
func() {},
nil,
},
{
Expand All @@ -864,14 +861,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {
}
}, ibcmock.MockApplicationCallbackError,
},
{
"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 _, ordering := range []channeltypes.Order{channeltypes.UNORDERED, channeltypes.ORDERED} {
Expand All @@ -880,7 +869,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {

suite.Run(tc.name, func() {
suite.SetupTest() // reset
isNilApp = false

path = NewICAPath(suite.chainA, suite.chainB, ordering)
path.SetupConnections()
Expand All @@ -892,18 +880,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {

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().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(path.EndpointA.ChannelConfig.PortID)
suite.Require().True(ok)

cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)

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

err = cbs.OnChanUpgradeAck(
suite.chainA.GetContext(),
path.EndpointA.ChannelConfig.PortID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -748,10 +748,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {
suite.Require().NoError(err)

// call application callback directly
module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down
22 changes: 4 additions & 18 deletions modules/apps/29-fee/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,25 +316,11 @@ func (IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string,
}

// OnChanUpgradeAck implements the IBCModule interface
func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
cbs, ok := im.app.(porttypes.UpgradableModule)
if !ok {
return errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")
}

versionMetadata, err := types.MetadataFromVersion(counterpartyVersion)
if err != nil {
// since it is valid for fee version to not be specified, the counterparty upgrade version may be for a middleware
// or application further down in the stack. Thus, pass through to next middleware or application in callstack.
return cbs.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion)
}

if versionMetadata.FeeVersion != types.Version {
return errorsmod.Wrapf(types.ErrInvalidVersion, "expected counterparty fee version: %s, got: %s", types.Version, versionMetadata.FeeVersion)
func (IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
if counterpartyVersion != types.Version {
return errorsmod.Wrapf(types.ErrInvalidVersion, "expected counterparty fee version: %s, got: %s", types.Version, counterpartyVersion)
}

// call underlying app's OnChanUpgradeAck callback with the counterparty app version.
return cbs.OnChanUpgradeAck(ctx, portID, channelID, versionMetadata.AppVersion)
return nil
}

// OnChanUpgradeOpen implements the IBCModule interface
Expand Down
5 changes: 1 addition & 4 deletions modules/apps/29-fee/ibc_middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1283,10 +1283,7 @@ func (suite *FeeTestSuite) TestOnChanUpgradeAck() {

counterpartyUpgrade := path.EndpointB.GetChannelUpgrade()

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)
suite.Require().NoError(err)

app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(ibctesting.MockFeePort)
suite.Require().True(ok)

cbs, ok := app.(porttypes.UpgradableModule)
Expand Down
9 changes: 2 additions & 7 deletions modules/apps/callbacks/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,13 +368,8 @@ func (IBCMiddleware) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string,
}

// OnChanUpgradeAck implements the IBCModule interface
func (im IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
cbs, ok := im.app.(porttypes.UpgradableModule)
if !ok {
return errorsmod.Wrap(porttypes.ErrInvalidRoute, "upgrade route not found to module in application callstack")
}

return cbs.OnChanUpgradeAck(ctx, portID, channelID, counterpartyVersion)
func (IBCMiddleware) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
return nil
}

// OnChanUpgradeOpen implements the IBCModule interface
Expand Down
5 changes: 1 addition & 4 deletions modules/apps/transfer/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -690,10 +690,7 @@ func (suite *TransferTestSuite) TestOnChanUpgradeAck() {

tc.malleate()

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID)
suite.Require().NoError(err)

app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(types.PortID)
suite.Require().True(ok)

cbs, ok := app.(porttypes.UpgradableModule)
Expand Down
34 changes: 33 additions & 1 deletion modules/core/05-port/types/ibc_legacy_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,39 @@ func (im *LegacyIBCModule) OnChanUpgradeTry(ctx sdk.Context, portID, channelID s
}

// OnChanUpgradeAck implements the IBCModule interface
func (LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
func (im *LegacyIBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as OnChanUpgradeTry except the negotiatedVersions array was removed.

for i := len(im.cbs) - 1; i >= 0; i-- {
cbVersion := counterpartyVersion

// To maintain backwards compatibility, we must handle two cases:
// - relayer provides empty version (use default versions)
// - relayer provides version which chooses to not enable a middleware
//
// If an application is a VersionWrapper which means it modifies the version string
// and the version string is non-empty (don't use default), then the application must
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion)
if err != nil {
// middleware disabled
continue
}
cbVersion, counterpartyVersion = appVersion, underlyingAppVersion
}

// in order to maintain backwards compatibility, every callback in the stack must implement the UpgradableModule interface.
upgradableModule, ok := im.cbs[i].(UpgradableModule)
if !ok {
return errorsmod.Wrap(ErrInvalidRoute, "upgrade route not found to module in application callstack")
}

err := upgradableModule.OnChanUpgradeAck(ctx, portID, channelID, cbVersion)
if err != nil {
return errorsmod.Wrapf(err, "channel open init callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
}
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ func (k *Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgC
func (k *Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeAck) (*channeltypes.MsgChannelUpgradeAckResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

app, ok := k.PortKeeper.Route(msg.PortId)
app, ok := k.PortKeeper.AppRouter.HandshakeRoute(msg.PortId)
if !ok {
err := errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", msg.PortId)
ctx.Logger().Error("channel upgrade ack failed", "port-id", msg.PortId, "error", err)
Expand Down
64 changes: 0 additions & 64 deletions modules/core/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1285,70 +1285,6 @@ func (suite *KeeperTestSuite) TestChannelUpgradeAck() {
ibctesting.AssertEvents(&suite.Suite, expEvents, events)
},
},
{
"application callback returns error and error receipt is written",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no more underlying app

func() {
suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeAck = func(
ctx sdk.Context, portID, channelID, counterpartyVersion string,
) error {
// set arbitrary value in store to mock application state changes
store := ctx.KVStore(suite.chainA.GetSimApp().GetKey(exported.ModuleName))
store.Set([]byte("foo"), []byte("bar"))
return fmt.Errorf("mock app callback failed")
}
},
func(res *channeltypes.MsgChannelUpgradeAckResponse, events []abci.Event, err error) {
suite.Require().NoError(err)

suite.Require().NotNil(res)
suite.Require().Equal(channeltypes.FAILURE, res.Result)

errorReceipt, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found)
suite.Require().Equal(uint64(1), errorReceipt.Sequence)

// assert application state changes are not committed
store := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetKey(exported.ModuleName))
suite.Require().False(store.Has([]byte("foo")))

channel := path.EndpointB.GetChannel()
expEvents := sdk.Events{
sdk.NewEvent(
channeltypes.EventTypeChannelUpgradeError,
sdk.NewAttribute(channeltypes.AttributeKeyPortID, path.EndpointA.ChannelConfig.PortID),
sdk.NewAttribute(channeltypes.AttributeKeyChannelID, path.EndpointA.ChannelID),
sdk.NewAttribute(channeltypes.AttributeCounterpartyPortID, path.EndpointB.ChannelConfig.PortID),
sdk.NewAttribute(channeltypes.AttributeCounterpartyChannelID, path.EndpointB.ChannelID),
sdk.NewAttribute(channeltypes.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", channel.UpgradeSequence)),
// need to manually insert this because the errorReceipt is a string constant as it is written into state
sdk.NewAttribute(channeltypes.AttributeKeyErrorReceipt, "mock app callback failed"),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, channeltypes.AttributeValueCategory),
),
}.ToABCIEvents()
ibctesting.AssertEvents(&suite.Suite, expEvents, events)
},
},
{
"ibc application does not implement the UpgradeableModule interface",
func() {
path = ibctesting.NewPath(suite.chainA, suite.chainB)
path.EndpointA.ChannelConfig.PortID = ibcmock.MockBlockUpgrade
path.EndpointB.ChannelConfig.PortID = ibcmock.MockBlockUpgrade

path.Setup()

msg.PortId = path.EndpointA.ChannelConfig.PortID
msg.ChannelId = path.EndpointA.ChannelID
},
func(res *channeltypes.MsgChannelUpgradeAckResponse, events []abci.Event, err error) {
suite.Require().ErrorIs(err, porttypes.ErrInvalidRoute)
suite.Require().Nil(res)
suite.Require().Empty(events)
},
},
{
"application callback returns an upgrade error",
func() {
Expand Down
Loading