Add counterparty port ID to controller portID (#319)

* refactor! move GeneratePortID to types, add counterpartyConnection sequence

change all PortId -> PortID
move GeneratePortID to types package
add counterparty connection sequence argument
utilize connectiontypes connectionID parsing function

* refactor! use counterparty portID in interchain account address gRPC

Remove existing args from gRPC request for interchain account address
Use counterparty portID

* tests add generate port id tests

* apply self-review fixes

docs/ibc/proto-docs.md

@@ -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 |
 
 
 

modules/apps/27-interchain-accounts/client/cli/query.go

@@ -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
 			}

modules/apps/27-interchain-accounts/keeper/account.go

@@ -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 {
+	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

modules/apps/27-interchain-accounts/keeper/account_test.go

@@ -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"
 )
 
@@ -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,
 		},
@@ -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)

modules/apps/27-interchain-accounts/keeper/grpc_query.go

@@ -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
 	}

modules/apps/27-interchain-accounts/keeper/handshake_test.go

@@ -14,7 +14,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
 		channel *channeltypes.Channel
 		path    *ibctesting.Path
 		chanCap *capabilitytypes.Capability
-		err     error
 	)
 
 	testCases := []struct {
@@ -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

modules/apps/27-interchain-accounts/keeper/keeper.go

@@ -2,7 +2,6 @@ package keeper
 
 import (
 	"fmt"
-	"strings"
 
 	baseapp "github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/codec"
@@ -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)
 

modules/apps/27-interchain-accounts/keeper/keeper_test.go

@@ -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
 	}
 

modules/apps/27-interchain-accounts/keeper/relay.go

@@ -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(

modules/apps/27-interchain-accounts/types/account.go

@@ -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"
 )
 
+// 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)
+	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
 }

modules/apps/27-interchain-accounts/types/account_test.go

@@ -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)
+			}
+		})
+	}
+}

modules/apps/27-interchain-accounts/types/errors.go

@@ -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")
 )