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

Add interchain account OnChanOpenInit and InitInterchainAccount tests #287

Merged
merged 3 commits into from
Jul 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
47 changes: 18 additions & 29 deletions modules/apps/27-interchain-accounts/keeper/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,51 +4,40 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/tendermint/tendermint/crypto/tmhash"

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
"github.com/tendermint/tendermint/crypto/tmhash"
)

// The first step in registering an interchain account
// Binds a new port & calls OnChanOpenInit
// InitInterchainAccount is the entry point to registering an interchain account.
// It generates a new port identifier using the owner address, connection identifier,
// and counterparty connection identifier. It will bind to the port identifier and
// call 04-channel 'ChanOpenInit'. An error is returned if the port identifier is
// already in use. Gaining access to interchain accounts whose channels have closed
// cannot be done with this function. A regular MsgChanOpenInit must be used.
func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionId, owner string) error {
portId := k.GeneratePortId(owner, connectionId)

// Check if the port is already bound
isBound := k.IsBound(ctx, portId)
if isBound == true {
// check if the port is already bound
if k.IsBound(ctx, portId) {
return sdkerrors.Wrap(types.ErrPortAlreadyBound, portId)
}

portCap := k.portKeeper.BindPort(ctx, portId)
err := k.ClaimCapability(ctx, portCap, host.PortPath(portId))
if err != nil {
return sdkerrors.Wrap(err, "unable to bind to newly generated portID")
}
Comment on lines 28 to +32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a followup pr can address this #289


msg := channeltypes.NewMsgChannelOpenInit(portId, types.Version, channeltypes.ORDERED, []string{connectionId}, types.PortID, types.ModuleName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note the sender is types.ModuleName, also see comment. When can change this later if we so desire, but lets discuss on the issue and not block dev work

handler := k.msgRouter.Handler(msg)
if _, err := handler(ctx, msg); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of interest, why did you go with this approach instead of what was there? It certainly looks cleaner with the refactor, is that the only reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong reasons. The previous functionality was definitively not ideal (calling 04-channel keeper and app callback). A better fix assumes that the 04-channel function handles all necessary functionality including app callback. Any change to this contract could cause future issues. Since we already have the message router, it seemed like the best option to me. It also aligns with the direction the SDK is pushing for so I figured future breaking changes would be easier to handle using this functionality

return err
}

counterParty := channeltypes.Counterparty{PortId: "ibcaccount", ChannelId: ""}
order := channeltypes.Order(2)
channelId, cap, err := k.channelKeeper.ChanOpenInit(ctx, order, []string{connectionId}, portId, portCap, counterParty, types.Version)

ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
channeltypes.EventTypeChannelOpenInit,
sdk.NewAttribute(channeltypes.AttributeKeyPortID, portId),
sdk.NewAttribute(channeltypes.AttributeKeyChannelID, channelId),
sdk.NewAttribute(channeltypes.AttributeCounterpartyPortID, "ibcaccount"),
sdk.NewAttribute(channeltypes.AttributeCounterpartyChannelID, ""),
sdk.NewAttribute(channeltypes.AttributeKeyConnectionID, connectionId),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, channeltypes.AttributeValueCategory),
),
})

_ = k.OnChanOpenInit(ctx, channeltypes.Order(2), []string{connectionId}, portId, channelId, cap, counterParty, types.Version)

return err
return nil
}

// Register interchain account if it has not already been created
Expand Down
62 changes: 62 additions & 0 deletions modules/apps/27-interchain-accounts/keeper/account_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package keeper_test

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

func (suite *KeeperTestSuite) TestInitInterchainAccount() {
var (
owner string
path *ibctesting.Path
err error
)

testCases := []struct {
name string
malleate func()
expPass bool
}{

{
"success", func() {}, true,
},
/*
// TODO: https://github.com/cosmos/ibc-go/issues/288
Copy link
Contributor Author

Choose a reason for hiding this comment

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

{
"port is already bound", func() {
// mock init interchain account
portID := suite.chainA.GetSimApp().ICAKeeper.GeneratePortId(owner, path.EndpointA.ConnectionID)
suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), portID)
}, false,
},
*/
{
"MsgChanOpenInit fails - channel is already active", func() {
portID := suite.chainA.GetSimApp().ICAKeeper.GeneratePortId(owner, path.EndpointA.ConnectionID)
suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, path.EndpointA.ChannelID)
}, false,
},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
suite.SetupTest() // reset
owner = "owner" // must be explicitly changed
path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

tc.malleate() // explicitly change fields in channel and testChannel

err = suite.chainA.GetSimApp().ICAKeeper.InitInterchainAccount(suite.chainA.GetContext(), path.EndpointA.ConnectionID, owner)

if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}

})
}
}
22 changes: 20 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,20 @@ 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/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
)

// OnChanOpenInit performs basic validation of channel initialization.
// The channel order must be ORDERED, the counterparty port identifier
// must be the host chain representation as defined in the types package,
// the channel version must be equal to the version in the types package,
// there must not be an active channel for the specfied port identifier,
// and the interchain accounts module must be able to claim the channel
// capability.
func (k Keeper) OnChanOpenInit(
ctx sdk.Context,
order channeltypes.Order,
Expand All @@ -18,11 +28,19 @@ func (k Keeper) OnChanOpenInit(
counterparty channeltypes.Counterparty,
version string,
) error {
//TODO:
// check version string
if order != channeltypes.ORDERED {
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "invalid channel ordering: %s, expected %s", order.String(), channeltypes.ORDERED.String())
}
if counterparty.PortId != types.PortID {
return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "counterparty port-id must be '%s', (%s != %s)", types.PortID, counterparty.PortId, types.PortID)
}
if version != types.Version {
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelVersion, "channel version must be '%s' (%s != %s)", types.Version, version, types.Version)
}
channelID, found := k.GetActiveChannel(ctx, portID)
if found {
return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "existing active channel (%s) for portID (%s)", channelID, portID)
}

// Claim channel capability passed back by IBC module
if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {
Expand Down
97 changes: 97 additions & 0 deletions modules/apps/27-interchain-accounts/keeper/handshake_test.go
Original file line number Diff line number Diff line change
@@ -1 +1,98 @@
package keeper_test

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

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
ibctesting "github.com/cosmos/ibc-go/testing"
)

func (suite *KeeperTestSuite) TestOnChanOpenInit() {
var (
channel *channeltypes.Channel
path *ibctesting.Path
chanCap *capabilitytypes.Capability
err error
)

testCases := []struct {
name string
malleate func()
expPass bool
}{

{
"success", func() {}, true,
},
{
"invalid order - UNORDERED", func() {
channel.Ordering = channeltypes.UNORDERED
}, false,
},
{
"invalid counterparty port ID", func() {
channel.Counterparty.PortId = ibctesting.MockPort
}, false,
},
{
"invalid version", func() {
channel.Version = "version"
}, false,
},
{
"channel is already active", func() {
suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, false,
},
{
"capability already claimed", func() {
err := suite.chainA.GetSimApp().ScopedICAKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
suite.Require().NoError(err)
}, false,
},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

// mock init interchain account
portID := suite.chainA.GetSimApp().ICAKeeper.GeneratePortId("owner", path.EndpointA.ConnectionID)
portCap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), portID)
suite.chainA.GetSimApp().ICAKeeper.ClaimCapability(suite.chainA.GetContext(), portCap, host.PortPath(portID))
path.EndpointA.ChannelConfig.PortID = portID

// default values
counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
channel = &channeltypes.Channel{
State: channeltypes.INIT,
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointA.ConnectionID},
Version: types.Version,
}

chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(portID, path.EndpointA.ChannelID))
suite.Require().NoError(err)

tc.malleate() // explicitly change fields in channel and testChannel

err = suite.chainA.GetSimApp().ICAKeeper.OnChanOpenInit(suite.chainA.GetContext(), channel.Ordering, channel.GetConnectionHops(),
path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, chanCap, channel.Counterparty, channel.GetVersion(),
)

if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}

})
}
}
14 changes: 10 additions & 4 deletions modules/apps/27-interchain-accounts/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
Expand Down Expand Up @@ -145,13 +144,20 @@ func (k Keeper) SetActiveChannel(ctx sdk.Context, portId, channelId string) erro
return nil
}

func (k Keeper) GetActiveChannel(ctx sdk.Context, portId string) (string, error) {
func (k Keeper) GetActiveChannel(ctx sdk.Context, portId string) (string, bool) {
store := ctx.KVStore(k.storeKey)
key := types.KeyActiveChannel(portId)
if !store.Has(key) {
return "", sdkerrors.Wrap(types.ErrActiveChannelNotFound, portId)
return "", false
}

activeChannel := string(store.Get(key))
return activeChannel, nil
return activeChannel, true
}

// IsActiveChannel returns true if there exists an active channel for
// the provided portID and false otherwise.
func (k Keeper) IsActiveChannel(ctx sdk.Context, portId string) bool {
_, found := k.GetActiveChannel(ctx, portId)
return found
}
8 changes: 4 additions & 4 deletions modules/apps/27-interchain-accounts/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
"github.com/tendermint/tendermint/crypto/tmhash"
)

func (k Keeper) TrySendTx(ctx sdk.Context, accountOwner sdk.AccAddress, connectionId string, data interface{}) ([]byte, error) {
portId := k.GeneratePortId(accountOwner.String(), connectionId)
// Check for the active channel
activeChannelId, err := k.GetActiveChannel(ctx, portId)
if err != nil {
return nil, err
activeChannelId, found := k.GetActiveChannel(ctx, portId)
if !found {
return nil, types.ErrActiveChannelNotFound
}

sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, portId, activeChannelId)
Expand Down
2 changes: 2 additions & 0 deletions modules/core/04-channel/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,6 @@ var (

// ORDERED channel error
ErrPacketSequenceOutOfOrder = sdkerrors.Register(SubModuleName, 21, "packet sequence is out of order")

ErrInvalidChannelVersion = sdkerrors.Register(SubModuleName, 24, "invalid channel version")
)