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

ICA Controller Side Middleware #417

Merged
merged 24 commits into from
Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7de03ee
initial draft for ica middleware
colin-axner Sep 16, 2021
9f0f495
add capability handling, fix build and tests
colin-axner Sep 17, 2021
51fe4b2
merge interchain-accounts branch and fix conflicts
colin-axner Sep 29, 2021
0eb681a
split module.go into ibc_module.go
colin-axner Sep 29, 2021
e3a3d8f
merge interchain-accounts branch, fix merge conflicts
colin-axner Sep 29, 2021
4404535
add middleware handshake tests
colin-axner Sep 29, 2021
eb071fb
fix app.go wiring and various tests
colin-axner Oct 5, 2021
607aa8f
merge interchain accounts branch
colin-axner Oct 8, 2021
a6066da
godoc self nits
colin-axner Oct 12, 2021
eccb110
merge interchain account branch
colin-axner Oct 12, 2021
70bf29b
remove unnecessary error
colin-axner Oct 12, 2021
205d6f0
update comment
colin-axner Nov 1, 2021
ee61276
merge interchain-accounts branch
colin-axner Nov 2, 2021
8e279ea
merge interchain accounts branch
colin-axner Nov 4, 2021
7baafe0
fix testing build
colin-axner Nov 4, 2021
b166880
split channel keeper interface
colin-axner Nov 4, 2021
45bee51
fix tests
colin-axner Nov 4, 2021
816293b
remove comments
colin-axner Nov 4, 2021
9a0e7de
add callback for timeouts
colin-axner Nov 4, 2021
b2db520
Apply suggestions from code review
colin-axner Nov 5, 2021
c4a4aeb
fix test and update testing README
colin-axner Nov 5, 2021
46f30de
add OnRecvPacket test
colin-axner Nov 5, 2021
e22684b
Merge branch 'colin/313-ica-middleware' of github.com:cosmos/ibc-go i…
colin-axner Nov 5, 2021
62dd3d3
add failing test case for NegotiateAppVersion
colin-axner Nov 5, 2021
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
62 changes: 40 additions & 22 deletions modules/apps/27-interchain-accounts/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@ func NewIBCModule(k keeper.Keeper, app porttypes.IBCModule) IBCModule {
}
}

// Implement IBCModule callbacks
// OnChanOpenInit implements the IBCModule callbacks. Interchain Accounts is
// implemented to act as middleware for connected authentication modules on
// the controller side. The connected modules may not change the portID or
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// version. They will be allowed to perform custom logic without changing
// the parameters stored within a channel struct.
//
// Controller Chain
func (im IBCModule) OnChanOpenInit(
ctx sdk.Context,
order channeltypes.Order,
Expand All @@ -40,9 +46,18 @@ func (im IBCModule) OnChanOpenInit(
counterparty channeltypes.Counterparty,
version string,
) error {
return im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version)
if err := im.keeper.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version); err != nil {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return err
}

// 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 callbacks.
//
// Host Chain
func (im IBCModule) OnChanOpenTry(
ctx sdk.Context,
order channeltypes.Order,
Expand All @@ -57,15 +72,26 @@ func (im IBCModule) OnChanOpenTry(
return im.keeper.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, chanCap, counterparty, version, counterpartyVersion)
}

// OnChanOpenAck implements the IBCModule callbacks.
//
// Controller Chain
func (im IBCModule) OnChanOpenAck(
ctx sdk.Context,
portID,
channelID string,
counterpartyVersion string,
) error {
return im.keeper.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion)
if err := im.keeper.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion); err != nil {
return err
}

// call underlying app's OnChanOpenAck callback with the counterparty app version.
return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyVersion)
}

// OnChanOpenAck implements the IBCModule callbacks.
//
// Host Chain
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
func (im IBCModule) OnChanOpenConfirm(
ctx sdk.Context,
portID,
Expand All @@ -74,6 +100,7 @@ func (im IBCModule) OnChanOpenConfirm(
return im.keeper.OnChanOpenConfirm(ctx, portID, channelID)
}

// OnChanCloseInit implements the IBCModule callbacks.
func (im IBCModule) OnChanCloseInit(
ctx sdk.Context,
portID,
Expand All @@ -83,6 +110,7 @@ func (im IBCModule) OnChanCloseInit(
return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel")
}

// OnChanCloseConfirm implements the IBCModule callbacks.
func (im IBCModule) OnChanCloseConfirm(
ctx sdk.Context,
portID,
Expand All @@ -91,6 +119,7 @@ func (im IBCModule) OnChanCloseConfirm(
return nil
}

// OnRecvPacket implements the IBCModule callbacks.
func (im IBCModule) OnRecvPacket(
ctx sdk.Context,
packet channeltypes.Packet,
Expand All @@ -116,36 +145,25 @@ func (im IBCModule) OnRecvPacket(
return ack
}

// OnAcknowledgementPacket implements the IBCModule callbacks.
func (im IBCModule) OnAcknowledgementPacket(
ctx sdk.Context,
packet channeltypes.Packet,
acknowledgement []byte,
_ sdk.AccAddress,
relayer sdk.AccAddress,
) error {
var ack channeltypes.Acknowledgement

if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-27 interchain account packet acknowledgment: %v", err)
}
var data types.IBCAccountPacketData
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-27 interchain account packet data: %s", err.Error())
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

if err := im.keeper.OnAcknowledgementPacket(ctx, packet, data, ack); err != nil {
return err
}

return nil
// call underlying app's OnAcknowledgementPacket callback.
return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

// OnTimeoutPacket implements the IBCModule callbacks.
func (im IBCModule) OnTimeoutPacket(
ctx sdk.Context,
packet channeltypes.Packet,
_ sdk.AccAddress,
relayer sdk.AccAddress,
) error {
// TODO
return nil
// call underlying app's OnTimeoutPacket callback
return im.app.OnTimeoutPacket(ctx, packet, relayer)
}

// NegotiateAppVersion implements the IBCModule interface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"testing"

sdk "github.com/cosmos/cosmos-sdk/types"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
"github.com/stretchr/testify/suite"
"github.com/tendermint/tendermint/crypto"

Expand Down Expand Up @@ -34,7 +35,6 @@ type InterchainAccountsTestSuite struct {
// testing chains used for convenience and readability
chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain
}

func TestICATestSuite(t *testing.T) {
Expand All @@ -45,7 +45,20 @@ func (suite *InterchainAccountsTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(2))

// modify mock module (ica authentication module) to no-op on OpenChanInit
// instead of attempting to claim channel capability
mockModuleA := suite.chainA.GetSimApp().GetMockModule()
mockModuleB := suite.chainB.GetSimApp().GetMockModule()
onChanOpenInit := func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
portID, channelID string, chanCap *capabilitytypes.Capability,
counterparty channeltypes.Counterparty, version string,
) error {
// do not claim channel capability
return nil
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
mockModuleA.IBCApp.OnChanOpenInit = onChanOpenInit
mockModuleB.IBCApp.OnChanOpenInit = onChanOpenInit
}

func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
Expand Down Expand Up @@ -195,7 +208,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
}

func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() {
suite.SetupTest() // reset
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

Expand Down
2 changes: 1 addition & 1 deletion modules/apps/27-interchain-accounts/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func (k Keeper) OnChanOpenInit(

// Claim channel capability passed back by IBC module
if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
return sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, err.Error())
return sdkerrors.Wrap(err, "interchain accounts moudle could not claim capability passed back by the IBC module")
}

return nil
Expand Down
5 changes: 1 addition & 4 deletions modules/apps/27-interchain-accounts/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ type Keeper struct {
storeKey sdk.StoreKey
cdc codec.BinaryCodec

hook types.IBCAccountHooks

channelKeeper types.ChannelKeeper
portKeeper types.PortKeeper
accountKeeper types.AccountKeeper
Expand All @@ -37,7 +35,7 @@ type Keeper struct {
func NewKeeper(
cdc codec.BinaryCodec, key sdk.StoreKey,
channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper,
accountKeeper types.AccountKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, msgRouter *baseapp.MsgServiceRouter, hook types.IBCAccountHooks,
accountKeeper types.AccountKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, msgRouter *baseapp.MsgServiceRouter,
) Keeper {

// ensure ibc interchain accounts module account is set
Expand All @@ -53,7 +51,6 @@ func NewKeeper(
accountKeeper: accountKeeper,
scopedKeeper: scopedKeeper,
msgRouter: msgRouter,
hook: hook,
}
}

Expand Down
46 changes: 14 additions & 32 deletions modules/apps/27-interchain-accounts/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
"github.com/tendermint/tendermint/crypto/tmhash"

"github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types"
Expand All @@ -13,9 +14,9 @@ import (
host "github.com/cosmos/ibc-go/v2/modules/core/24-host"
)

// TODO: implement middleware functionality, this will allow us to use capabilities to
// manage helper module access to owner addresses they do not have capabilities for
func (k Keeper) TrySendTx(ctx sdk.Context, portID string, data interface{}) ([]byte, error) {
// TrySendTx takes in a transaction from a base application and attempts to send the packet
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// if the base application has the capability to send on the provided portID
func (k Keeper) TrySendTx(ctx sdk.Context, appCap *capabilitytypes.Capability, portID string, data interface{}) ([]byte, error) {
// Check for the active channel
activeChannelId, found := k.GetActiveChannel(ctx, portID)
if !found {
Expand All @@ -24,9 +25,13 @@ func (k Keeper) TrySendTx(ctx sdk.Context, portID string, data interface{}) ([]b

sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelId)
if !found {
return []byte{}, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelId)
return nil, sdkerrors.Wrap(channeltypes.ErrChannelNotFound, activeChannelId)
}

// if !k.AuthenticateCapability(ctx, appCap, types.AppCapabilityName(portID, activeChannelId)) {
// return nil, sdkerrors.Wrapf(types.ErrAppCapabilityNotFound, "could not authenticate provided capability for portID %s and channelID %s", portID, activeChannelId)
// }

destinationPort := sourceChannelEnd.GetCounterparty().GetPortID()
destinationChannel := sourceChannelEnd.GetCounterparty().GetChannelID()

Expand Down Expand Up @@ -81,7 +86,11 @@ func (k Keeper) createOutgoingPacket(
timeoutTimestamp,
)

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

return k.ComputeVirtualTxHash(packetData.Data, packet.Sequence), nil
}

func (k Keeper) DeserializeTx(_ sdk.Context, txBytes []byte) ([]sdk.Msg, error) {
Expand Down Expand Up @@ -212,30 +221,3 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error
return types.ErrUnknownPacketData
}
}

func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.IBCAccountPacketData, ack channeltypes.Acknowledgement) error {
switch ack.Response.(type) {
case *channeltypes.Acknowledgement_Error:
if k.hook != nil {
k.hook.OnTxFailed(ctx, packet.SourcePort, packet.SourceChannel, k.ComputeVirtualTxHash(data.Data, packet.Sequence), data.Data)
}
return nil
case *channeltypes.Acknowledgement_Result:
if k.hook != nil {
k.hook.OnTxSucceeded(ctx, packet.SourcePort, packet.SourceChannel, k.ComputeVirtualTxHash(data.Data, packet.Sequence), data.Data)
}
return nil
default:
// the acknowledgement succeeded on the receiving chain so nothing
// needs to be executed and no error needs to be returned
return nil
}
}

func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, data types.IBCAccountPacketData) error {
if k.hook != nil {
k.hook.OnTxFailed(ctx, packet.SourcePort, packet.SourceChannel, k.ComputeVirtualTxHash(data.Data, packet.Sequence), data.Data)
}

return nil
}
11 changes: 9 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/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"

ibctesting "github.com/cosmos/ibc-go/v2/testing"
)

Expand Down Expand Up @@ -62,6 +61,14 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, "channel-100")
}, false,
},
/* {
"could not authenticate app capability", func() {
amount, _ := sdk.ParseCoinsNormalized("100stake")
interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
appCap = nil
}, false,
},*/
{
"data is nil", func() {
msg = nil
Expand Down Expand Up @@ -89,7 +96,7 @@ func (suite *KeeperTestSuite) TestTrySendTx() {

tc.malleate()

_, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), portID, msg)
_, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), nil, portID, msg)

if tc.expPass {
suite.Require().NoError(err)
Expand Down
6 changes: 6 additions & 0 deletions modules/apps/27-interchain-accounts/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ var (
_ module.AppModule = AppModule{}
_ porttypes.IBCModule = IBCModule{}
_ module.AppModuleBasic = AppModuleBasic{}

// Middleware must implement types.ChannelKeeper and types.PortKeeper expected interfaces
// so that it can wrap IBC channel and port logic for underlying application.
// _ types.ChannelKeeper = Keeper{}
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep these commented out interface assertions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed these assertions. ICA is only middleware for ICA authentication modules which must rely on TrySendTx function, not SendPacket and WriteAcknowledgement. The middleware interaction is more restricted than on 29-fee which is very open and thus must fulfill the entire interface

cc @AdityaSripal

// _ types.PortKeeper = Keeper{}
)

type AppModuleBasic struct{}
Expand Down Expand Up @@ -72,6 +77,7 @@ type AppModule struct {
keeper keeper.Keeper
}

// NewAppModule creates an interchain accounts app module.
func NewAppModule(k keeper.Keeper) AppModule {
return AppModule{
keeper: k,
Expand Down
1 change: 1 addition & 0 deletions modules/apps/27-interchain-accounts/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ var (
ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version")
ErrInvalidAccountAddress = sdkerrors.Register(ModuleName, 12, "invalid account address")
ErrUnsupported = sdkerrors.Register(ModuleName, 13, "interchain account does not support this action")
ErrAppCapabilityNotFound = sdkerrors.Register(ModuleName, 14, "app capability not found")
)
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ 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
ChanCloseInit(ctx sdk.Context, portID, channelID string, chanCap *capabilitytypes.Capability) error
ChanOpenInit(ctx sdk.Context, order channeltypes.Order, connectionHops []string, portID string, portCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, version string) (string, *capabilitytypes.Capability, error)
}

Expand Down
30 changes: 0 additions & 30 deletions modules/apps/27-interchain-accounts/types/hook.go

This file was deleted.

Loading