reorganize channel handshake handler (cosmos#647)

* reorganize channel handshake handler

split out channel state changes into its own function.

* readjust 27-interchain-accounts to not rely on state being set before the application callback

* add changelog and migration doc entry

* Update modules/core/04-channel/keeper/handshake.go

CHANGELOG.md

@@ -54,6 +54,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [\#383](https://github.com/cosmos/ibc-go/pull/383) Adds helper functions for merging and splitting middleware versions from the underlying app version.
 * (modules/core/05-port) [\#288](https://github.com/cosmos/ibc-go/issues/288) Making the 05-port keeper function IsBound public. The IsBound function checks if the provided portID is already binded to a module.
 * (channel) [\#644](https://github.com/cosmos/ibc-go/pull/644) Adds `GetChannelConnection` to the ChannelKeeper. This function returns the connectionID and connection state associated with a channel. 
+* (channel) [\647](https://github.com/cosmos/ibc-go/pull/647) Reorganizes channel handshake handling to set channel state after IBC application callbacks. 
 
 ### Features
 

docs/migrations/v2-to-v3.md

@@ -22,6 +22,14 @@ ICS27 Interchain Accounts has been added as a supported IBC application of ibc-g
 
 ## IBC Apps
 
+### Channel state will not be set before application callback
+
+The channel handshake logic has been reorganized within core IBC. 
+Channel state will not be set in state after the application callback is performed.
+Applications must rely only on the passed in channel parameters instead of querying the channel keeper for channel state.
+
+### IBC application callbacks moved from `AppModule` to `IBCModule`
+
 Previously, IBC module callbacks were apart of the `AppModule` type. 
 The recommended approach is to create an `IBCModule` type and move the IBC module callbacks from `AppModule` to `IBCModule` in a separate file `ibc_module.go`. 
 

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

@@ -42,7 +42,7 @@ func (k Keeper) OnChanOpenInit(
 		return sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, portID)
 	}
 
-	if err := k.validateControllerPortParams(ctx, channelID, portID, connSequence, counterpartyConnSequence); err != nil {
+	if err := k.validateControllerPortParams(ctx, connectionHops, connSequence, counterpartyConnSequence); err != nil {
 		return sdkerrors.Wrapf(err, "failed to validate controller port %s", portID)
 	}
 
@@ -104,8 +104,9 @@ func (k Keeper) OnChanCloseConfirm(
 
 // validateControllerPortParams asserts the provided connection sequence and counterparty connection sequence
 // match that of the associated connection stored in state
-func (k Keeper) validateControllerPortParams(ctx sdk.Context, channelID, portID string, connectionSeq, counterpartyConnectionSeq uint64) error {
-	connectionID, connection, err := k.channelKeeper.GetChannelConnection(ctx, portID, channelID)
+func (k Keeper) validateControllerPortParams(ctx sdk.Context, connectionHops []string, connectionSeq, counterpartyConnectionSeq uint64) error {
+	connectionID := connectionHops[0]
+	connection, err := k.channelKeeper.GetConnection(ctx, connectionID)
 	if err != nil {
 		return err
 	}

modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go

@@ -59,13 +59,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
 			},
 			false,
 		},
-		{
-			"channel not found",
-			func() {
-				path.EndpointA.ChannelID = "invalid-channel-id"
-			},
-			false,
-		},
 		{
 			"connection not found",
 			func() {

modules/apps/27-interchain-accounts/host/keeper/handshake.go

@@ -43,7 +43,7 @@ func (k Keeper) OnChanOpenTry(
 		return sdkerrors.Wrapf(err, "expected format %s, got %s", icatypes.ControllerPortFormat, counterparty.PortId)
 	}
 
-	if err := k.validateControllerPortParams(ctx, channelID, portID, connSequence, counterpartyConnSequence); err != nil {
+	if err := k.validateControllerPortParams(ctx, connectionHops, connSequence, counterpartyConnSequence); err != nil {
 		return sdkerrors.Wrapf(err, "failed to validate controller port %s", counterparty.PortId)
 	}
 
@@ -104,8 +104,9 @@ func (k Keeper) OnChanCloseConfirm(
 
 // validateControllerPortParams asserts the provided connection sequence and counterparty connection sequence
 // match that of the associated connection stored in state
-func (k Keeper) validateControllerPortParams(ctx sdk.Context, channelID, portID string, connectionSeq, counterpartyConnectionSeq uint64) error {
-	connectionID, connection, err := k.channelKeeper.GetChannelConnection(ctx, portID, channelID)
+func (k Keeper) validateControllerPortParams(ctx sdk.Context, connectionHops []string, connectionSeq, counterpartyConnectionSeq uint64) error {
+	connectionID := connectionHops[0]
+	connection, err := k.channelKeeper.GetConnection(ctx, connectionID)
 	if err != nil {
 		return err
 	}

modules/apps/27-interchain-accounts/host/keeper/handshake_test.go

@@ -51,13 +51,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 			},
 			false,
 		},
-		{
-			"channel not found",
-			func() {
-				path.EndpointB.ChannelID = "invalid-channel-id"
-			},
-			false,
-		},
 		{
 			"connection not found",
 			func() {

modules/apps/27-interchain-accounts/types/expected_keepers.go

@@ -27,7 +27,7 @@ type ICS4Wrapper interface {
 type ChannelKeeper interface {
 	GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool)
 	GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool)
-	GetChannelConnection(ctx sdk.Context, portID, channelID string) (string, ibcexported.ConnectionI, error)
+	GetConnection(ctx sdk.Context, connectionID string) (ibcexported.ConnectionI, error)
 }
 
 // PortKeeper defines the expected IBC port keeper

modules/core/04-channel/keeper/handshake.go

@@ -1,6 +1,8 @@
 package keeper
 
 import (
+	"fmt"
+
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -53,14 +55,30 @@ func (k Keeper) ChanOpenInit(
 	}
 
 	channelID := k.GenerateChannelIdentifier(ctx)
-	channel := types.NewChannel(types.INIT, order, counterparty, connectionHops, version)
-	k.SetChannel(ctx, portID, channelID, channel)
 
 	capKey, err := k.scopedKeeper.NewCapability(ctx, host.ChannelCapabilityPath(portID, channelID))
 	if err != nil {
 		return "", nil, sdkerrors.Wrapf(err, "could not create channel capability for port ID %s and channel ID %s", portID, channelID)
 	}
 
+	return channelID, capKey, nil
+}
+
+// WriteOpenInitChannel writes a channel which has successfully passed the OpenInit handshake step.
+// The channel is set in state and all the associated Send and Recv sequences are set to 1.
+// An event is emitted for the handshake step.
+func (k Keeper) WriteOpenInitChannel(
+	ctx sdk.Context,
+	portID,
+	channelID string,
+	order types.Order,
+	connectionHops []string,
+	counterparty types.Counterparty,
+	version string,
+) {
+	channel := types.NewChannel(types.INIT, order, counterparty, connectionHops, version)
+	k.SetChannel(ctx, portID, channelID, channel)
+
 	k.SetNextSequenceSend(ctx, portID, channelID, 1)
 	k.SetNextSequenceRecv(ctx, portID, channelID, 1)
 	k.SetNextSequenceAck(ctx, portID, channelID, 1)
@@ -82,7 +100,12 @@ func (k Keeper) ChanOpenInit(
 		),
 	})
 
-	return channelID, capKey, nil
+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+		),
+	})
 }
 
 // ChanOpenTry is called by a module to accept the first step of a channel opening
@@ -171,16 +194,13 @@ func (k Keeper) ChanOpenTry(
 		)
 	}
 
-	// NOTE: this step has been switched with the one below to reverse the connection
-	// hops
-	channel := types.NewChannel(types.TRYOPEN, order, counterparty, connectionHops, version)
 	counterpartyHops := []string{connectionEnd.GetCounterparty().GetConnectionID()}
 
 	// expectedCounterpaty is the counterparty of the counterparty's channel end
 	// (i.e self)
 	expectedCounterparty := types.NewCounterparty(portID, "")
 	expectedChannel := types.NewChannel(
-		types.INIT, channel.Ordering, expectedCounterparty,
+		types.INIT, order, expectedCounterparty,
 		counterpartyHops, counterpartyVersion,
 	)
 
@@ -202,9 +222,6 @@ func (k Keeper) ChanOpenTry(
 			return "", nil, sdkerrors.Wrapf(err, "could not create channel capability for port ID %s and channel ID %s", portID, channelID)
 		}
 
-		k.SetNextSequenceSend(ctx, portID, channelID, 1)
-		k.SetNextSequenceRecv(ctx, portID, channelID, 1)
-		k.SetNextSequenceAck(ctx, portID, channelID, 1)
 	} else {
 		// capability initialized in ChanOpenInit
 		capKey, found = k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(portID, channelID))
@@ -215,6 +232,30 @@ func (k Keeper) ChanOpenTry(
 		}
 	}
 
+	return channelID, capKey, nil
+}
+
+// WriteOpenTryChannel writes a channel which has successfully passed the OpenTry handshake step.
+// The channel is set in state. If a previous channel state did not exist, all the Send and Recv
+// sequences are set to 1. An event is emitted for the handshake step.
+func (k Keeper) WriteOpenTryChannel(
+	ctx sdk.Context,
+	portID,
+	channelID string,
+	order types.Order,
+	connectionHops []string,
+	counterparty types.Counterparty,
+	version string,
+) {
+	previousChannel, previousChannelFound := k.GetChannel(ctx, portID, channelID)
+	if !previousChannelFound {
+		k.SetNextSequenceSend(ctx, portID, channelID, 1)
+		k.SetNextSequenceRecv(ctx, portID, channelID, 1)
+		k.SetNextSequenceAck(ctx, portID, channelID, 1)
+	}
+
+	channel := types.NewChannel(types.TRYOPEN, order, counterparty, connectionHops, version)
+
 	k.SetChannel(ctx, portID, channelID, channel)
 
 	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousChannel.State.String(), "new-state", "TRYOPEN")
@@ -233,8 +274,12 @@ func (k Keeper) ChanOpenTry(
 			sdk.NewAttribute(types.AttributeKeyConnectionID, channel.ConnectionHops[0]),
 		),
 	})
-
-	return channelID, capKey, nil
+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+		),
+	})
 }
 
 // ChanOpenAck is called by the handshake-originating module to acknowledge the
@@ -294,17 +339,34 @@ func (k Keeper) ChanOpenAck(
 		return err
 	}
 
-	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", channel.State.String(), "new-state", "OPEN")
+	return nil
+}
 
-	defer func() {
-		telemetry.IncrCounter(1, "ibc", "channel", "open-ack")
-	}()
+// WriteOpenAckChannel writes an updated channel state for the successful OpenAck handshake step.
+// An event is emitted for the handshake step.
+func (k Keeper) WriteOpenAckChannel(
+	ctx sdk.Context,
+	portID,
+	channelID,
+	counterpartyVersion,
+	counterpartyChannelID string,
+) {
+	channel, found := k.GetChannel(ctx, portID, channelID)
+	if !found {
+		panic(fmt.Sprintf("could not find existing channel when updating channel state in successful ChanOpenAck step, channelID: %s, portID: %s", channelID, portID))
+	}
 
 	channel.State = types.OPEN
 	channel.Version = counterpartyVersion
 	channel.Counterparty.ChannelId = counterpartyChannelID
 	k.SetChannel(ctx, portID, channelID, channel)
 
+	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", channel.State.String(), "new-state", "OPEN")
+
+	defer func() {
+		telemetry.IncrCounter(1, "ibc", "channel", "open-ack")
+	}()
+
 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
 			types.EventTypeChannelOpenAck,
@@ -315,8 +377,13 @@ func (k Keeper) ChanOpenAck(
 			sdk.NewAttribute(types.AttributeKeyConnectionID, channel.ConnectionHops[0]),
 		),
 	})
+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+		),
+	})
 
-	return nil
 }
 
 // ChanOpenConfirm is called by the counterparty module to close their end of the
@@ -373,6 +440,21 @@ func (k Keeper) ChanOpenConfirm(
 		return err
 	}
 
+	return nil
+}
+
+// WriteOpenConfirmChannel writes an updated channel state for the successful OpenConfirm handshake step.
+// An event is emitted for the handshake step.
+func (k Keeper) WriteOpenConfirmChannel(
+	ctx sdk.Context,
+	portID,
+	channelID string,
+) {
+	channel, found := k.GetChannel(ctx, portID, channelID)
+	if !found {
+		panic(fmt.Sprintf("could not find existing channel when updating channel state in successful ChanOpenConfirm step, channelID: %s, portID: %s", channelID, portID))
+	}
+
 	channel.State = types.OPEN
 	k.SetChannel(ctx, portID, channelID, channel)
 	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", "TRYOPEN", "new-state", "OPEN")
@@ -391,8 +473,12 @@ func (k Keeper) ChanOpenConfirm(
 			sdk.NewAttribute(types.AttributeKeyConnectionID, channel.ConnectionHops[0]),
 		),
 	})
-
-	return nil
+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+		),
+	})
 }
 
 // Closing Handshake

modules/core/04-channel/keeper/keeper.go

@@ -403,6 +403,16 @@ func (k Keeper) GetChannelClientState(ctx sdk.Context, portID, channelID string)
 	return connection.ClientId, clientState, nil
 }
 
+// GetConnection wraps the conenction keeper's GetConnection function.
+func (k Keeper) GetConnection(ctx sdk.Context, connectionID string) (exported.ConnectionI, error) {
+	connection, found := k.connectionKeeper.GetConnection(ctx, connectionID)
+	if !found {
+		return nil, sdkerrors.Wrapf(connectiontypes.ErrConnectionNotFound, "connection-id: %s", connectionID)
+	}
+
+	return connection, nil
+}
+
 // GetChannelConnection returns the connection ID and state associated with the given port and channel identifier.
 func (k Keeper) GetChannelConnection(ctx sdk.Context, portID, channelID string) (string, exported.ConnectionI, error) {
 	channel, found := k.GetChannel(ctx, portID, channelID)