chore: remove OnChanUpgradeRestore callbacks and discard state changes on app upgrade callbacks

docs/docs/01-ibc/06-channel-upgrades.md

@@ -152,8 +152,6 @@ It is possible for a relayer to cancel an in-progress channel upgrade if the fol
 Upon cancelling a channel upgrade, an `ErrorReceipt` will be written with the channel's current upgrade sequence, and
 the channel will move back to `OPEN` state keeping its original parameters.
 
-The application's `OnChanUpgradeRestore` callback method will be invoked.
-
 It will then be possible to re-initiate an upgrade by sending a `MsgChannelOpenInit` message.
 
 ## Timing Out a Channel Upgrade
@@ -204,8 +202,6 @@ type MsgChannelUpgradeTimeout struct {
 
 An `ErrorReceipt` will be written with the channel's current upgrade sequence, and the channel will move back to `OPEN` state keeping its original parameters.
 
-The application's `OnChanUpgradeRestore` callback method will also be invoked.
-
 Note that timing out a channel upgrade will end the upgrade process, and a new `MsgChannelUpgradeInit` will have to be submitted via governance in order to restart the upgrade process.
 
 ## Pruning Acknowledgements
@@ -252,8 +248,6 @@ should be aware that callbacks will be invoked before any core ibc state changes
 
 `OnChanUpgradeOpen` should perform any logic associated with changing of the channel fields.
 
-`OnChanUpgradeRestore`  should perform any logic that needs to be executed when an upgrade attempt fails as is reverted.
-
 > IBC applications should not attempt to process any packet data under the new conditions until after `OnChanUpgradeOpen`
 > has been executed, as up until this point it is still possible for the upgrade handshake to fail and for the channel
 > to remain in the pre-upgraded state. 

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -309,23 +309,6 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str
 	}
 }
 
-// OnChanUpgradeRestore implements the IBCModule interface
-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(
 	ctx sdk.Context,

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go

@@ -1066,67 +1066,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() {
 	}
 }
 
-func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeRestore() {
-	var (
-		path     *ibctesting.Path
-		isNilApp bool
-	)
-
-	testCases := []struct {
-		name     string
-		malleate func()
-	}{
-		{
-			"success", func() {},
-		},
-		{
-			"success: nil app",
-			func() {
-				isNilApp = true
-			},
-		},
-		{
-			"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
-				}
-			},
-		},
-	}
-
-	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)
-		})
-	}
-}
-
 func (suite *InterchainAccountsTestSuite) TestSingleHostMultipleControllers() {
 	var (
 		pathAToB *ibctesting.Path

modules/apps/27-interchain-accounts/host/ibc_module.go

@@ -180,9 +180,6 @@ func (IBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpar
 func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, proposedVersion string) {
 }
 
-// OnChanUpgradeRestore implements the IBCModule interface
-func (IBCModule) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {}
-
 // UnmarshalPacketData attempts to unmarshal the provided packet data bytes
 // into an InterchainAccountPacketData. This function implements the optional
 // PacketDataUnmarshaler interface required for ADR 008 support.

modules/apps/29-fee/ibc_middleware.go

@@ -440,16 +440,6 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str
 	cbs.OnChanUpgradeOpen(ctx, portID, channelID, proposedOrder, proposedConnectionHops, versionMetadata.AppVersion)
 }
 
-// OnChanUpgradeRestore implements the IBCModule interface
-func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {
-	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 (im IBCMiddleware) SendPacket(
 	ctx sdk.Context,

modules/apps/callbacks/ibc_middleware.go

@@ -404,16 +404,6 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str
 	cbs.OnChanUpgradeOpen(ctx, portID, channelID, proposedOrder, proposedConnectionHops, proposedVersion)
 }
 
-// OnChanUpgradeRestore implements the IBCModule interface
-func (im IBCMiddleware) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {
-	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)
-}
-
 // GetAppVersion implements the ICS4Wrapper interface. Callbacks has no version,
 // so the call is deferred to the underlying application.
 func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {

modules/apps/transfer/ibc_module.go

@@ -347,9 +347,6 @@ func (IBCModule) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpar
 func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, proposedVersion string) {
 }
 
-// OnChanUpgradeRestore implements the IBCModule interface
-func (IBCModule) OnChanUpgradeRestore(ctx sdk.Context, portID, channelID string) {}
-
 // UnmarshalPacketData attempts to unmarshal the provided packet data bytes
 // into a FungibleTokenPacketData. This function implements the optional
 // PacketDataUnmarshaler interface required for ADR 008 support.

modules/core/05-port/types/module.go

@@ -112,7 +112,8 @@ type IBCModule interface {
 type UpgradableModule interface {
 	// OnChanUpgradeInit enables additional custom logic to be executed when the channel upgrade is initialized.
 	// It must validate the proposed version, order, and connection hops.
-	// Note: in the case of crossing hellos, this callback may be executed on both chains.
+	// NOTE: in the case of crossing hellos, this callback may be executed on both chains.
+	// NOTE: Any IBC application state changes made in this callback handler are not committed.
 	OnChanUpgradeInit(
 		ctx sdk.Context,
 		portID, channelID string,
@@ -124,6 +125,7 @@ type UpgradableModule interface {
 	// OnChanUpgradeTry enables additional custom logic to be executed in the ChannelUpgradeTry step of the
 	// channel upgrade handshake. It must validate the proposed version (provided by the counterparty), order,
 	// and connection hops.
+	// NOTE: Any IBC application state changes made in this callback handler are not committed.
 	OnChanUpgradeTry(
 		ctx sdk.Context,
 		portID, channelID string,
@@ -134,6 +136,7 @@ type UpgradableModule interface {
 
 	// OnChanUpgradeAck enables additional custom logic to be executed in the ChannelUpgradeAck step of the
 	// channel upgrade handshake. It must validate the version proposed by the counterparty.
+	// NOTE: Any IBC application state changes made in this callback handler are not committed.
 	OnChanUpgradeAck(
 		ctx sdk.Context,
 		portID,
@@ -152,17 +155,6 @@ type UpgradableModule interface {
 		proposedConnectionHops []string,
 		proposedVersion string,
 	)
-
-	// OnChanUpgradeRestore enables additional custom logic to be executed when any of the following occur:
-	//    - the channel upgrade is aborted.
-	//    - the channel upgrade is cancelled.
-	//    - the channel upgrade times out.
-	// Any logic set due to an upgrade attempt should be reverted in this callback.
-	OnChanUpgradeRestore(
-		ctx sdk.Context,
-		portID,
-		channelID string,
-	)
 }
 
 // ICS4Wrapper implements the ICS4 interfaces that IBC applications use to send packets and acknowledgements.

modules/core/keeper/msg_server.go

@@ -769,8 +769,9 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC
 		return nil, errorsmod.Wrap(err, "channel upgrade init failed")
 	}
 
+	cacheCtx, _ := ctx.CacheContext() // NOTE: the writeFn is discarded and application state changes are not committed.
 	upgradeVersion, err := cbs.OnChanUpgradeInit(
-		ctx,
+		cacheCtx,
 		msg.PortId,
 		msg.ChannelId,
 		upgrade.Fields.Ordering,
@@ -829,7 +830,8 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh
 		return nil, errorsmod.Wrap(err, "channel upgrade try failed")
 	}
 
-	upgradeVersion, err := cbs.OnChanUpgradeTry(ctx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version)
+	cacheCtx, _ := ctx.CacheContext() // NOTE: the writeFn is discarded and application state changes are not committed.
+	upgradeVersion, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version)
 	if err != nil {
 		ctx.Logger().Error("channel upgrade try callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error())
 		return nil, errorsmod.Wrapf(err, "channel upgrade try callback failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId)
@@ -875,7 +877,6 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
 	if err != nil {
 		ctx.Logger().Error("channel upgrade ack failed", "error", errorsmod.Wrap(err, "channel upgrade ack failed"))
 		if channeltypes.IsUpgradeError(err) {
-			cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId)
 			k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, err)
 
 			// NOTE: a FAILURE result is returned to the client and an error receipt is written to state.
@@ -887,18 +888,15 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
 		return nil, errorsmod.Wrap(err, "channel upgrade ack failed")
 	}
 
-	cacheCtx, writeFn := ctx.CacheContext()
+	cacheCtx, _ := ctx.CacheContext() // NOTE: the writeFn is discarded and application state changes are not committed.
 	err = cbs.OnChanUpgradeAck(cacheCtx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade.Fields.Version)
 	if err != nil {
 		ctx.Logger().Error("channel upgrade ack callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error())
-		cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId)
 		k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, err)
 
 		return &channeltypes.MsgChannelUpgradeAckResponse{Result: channeltypes.FAILURE}, nil
 	}
 
-	writeFn()
-
 	channel, upgrade := k.ChannelKeeper.WriteUpgradeAckChannel(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade)
 
 	ctx.Logger().Info("channel upgrade ack succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)
@@ -935,7 +933,6 @@ func (k Keeper) ChannelUpgradeConfirm(goCtx context.Context, msg *channeltypes.M
 	if err != nil {
 		ctx.Logger().Error("channel upgrade confirm failed", "error", errorsmod.Wrap(err, "channel upgrade confirm failed"))
 		if channeltypes.IsUpgradeError(err) {
-			cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId)
 			k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, err)
 
 			// NOTE: a FAILURE result is returned to the client and an error receipt is written to state.
@@ -1015,32 +1012,11 @@ func (k Keeper) ChannelUpgradeOpen(goCtx context.Context, msg *channeltypes.MsgC
 // ChannelUpgradeTimeout defines a rpc handler method for MsgChannelUpgradeTimeout.
 func (k Keeper) ChannelUpgradeTimeout(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeTimeout) (*channeltypes.MsgChannelUpgradeTimeoutResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
-	module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId)
-	if err != nil {
-		ctx.Logger().Error("channel upgrade timeout failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id"))
-		return nil, errorsmod.Wrap(err, "could not retrieve module from port-id")
-	}
-
-	app, ok := k.Router.GetRoute(module)
-	if !ok {
-		err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)
-		ctx.Logger().Error("channel upgrade timeout failed", "port-id", msg.PortId, "error", err)
-		return nil, err
-	}
-
-	cbs, ok := app.(porttypes.UpgradableModule)
-	if !ok {
-		err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "upgrade route not found to module: %s", module)
-		ctx.Logger().Error("channel upgrade timeout failed", "port-id", msg.PortId, "error", err)
-		return nil, err
-	}
 
-	err = k.ChannelKeeper.ChanUpgradeTimeout(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyChannel, msg.ProofChannel, msg.ProofHeight)
-	if err != nil {
+	if err := k.ChannelKeeper.ChanUpgradeTimeout(ctx, msg.PortId, msg.ChannelId, msg.CounterpartyChannel, msg.ProofChannel, msg.ProofHeight); err != nil {
 		return nil, errorsmod.Wrapf(err, "could not timeout upgrade for channel: %s", msg.ChannelId)
 	}
 
-	cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId)
 	channel, upgrade := k.ChannelKeeper.WriteUpgradeTimeoutChannel(ctx, msg.PortId, msg.ChannelId)
 
 	ctx.Logger().Info("channel upgrade timeout callback succeeded: portID %s, channelID %s", msg.PortId, msg.ChannelId)
@@ -1052,25 +1028,6 @@ func (k Keeper) ChannelUpgradeTimeout(goCtx context.Context, msg *channeltypes.M
 // ChannelUpgradeCancel defines a rpc handler method for MsgChannelUpgradeCancel.
 func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeCancel) (*channeltypes.MsgChannelUpgradeCancelResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
-	module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId)
-	if err != nil {
-		ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id"))
-		return nil, errorsmod.Wrap(err, "could not retrieve module from port-id")
-	}
-
-	app, ok := k.Router.GetRoute(module)
-	if !ok {
-		err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)
-		ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err)
-		return nil, err
-	}
-
-	cbs, ok := app.(porttypes.UpgradableModule)
-	if !ok {
-		err = errorsmod.Wrapf(porttypes.ErrInvalidRoute, "upgrade route not found to module: %s", module)
-		ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, err)
-		return nil, err
-	}
 
 	channel, found := k.ChannelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId)
 	if !found {
@@ -1086,7 +1043,6 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms
 			return nil, errorsmod.Wrapf(channeltypes.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", msg.PortId, msg.ChannelId)
 		}
 
-		cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId)
 		k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId, channel.UpgradeSequence)
 
 		ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)
@@ -1107,7 +1063,6 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms
 		return nil, errorsmod.Wrapf(channeltypes.ErrUpgradeNotFound, "failed to retrieve channel upgrade: port ID (%s) channel ID (%s)", msg.PortId, msg.ChannelId)
 	}
 
-	cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId)
 	k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt.Sequence)
 
 	ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)