Skip to content

Commit

Permalink
chore: remove OnChanUpgradeRestore callbacks and discard state chan…
Browse files Browse the repository at this point in the history 
…ges on app upgrade callbacks (cosmos#5696)

* docs: remove refs to OnChanUpgradeRestore in docs

* testing: rm OnChanUpgradeRestore callback from mock app test helpers

* chore: rm OnChanUpgradeRestore application callback from ibc core and apps

* chore: discard app state changes by using a cacheCtx and discarding writeFn

* docs: add godoc notes to app callbacks discarding state changes

* test: adding unit tests for discarding app state changes

* chore: clean imports

* chore: review suggestions for in-line docstring comments

* chore: fix linting tests, add mock types for testing kv and events

* doc: add more context to in-line docstrings
  • Loading branch information
@damiannolan
damiannolan authored Jan 24, 2024
1 parent 0d897cd commit 9faaff5
Show file tree
Hide file tree
Showing 14 changed files with 113 additions and 268 deletions.
6 changes: 0 additions & 6 deletions docs/docs/01-ibc/06-channel-upgrades.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,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
Expand Down Expand Up @@ -208,8 +206,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
Expand Down Expand Up @@ -256,8 +252,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.
Expand Down
17 changes: 0 additions & 17 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
61 changes: 0 additions & 61 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 @@ -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
Expand Down
3 changes: 0 additions & 3 deletions modules/apps/27-interchain-accounts/host/ibc_module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 0 additions & 10 deletions modules/apps/29-fee/ibc_middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 0 additions & 10 deletions modules/apps/callbacks/ibc_middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 0 additions & 3 deletions modules/apps/transfer/ibc_module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
16 changes: 4 additions & 12 deletions modules/core/05-port/types/module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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.
Expand Down
70 changes: 12 additions & 58 deletions modules/core/keeper/msg_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,14 +769,10 @@ func (k Keeper) ChannelUpgradeInit(goCtx context.Context, msg *channeltypes.MsgC
return nil, errorsmod.Wrap(err, "channel upgrade init failed")
}

upgradeVersion, err := cbs.OnChanUpgradeInit(
ctx,
msg.PortId,
msg.ChannelId,
upgrade.Fields.Ordering,
upgrade.Fields.ConnectionHops,
upgrade.Fields.Version,
)
// NOTE: a cached context is used to discard ibc application state changes and events.
// IBC applications must flush in-flight packets using the pre-upgrade channel parameters.
cacheCtx, _ := ctx.CacheContext()
upgradeVersion, err := cbs.OnChanUpgradeInit(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version)
if err != nil {
ctx.Logger().Error("channel upgrade init callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error())
return nil, errorsmod.Wrapf(err, "channel upgrade init callback failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId)
Expand Down Expand Up @@ -829,7 +825,10 @@ 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)
// NOTE: a cached context is used to discard ibc application state changes and events.
// IBC applications must flush in-flight packets using the pre-upgrade channel parameters.
cacheCtx, _ := ctx.CacheContext()
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)
Expand Down Expand Up @@ -875,7 +874,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.
Expand All @@ -887,7 +885,9 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
return nil, errorsmod.Wrap(err, "channel upgrade ack failed")
}

cacheCtx, writeFn := ctx.CacheContext()
// NOTE: a cached context is used to discard ibc application state changes and events.
// IBC applications must flush in-flight packets using the pre-upgrade channel parameters.
cacheCtx, _ := ctx.CacheContext()
err = cbs.OnChanUpgradeAck(cacheCtx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade.Fields.Version)
if err != nil {
channel, found := k.ChannelKeeper.GetChannel(ctx, msg.PortId, msg.ChannelId)
Expand All @@ -896,7 +896,6 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
}

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)

// explicitly wrap the application callback in an upgrade error with the correct upgrade sequence.
// this prevents any errors caused from the application returning an UpgradeError with an incorrect sequence.
Expand All @@ -905,8 +904,6 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
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)
Expand Down Expand Up @@ -943,7 +940,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.
Expand Down Expand Up @@ -1023,32 +1019,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)
Expand All @@ -1060,25 +1035,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 {
Expand All @@ -1094,7 +1050,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)
Expand All @@ -1115,7 +1070,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)
Expand Down
Loading

0 comments on commit 9faaff5

Please sign in to comment.