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

reorganize channel handshake handler #647

Merged
merged 7 commits into from
Dec 21, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 8 additions & 0 deletions docs/migrations/v2-to-v3.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

applications must not rely on channel state being set before the application callback

Copy link
Contributor

Choose a reason for hiding this comment

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

should we document this somewhere? 🤔

Copy link
Contributor Author

@colin-axner colin-axner Dec 21, 2021

Choose a reason for hiding this comment

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

I'll update the docs when I do the OnChanOpenTry change, for now it is in the migration docs

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I was just thinking out loud :)

connectionID := connectionHops[0]
connection, err := k.channelKeeper.GetConnection(ctx, connectionID)
if err != nil {
return err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
},
false,
},
{
"channel not found",
func() {
path.EndpointA.ChannelID = "invalid-channel-id"
},
false,
},
{
"connection not found",
func() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
},
false,
},
{
"channel not found",
func() {
path.EndpointB.ChannelID = "invalid-channel-id"
},
false,
},
{
"connection not found",
func() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
124 changes: 105 additions & 19 deletions modules/core/04-channel/keeper/handshake.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -82,7 +100,12 @@ func (k Keeper) ChanOpenInit(
),
})

return channelID, capKey, nil
ctx.EventManager().EmitEvents(sdk.Events{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

someone can do a followup pr to add a function EmitOpenInitEvents to the 04-channel/keeper/events.go file for each handshake step

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
Expand Down Expand Up @@ -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,
)

Expand All @@ -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))
Expand All @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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")
Expand All @@ -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
Expand Down
10 changes: 10 additions & 0 deletions modules/core/04-channel/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading