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.

Sign up for GitHub

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

feat: middleware support for ICS20 #533

Closed
wants to merge 7 commits into from
Closed

feat: middleware support for ICS20 #533

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae0629b
middleware support for ICS20
thomas-nguy Nov 10, 2021
9b4ac36
update tests
thomas-nguy Nov 16, 2021
a019faf
Merge branch 'main' into thomas/add-middleware-support-to-ICS20
fedekunze Dec 28, 2021
e18c29f
fix and changelog
fedekunze Dec 29, 2021
5826f83
test fixes
fedekunze Dec 31, 2021
13bb71e
panic if middleware already set
thomas-nguy Jan 1, 2022
4d8f264
fix
fedekunze Jan 3, 2022
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
34 changes: 30 additions & 4 deletions modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
// IBCModule implements the ICS26 interface for transfer given the transfer keeper.
type IBCModule struct {
keeper keeper.Keeper
app porttypes.IBCModule
}

// NewIBCModule creates a new IBCModule given the keeper
Expand Down Expand Up @@ -64,6 +65,11 @@ func ValidateTransferChannelParams(
return nil
}

// SetMiddleware sets ICS30 middleware
func (im IBCModule) SetMiddleware(app porttypes.IBCModule) {
im.app = app
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
}

// OnChanOpenInit implements the IBCModule interface
func (im IBCModule) OnChanOpenInit(
ctx sdk.Context,
Expand All @@ -84,7 +90,13 @@ func (im IBCModule) OnChanOpenInit(
return err
}

return nil
if im.app == nil {
return nil
}

// call underlying app's OnChanOpenInit callback with the appVersion
return im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID,
chanCap, counterparty, version)
}

// OnChanOpenTry implements the IBCModule interface
Expand Down Expand Up @@ -131,7 +143,12 @@ func (im IBCModule) OnChanOpenAck(
if counterpartyVersion != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version)
}
return nil

if im.app == nil {
return nil
}
// call underlying app's OnChanOpenAck callback with the counterparty app version.
return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning for now adding callbacks for OnChanOpenTry and OnChanOpenConfirm?

Copy link
Author

Choose a reason for hiding this comment

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

not much reason beside adding more flexibility, might be useful for the wired app? if not it should return nil anyways?

}

// OnChanOpenConfirm implements the IBCModule interface
Expand Down Expand Up @@ -249,7 +266,12 @@ func (im IBCModule) OnAcknowledgementPacket(
)
}

return nil
if im.app == nil {
return nil
}

// call underlying app's OnAcknowledgementPacket callback.
return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
}

// OnTimeoutPacket implements the IBCModule interface
Expand Down Expand Up @@ -277,7 +299,11 @@ func (im IBCModule) OnTimeoutPacket(
),
)

return nil
if im.app == nil {
return nil
}

return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

// NegotiateAppVersion implements the IBCModule interface
Expand Down
4 changes: 3 additions & 1 deletion modules/apps/transfer/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type Keeper struct {
cdc codec.BinaryCodec
paramSpace paramtypes.Subspace

ics4Wrapper types.ICS4Wrapper
channelKeeper types.ChannelKeeper
portKeeper types.PortKeeper
authKeeper types.AccountKeeper
Expand All @@ -31,7 +32,7 @@ type Keeper struct {
// NewKeeper creates a new IBC transfer Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key sdk.StoreKey, paramSpace paramtypes.Subspace,
channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper,
ics4Wrapper types.ICS4Wrapper, channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper,
authKeeper types.AccountKeeper, bankKeeper types.BankKeeper, scopedKeeper capabilitykeeper.ScopedKeeper,
) Keeper {

Expand All @@ -49,6 +50,7 @@ func NewKeeper(
cdc: cdc,
storeKey: key,
paramSpace: paramSpace,
ics4Wrapper: ics4Wrapper,
channelKeeper: channelKeeper,
portKeeper: portKeeper,
authKeeper: authKeeper,
Expand Down
11 changes: 10 additions & 1 deletion modules/apps/transfer/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package keeper

import (
"context"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v2/modules/core/24-host"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/ibc-go/v2/modules/apps/transfer/types"
Expand All @@ -19,8 +22,14 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
if err != nil {
return nil, err
}

channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(msg.SourcePort, msg.SourceChannel))
Copy link
Author

Choose a reason for hiding this comment

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

@colin-axner I am not totally sure how I should handle the channel cap here. I moved it to the msg_server level. Is there anything else that needs to be done?

Copy link
Contributor

@colin-axner colin-axner Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the delayed reply! This looks good to me, although I'm not sure it matters if ics20 continues to claim the channel capability in the channel handshake callbacks.

@AdityaSripal Do we want to allow applications using ics20 as middleware to claim the capability?

Copy link
Contributor

Choose a reason for hiding this comment

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

@thomas-nguy Is it desirable for your use case for the connected application to be capable of sending packets? Otherwise I'm thinking maybe we should leave all the capability management as it was. Essentially only allowing the connected application to get information about the ICS20 transfer, but not act upon it

Copy link
Author

@thomas-nguy thomas-nguy Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not necessary, but I did claim here to preserve the original behavior.

Before it was claimed at "SendTransfer" level, but this line needs to be removed since the capability is now passed by parameter

Copy link
Author

Choose a reason for hiding this comment

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

So in this case, we should have some sort of conditional logic that only claims capability if im.app is nil. This applies to both port and channnel capabilities.

Do you refer to this section @AdityaSripal ?

The capability here is claim by the msg_server which is outside the middleware layer. Not sure to understand which check you want to do

if !ok {
return nil, sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
}

if err := k.SendTransfer(
ctx, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
ctx, channelCap, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
); err != nil {
return nil, err
}
Expand Down
14 changes: 3 additions & 11 deletions modules/apps/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper

import (
"fmt"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
"strings"

"github.com/armon/go-metrics"
Expand All @@ -12,7 +13,6 @@ import (
"github.com/cosmos/ibc-go/v2/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v2/modules/core/24-host"
coretypes "github.com/cosmos/ibc-go/v2/modules/core/types"
)

Expand Down Expand Up @@ -50,6 +50,7 @@ import (
// 6. B -> A : sender chain is sink zone. Denom upon receiving: 'denom'
func (k Keeper) SendTransfer(
ctx sdk.Context,
chanCap *capabilitytypes.Capability,
sourcePort,
sourceChannel string,
token sdk.Coin,
Expand Down Expand Up @@ -82,11 +83,6 @@ func (k Keeper) SendTransfer(

// begin createOutgoingPacket logic
// See spec for this logic: https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer#packet-relay
channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(sourcePort, sourceChannel))
if !ok {
return sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
}

// NOTE: denomination and hex hash correctness checked during msg.ValidateBasic
fullDenomPath := token.Denom

Expand Down Expand Up @@ -158,10 +154,6 @@ func (k Keeper) SendTransfer(
timeoutTimestamp,
)

if err := k.channelKeeper.SendPacket(ctx, channelCap, packet); err != nil {
return err
}

defer func() {
if token.Amount.IsInt64() {
telemetry.SetGaugeWithLabels(
Expand All @@ -178,7 +170,7 @@ func (k Keeper) SendTransfer(
)
}()

return nil
return k.ics4Wrapper.SendPacket(ctx, chanCap, packet)
}

// OnRecvPacket processes a cross chain fungible token transfer. If the
Expand Down
6 changes: 5 additions & 1 deletion modules/apps/transfer/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,15 @@ type BankKeeper interface {
SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error
}

// ICS4Wrapper defines the expected ICS4Wrapper for middleware
type ICS4Wrapper interface {
SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error
}

// ChannelKeeper defines the expected IBC channel keeper
type ChannelKeeper interface {
GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool)
GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool)
SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error
}

// ClientKeeper defines the expected IBC client keeper
Expand Down