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 counterparty port ID to controller portID #319

Merged
merged 5 commits into from
Aug 9, 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
3 changes: 1 addition & 2 deletions docs/ibc/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -343,8 +343,7 @@ Query request for an interchain account address

| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `owner_address` | [string](#string) | | Owner address is the owner of the interchain account on the controller chain |
| `connection_id` | [string](#string) | | |
| `counterparty_port_id` | [string](#string) | | Counterparty PortID is the portID on the controller chain |



Expand Down
7 changes: 3 additions & 4 deletions modules/apps/27-interchain-accounts/client/cli/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,17 @@ func GetQueryCmd() *cobra.Command {

func GetInterchainAccountCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "address [address] [connection-id]",
Use: "address [counterparty-port-id]",
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
}

ownerAddress := args[0]
connectionId := args[1]
counterpartyPortID := args[0]

queryClient := types.NewQueryClient(clientCtx)
res, err := queryClient.InterchainAccountAddress(context.Background(), &types.QueryInterchainAccountAddressRequest{OwnerAddress: ownerAddress, ConnectionId: connectionId})
res, err := queryClient.InterchainAccountAddress(context.Background(), &types.QueryInterchainAccountAddressRequest{CounterpartyPortId: counterpartyPortID})
if err != nil {
return err
}
Expand Down
11 changes: 7 additions & 4 deletions modules/apps/27-interchain-accounts/keeper/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,24 @@ import (
// 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)
func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpartyConnectionID, owner string) 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.

Id -> ID

Historically we always used ID. We only use Id on proto defintions because of an issue with gRPC gateway not allowing ID. All other variables names in core IBC use ID

portId, err := types.GeneratePortID(owner, connectionID, counterpartyConnectionID)
if err != nil {
return err
}

// 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))
err = k.ClaimCapability(ctx, portCap, host.PortPath(portId))
if err != nil {
return sdkerrors.Wrap(err, "unable to bind to newly generated portID")
}

msg := channeltypes.NewMsgChannelOpenInit(portId, types.Version, channeltypes.ORDERED, []string{connectionId}, types.PortID, types.ModuleName)
msg := channeltypes.NewMsgChannelOpenInit(portId, types.Version, channeltypes.ORDERED, []string{connectionID}, types.PortID, types.ModuleName)
handler := k.msgRouter.Handler(msg)
if _, err := handler(ctx, msg); err != nil {
return err
Expand Down
6 changes: 4 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/account_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
ibctesting "github.com/cosmos/ibc-go/testing"
)

Expand Down Expand Up @@ -32,7 +33,8 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
*/
{
"MsgChanOpenInit fails - channel is already active", func() {
portID := suite.chainA.GetSimApp().ICAKeeper.GeneratePortId(owner, path.EndpointA.ConnectionID)
portID, err := types.GeneratePortID(owner, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
suite.Require().NoError(err)
suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, path.EndpointA.ChannelID)
}, false,
},
Expand All @@ -49,7 +51,7 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {

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

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

if tc.expPass {
suite.Require().NoError(err)
Expand Down
7 changes: 3 additions & 4 deletions modules/apps/27-interchain-accounts/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,13 @@ func (k Keeper) InterchainAccountAddress(ctx context.Context, req *types.QueryIn
return nil, status.Error(codes.InvalidArgument, "empty request")
}

if req.OwnerAddress == "" {
return nil, status.Error(codes.InvalidArgument, "address cannot be empty")
if req.CounterpartyPortId == "" {
return nil, status.Error(codes.InvalidArgument, "counterparty portID cannot be empty")
}

sdkCtx := sdk.UnwrapSDKContext(ctx)
portId := k.GeneratePortId(req.OwnerAddress, req.ConnectionId)

interchainAccountAddress, err := k.GetInterchainAccountAddress(sdkCtx, portId)
interchainAccountAddress, err := k.GetInterchainAccountAddress(sdkCtx, req.CounterpartyPortId)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/handshake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
channel *channeltypes.Channel
path *ibctesting.Path
chanCap *capabilitytypes.Capability
err error
)

testCases := []struct {
Expand Down Expand Up @@ -63,7 +62,8 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
suite.coordinator.SetupConnections(path)

// mock init interchain account
portID := suite.chainA.GetSimApp().ICAKeeper.GeneratePortId("owner", path.EndpointA.ConnectionID)
portID, err := types.GeneratePortID("owner", path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
suite.Require().NoError(err)
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
Expand Down
14 changes: 0 additions & 14 deletions modules/apps/27-interchain-accounts/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package keeper

import (
"fmt"
"strings"

baseapp "github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -123,19 +122,6 @@ func (k Keeper) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability
return k.scopedKeeper.ClaimCapability(ctx, cap, name)
}

// Utility function for parsing the connection number from the connection-id
func getConnectionNumber(connectionId string) string {
ss := strings.Split(connectionId, "-")
return ss[len(ss)-1]
}

func (k Keeper) GeneratePortId(owner, connectionId string) string {
ownerId := strings.TrimSpace(owner)
connectionNumber := getConnectionNumber(connectionId)
portId := types.IcaPrefix + connectionNumber + "-" + ownerId
return portId
}

func (k Keeper) SetActiveChannel(ctx sdk.Context, portId, channelId string) error {
store := ctx.KVStore(k.storeKey)

Expand Down
8 changes: 6 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,15 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
}

// InitInterchainAccount is a helper function for starting the channel handshake
// TODO: parse identifiers from events
func InitInterchainAccount(endpoint *ibctesting.Endpoint, owner string) error {
portID := endpoint.Chain.GetSimApp().ICAKeeper.GeneratePortId(owner, endpoint.ConnectionID)
portID, err := types.GeneratePortID(owner, endpoint.ConnectionID, endpoint.Counterparty.ConnectionID)
if err != nil {
return err
}
channelSequence := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(endpoint.Chain.GetContext())

if err := endpoint.Chain.GetSimApp().ICAKeeper.InitInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, owner); err != nil {
if err := endpoint.Chain.GetSimApp().ICAKeeper.InitInterchainAccount(endpoint.Chain.GetContext(), endpoint.ConnectionID, endpoint.Counterparty.ConnectionID, owner); err != nil {
return err
}

Expand Down
11 changes: 6 additions & 5 deletions modules/apps/27-interchain-accounts/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,24 @@ import (
"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)
// 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) {
// Check for the active channel
activeChannelId, found := k.GetActiveChannel(ctx, portId)
activeChannelId, found := k.GetActiveChannel(ctx, portID)
if !found {
return nil, types.ErrActiveChannelNotFound
}

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

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

return k.createOutgoingPacket(ctx, portId, activeChannelId, destinationPort, destinationChannel, data)
return k.createOutgoingPacket(ctx, portID, activeChannelId, destinationPort, destinationChannel, data)
}

func (k Keeper) createOutgoingPacket(
Expand Down
30 changes: 29 additions & 1 deletion modules/apps/27-interchain-accounts/types/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,46 @@ package types
import (
"encoding/json"
"fmt"
"strings"

yaml "gopkg.in/yaml.v2"

crypto "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"

connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types"
)

const (
IcaPrefix string = "ics27-1-"
ICAPrefix string = "ics-27"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the spec specifies two different formats:
ics27-1 and ics-27

my preference is not to reference any version number in the ports, we should never have a reason to change the port ID generation as backwards compatibility/migration would be a pain, it'd be a different module at that point

Either ics27 or ics-27 sounds fine to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets go with ics-27 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually just remembered why I kept the -1 version. I figured in the situation whereby we might have a version -2 we may still have interchain accounts from version 1 still active and we will want to differentiate. A version two could potentially have 1 owner for N accounts rather than 1 to 1 mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't this be represented in the actual interchain accounts version?

Copy link
Contributor

@seantking seantking Aug 6, 2021

Choose a reason for hiding this comment

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

Let's say we have a bunch of interchain accounts registered with version 1. If we upgrade the architecture to version 2 we have to either:

  1. keep supporting the version 1 account
    or
  2. provide a migration path for all of the currently existing interchain accounts.

I imagined the first choice to be easier and considered using the version in the port-id as a future-proofing step if it's ever required.

It's a minimal addition that may save time in the future. It probably will never come up, but if it does it could be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

If a DAO registers an account on the hub with version 1 of interchain accounts, but version 2 (hypothetically) ended up having a different architecture, we would need to either migrate this account (ownership + any tokens) to version a version 2 account or keep supporting version 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The channel would still be accessible on both the host and controller side even if versions changed? My main hesitation is it seems odd to overload the port ID with information that could be contained in places it is designated for (app version in the channel version)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you're saying if both sides update to version 2 of interchain accounts, it will be possible to get the version string that was originally used to create the channel, and that we can use that to distinguish between accounts registered under different versions?

Getting very edge case here, but what would happen if the channel closed? Would we still be able to access the version string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the structure of tracking the active channels. The channel version is always accessible even if the channel is closed. The active channel mapping would just need to know the previous channel which was active

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, let's go with this for now. Before we go live with ICA I think it's a good idea to have some plan in place for how we would handle this kind of issue if it arises.

)

// GeneratePortID generates the portID for a specific owner
// on the controller chain in the format:
//
// 'ics-27-<connectionSequence>-<counterpartyConnectionSequence>-<owner-address>'
// https://github.com/seantking/ibc/tree/sean/ics-27-updates/spec/app/ics-027-interchain-accounts#registering--controlling-flows
// TODO: update link to spec
func GeneratePortID(owner, connectionID, counterpartyConnectionID string) (string, error) {
ownerID := strings.TrimSpace(owner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should consider enforcing limits on ownerID

if ownerID == "" {
return "", sdkerrors.Wrap(ErrInvalidOwnerAddress, "owner address cannot be empty")
}
connectionSeq, err := connectiontypes.ParseConnectionSequence(connectionID)
if err != nil {
return "", sdkerrors.Wrap(err, "invalid connection identifier")
}
counterpartyConnectionSeq, err := connectiontypes.ParseConnectionSequence(counterpartyConnectionID)
if err != nil {
return "", sdkerrors.Wrap(err, "invalid counterparty connection identifier")
}

portID := fmt.Sprintf("%s-%d-%d-%s", ICAPrefix, connectionSeq, counterpartyConnectionSeq, ownerID)
return portID, nil
}

type InterchainAccountI interface {
authtypes.AccountI
}
Expand Down
90 changes: 90 additions & 0 deletions modules/apps/27-interchain-accounts/types/account_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package types_test

import (
"testing"

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

type TypesTestSuite struct {
suite.Suite

coordinator *ibctesting.Coordinator

chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
}

func (suite *TypesTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)

suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
}

func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
path := ibctesting.NewPath(chainA, chainB)
path.EndpointA.ChannelConfig.PortID = types.PortID
path.EndpointB.ChannelConfig.PortID = types.PortID
path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED
path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED
path.EndpointA.ChannelConfig.Version = types.Version
path.EndpointB.ChannelConfig.Version = types.Version

return path
}

func TestTypesTestSuite(t *testing.T) {
suite.Run(t, new(TypesTestSuite))
}

func (suite *TypesTestSuite) TestGeneratePortID() {
var (
path *ibctesting.Path
owner string
)
var testCases = []struct {
name string
malleate func()
expValue string
expPass bool
}{
{"success", func() {}, "ics-27-0-0-owner123", true},
{"success with non matching connection sequences", func() {
path.EndpointA.ConnectionID = "connection-1"
}, "ics-27-1-0-owner123", true},
{"invalid owner address", func() {
owner = " "
}, "", false},
{"invalid connectionID", func() {
path.EndpointA.ConnectionID = "connection"
}, "", false},
{"invalid counterparty connectionID", func() {
path.EndpointB.ConnectionID = "connection"
}, "", false},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)
owner = "owner123" // must be explicitly changed

tc.malleate()

portID, err := types.GeneratePortID(owner, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
if tc.expPass {
suite.Require().NoError(err, tc.name)
suite.Require().Equal(tc.expValue, portID)
} else {
suite.Require().Error(err, tc.name)
suite.Require().Empty(portID)
}
})
}
}
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 @@ -15,4 +15,5 @@ var (
ErrInterchainAccountAlreadySet = sdkerrors.Register(ModuleName, 9, "Interchain Account is already set")
ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 10, "no active channel for this owner")
ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version")
ErrInvalidOwnerAddress = sdkerrors.Register(ModuleName, 12, "invalid owner address")
)
Loading