refactor: always set ics27 ports regardless of whether the capability…

… exists (cosmos#4640)

* refactor: always set ics27 ports regardless of whether the capability exists

Add e2e to test integration points.
Refactor persisted port state to be separate from port binding/creation

* test: add tests for host genesis logic

* test: add controller genesis test and fixup host test

* lint lint lint

* fix: remove verification assertion in e2e as we have not checked for relayer support across genesis retarts

---------

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

e2e/tests/upgrades/genesis_test.go

@@ -6,14 +6,24 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cosmos/gogoproto/proto"
+	"github.com/strangelove-ventures/interchaintest/v8"
 	cosmos "github.com/strangelove-ventures/interchaintest/v8/chain/cosmos"
 	"github.com/strangelove-ventures/interchaintest/v8/ibc"
 	test "github.com/strangelove-ventures/interchaintest/v8/testutil"
 	"github.com/stretchr/testify/suite"
 	"go.uber.org/zap"
 
+	sdkmath "cosmossdk.io/math"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
+
 	"github.com/cosmos/ibc-go/e2e/testsuite"
 	"github.com/cosmos/ibc-go/e2e/testvalues"
+	controllertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
+	icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
+	ibctesting "github.com/cosmos/ibc-go/v8/testing"
 )
 
 func TestGenesisTestSuite(t *testing.T) {
@@ -55,24 +65,40 @@ func (s *GenesisTestSuite) TestIBCGenesis() {
 
 	s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks")
 
-	t.Run("native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
+	t.Run("ics20: native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
 		transferTxResp := s.Transfer(ctx, chainA, chainAWallet, channelA.PortID, channelA.ChannelID, testvalues.DefaultTransferAmount(chainADenom), chainAAddress, chainBAddress, s.GetTimeoutHeight(ctx, chainB), 0, "")
 		s.AssertTxSuccess(transferTxResp)
 	})
 
-	t.Run("tokens are escrowed", func(t *testing.T) {
+	t.Run("ics20: tokens are escrowed", func(t *testing.T) {
 		actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet)
 		s.Require().NoError(err)
 
 		expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount
 		s.Require().Equal(expected, actualBalance)
 	})
 
+	// setup 2 accounts: controller account on chain A, a second chain B account.
+	// host account will be created when the ICA is registered
+	controllerAccount := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
+	controllerAddress := controllerAccount.FormattedAddress()
+	chainBAccount := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount)
+	var hostAccount string
+
+	t.Run("ics27: broadcast MsgRegisterInterchainAccount", func(t *testing.T) {
+		// explicitly set the version string because we don't want to use incentivized channels.
+		version := icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID)
+		msgRegisterAccount := controllertypes.NewMsgRegisterInterchainAccount(ibctesting.FirstConnectionID, controllerAddress, version)
+
+		txResp := s.BroadcastMessages(ctx, chainA, controllerAccount, msgRegisterAccount)
+		s.AssertTxSuccess(txResp)
+	})
+
 	t.Run("start relayer", func(t *testing.T) {
 		s.StartRelayer(relayer)
 	})
 
-	t.Run("packets are relayed", func(t *testing.T) {
+	t.Run("ics20: packets are relayed", func(t *testing.T) {
 		s.AssertPacketRelayed(ctx, chainA, channelA.PortID, channelA.ChannelID, 1)
 
 		actualBalance, err := chainB.GetBalance(ctx, chainBAddress, chainBIBCToken.IBCDenom())
@@ -82,25 +108,80 @@ func (s *GenesisTestSuite) TestIBCGenesis() {
 		s.Require().Equal(expected, actualBalance.Int64())
 	})
 
+	t.Run("ics27: verify interchain account", func(t *testing.T) {
+		var err error
+		hostAccount, err = s.QueryInterchainAccount(ctx, chainA, controllerAddress, ibctesting.FirstConnectionID)
+		s.Require().NoError(err)
+		s.Require().NotZero(len(hostAccount))
+
+		channels, err := relayer.GetChannels(ctx, s.GetRelayerExecReporter(), chainA.Config().ChainID)
+		s.Require().NoError(err)
+		s.Require().Equal(len(channels), 2)
+	})
+
 	s.Require().NoError(test.WaitForBlocks(ctx, 10, chainA, chainB), "failed to wait for blocks")
 
 	t.Run("Halt chain and export genesis", func(t *testing.T) {
 		s.HaltChainAndExportGenesis(ctx, chainA, relayer, int64(haltHeight))
 	})
 
-	t.Run("native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
+	t.Run("ics20: native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
 		transferTxResp := s.Transfer(ctx, chainA, chainAWallet, channelA.PortID, channelA.ChannelID, testvalues.DefaultTransferAmount(chainADenom), chainAAddress, chainBAddress, s.GetTimeoutHeight(ctx, chainB), 0, "")
 		s.AssertTxSuccess(transferTxResp)
 	})
 
-	t.Run("tokens are escrowed", func(t *testing.T) {
+	t.Run("ics20: tokens are escrowed", func(t *testing.T) {
 		actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet)
 		s.Require().NoError(err)
 
 		expected := testvalues.StartingTokenAmount - 2*testvalues.IBCTransferAmount
 		s.Require().Equal(expected, actualBalance)
 	})
 
+	t.Run("ics27: interchain account executes a bank transfer on behalf of the corresponding owner account", func(t *testing.T) {
+		t.Run("fund interchain account wallet", func(t *testing.T) {
+			// fund the host account so it has some $$ to send
+			err := chainB.SendFunds(ctx, interchaintest.FaucetAccountKeyName, ibc.WalletAmount{
+				Address: hostAccount,
+				Amount:  sdkmath.NewInt(testvalues.StartingTokenAmount),
+				Denom:   chainB.Config().Denom,
+			})
+			s.Require().NoError(err)
+		})
+
+		t.Run("broadcast MsgSendTx", func(t *testing.T) {
+			// assemble bank transfer message from host account to user account on host chain
+			msgSend := &banktypes.MsgSend{
+				FromAddress: hostAccount,
+				ToAddress:   chainBAccount.FormattedAddress(),
+				Amount:      sdk.NewCoins(testvalues.DefaultTransferAmount(chainB.Config().Denom)),
+			}
+
+			cdc := testsuite.Codec()
+			bz, err := icatypes.SerializeCosmosTx(cdc, []proto.Message{msgSend}, icatypes.EncodingProtobuf)
+			s.Require().NoError(err)
+
+			packetData := icatypes.InterchainAccountPacketData{
+				Type: icatypes.EXECUTE_TX,
+				Data: bz,
+				Memo: "e2e",
+			}
+
+			msgSendTx := controllertypes.NewMsgSendTx(controllerAccount.FormattedAddress(), ibctesting.FirstConnectionID, uint64(time.Hour.Nanoseconds()), packetData)
+
+			resp := s.BroadcastMessages(
+				ctx,
+				chainA,
+				controllerAccount,
+				msgSendTx,
+			)
+
+			s.AssertTxSuccess(resp)
+
+			s.Require().NoError(test.WaitForBlocks(ctx, 10, chainA, chainB))
+		})
+	})
+
 	s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")
 }
 

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

@@ -62,7 +62,8 @@ func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, portID,
 	case k.portKeeper.IsBound(ctx, portID) && !k.hasCapability(ctx, portID):
 		return "", errorsmod.Wrapf(icatypes.ErrPortAlreadyBound, "another module has claimed capability for and bound port with portID: %s", portID)
 	case !k.portKeeper.IsBound(ctx, portID):
-		capability := k.BindPort(ctx, portID)
+		k.setPort(ctx, portID)
+		capability := k.portKeeper.BindPort(ctx, portID)
 		if err := k.ClaimCapability(ctx, capability, host.PortPath(portID)); err != nil {
 			return "", errorsmod.Wrapf(err, "unable to bind to newly generated portID: %s", portID)
 		}

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

@@ -12,8 +12,14 @@ import (
 // InitGenesis initializes the interchain accounts controller application state from a provided genesis state
 func InitGenesis(ctx sdk.Context, keeper Keeper, state genesistypes.ControllerGenesisState) {
 	for _, portID := range state.Ports {
+		keeper.setPort(ctx, portID)
+
+		// generate port capability if it does not already exist
 		if !keeper.hasCapability(ctx, portID) {
-			capability := keeper.BindPort(ctx, portID)
+			// use the port keeper to generate a new capability
+			capability := keeper.portKeeper.BindPort(ctx, portID)
+
+			// use the controller scoped keeper to claim the port capability
 			if err := keeper.ClaimCapability(ctx, capability, host.PortPath(portID)); err != nil {
 				panic(fmt.Sprintf("could not claim port capability: %v", err))
 			}

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

@@ -5,11 +5,29 @@ import (
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
 	genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types"
 	icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
+	host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
 	ibctesting "github.com/cosmos/ibc-go/v8/testing"
 )
 
 func (suite *KeeperTestSuite) TestInitGenesis() {
-	suite.SetupTest()
+	ports := []string{"port1", "port2", "port3"}
+
+	testCases := []struct {
+		name     string
+		malleate func()
+	}{
+		{
+			"success", func() {},
+		},
+		{
+			"success: capabilities already initialized for first port",
+			func() {
+				capability := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), ports[0])
+				err := suite.chainA.GetSimApp().ICAControllerKeeper.ClaimCapability(suite.chainA.GetContext(), capability, host.PortPath(ports[0]))
+				suite.Require().NoError(err)
+			},
+		},
+	}
 
 	interchainAccAddr := icatypes.GenerateAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, TestPortID)
 	genesisState := genesistypes.ControllerGenesisState{
@@ -34,28 +52,46 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
 				AccountAddress: interchainAccAddr.String(),
 			},
 		},
-		Ports: []string{TestPortID},
+		Ports: ports,
 	}
+	for _, tc := range testCases {
+		tc := tc
 
-	keeper.InitGenesis(suite.chainA.GetContext(), suite.chainA.GetSimApp().ICAControllerKeeper, genesisState)
+		suite.Run(tc.name, func() {
+			suite.SetupTest()
 
-	channelID, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
-	suite.Require().True(found)
-	suite.Require().Equal(ibctesting.FirstChannelID, channelID)
+			tc.malleate()
 
-	isMiddlewareEnabled := suite.chainA.GetSimApp().ICAControllerKeeper.IsMiddlewareEnabled(suite.chainA.GetContext(), TestPortID, ibctesting.FirstConnectionID)
-	suite.Require().True(isMiddlewareEnabled)
+			keeper.InitGenesis(suite.chainA.GetContext(), suite.chainA.GetSimApp().ICAControllerKeeper, genesisState)
 
-	isMiddlewareDisabled := suite.chainA.GetSimApp().ICAControllerKeeper.IsMiddlewareDisabled(suite.chainA.GetContext(), "test-port-1", "connection-1")
-	suite.Require().True(isMiddlewareDisabled)
+			channelID, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
+			suite.Require().True(found)
+			suite.Require().Equal(ibctesting.FirstChannelID, channelID)
 
-	accountAdrr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
-	suite.Require().True(found)
-	suite.Require().Equal(interchainAccAddr.String(), accountAdrr)
+			isMiddlewareEnabled := suite.chainA.GetSimApp().ICAControllerKeeper.IsMiddlewareEnabled(suite.chainA.GetContext(), TestPortID, ibctesting.FirstConnectionID)
+			suite.Require().True(isMiddlewareEnabled)
 
-	expParams := types.NewParams(false)
-	params := suite.chainA.GetSimApp().ICAControllerKeeper.GetParams(suite.chainA.GetContext())
-	suite.Require().Equal(expParams, params)
+			isMiddlewareDisabled := suite.chainA.GetSimApp().ICAControllerKeeper.IsMiddlewareDisabled(suite.chainA.GetContext(), "test-port-1", "connection-1")
+			suite.Require().True(isMiddlewareDisabled)
+
+			accountAdrr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
+			suite.Require().True(found)
+			suite.Require().Equal(interchainAccAddr.String(), accountAdrr)
+
+			expParams := types.NewParams(false)
+			params := suite.chainA.GetSimApp().ICAControllerKeeper.GetParams(suite.chainA.GetContext())
+			suite.Require().Equal(expParams, params)
+
+			for _, port := range ports {
+				store := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetKey(types.StoreKey))
+				suite.Require().True(store.Has(icatypes.KeyPort(port)))
+
+				capability, found := suite.chainA.GetSimApp().ScopedICAControllerKeeper.GetCapability(suite.chainA.GetContext(), host.PortPath(port))
+				suite.Require().True(found)
+				suite.Require().NotNil(capability)
+			}
+		})
+	}
 }
 
 func (suite *KeeperTestSuite) TestExportGenesis() {

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

@@ -102,12 +102,10 @@ func (k Keeper) GetAllPorts(ctx sdk.Context) []string {
 	return ports
 }
 
-// BindPort stores the provided portID and binds to it, returning the associated capability
-func (k Keeper) BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability {
+// setPort sets the provided portID in state
+func (k Keeper) setPort(ctx sdk.Context, portID string) {
 	store := ctx.KVStore(k.storeKey)
 	store.Set(icatypes.KeyPort(portID), []byte{0x01})
-
-	return k.portKeeper.BindPort(ctx, portID)
 }
 
 // hasCapability checks if the interchain account controller module owns the port capability for the desired port

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

@@ -12,8 +12,14 @@ import (
 
 // InitGenesis initializes the interchain accounts host application state from a provided genesis state
 func InitGenesis(ctx sdk.Context, keeper Keeper, state genesistypes.HostGenesisState) {
+	keeper.setPort(ctx, state.Port)
+
+	// generate port capability if it does not already exist
 	if !keeper.hasCapability(ctx, state.Port) {
-		capability := keeper.BindPort(ctx, state.Port)
+		// use the port keeper to generate a new capability
+		capability := keeper.portKeeper.BindPort(ctx, state.Port)
+
+		// use the host scoped keeper to claim the port capability
 		if err := keeper.ClaimCapability(ctx, capability, host.PortPath(state.Port)); err != nil {
 			panic(fmt.Sprintf("could not claim port capability: %v", err))
 		}

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

@@ -5,14 +5,12 @@ import (
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/keeper"
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
+	host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
 	ibctesting "github.com/cosmos/ibc-go/v8/testing"
 )
 
 func (suite *KeeperTestSuite) TestInitGenesis() {
-	suite.SetupTest()
-
 	interchainAccAddr := icatypes.GenerateAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, TestPortID)
-
 	genesisState := genesistypes.HostGenesisState{
 		ActiveChannels: []genesistypes.ActiveChannel{
 			{
@@ -44,6 +42,13 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
 	expParams := genesisState.GetParams()
 	params := suite.chainA.GetSimApp().ICAHostKeeper.GetParams(suite.chainA.GetContext())
 	suite.Require().Equal(expParams, params)
+
+	store := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetKey(types.StoreKey))
+	suite.Require().True(store.Has(icatypes.KeyPort(icatypes.HostPortID)))
+
+	capability, found := suite.chainA.GetSimApp().ScopedICAHostKeeper.GetCapability(suite.chainA.GetContext(), host.PortPath(icatypes.HostPortID))
+	suite.Require().True(found)
+	suite.Require().NotNil(capability)
 }
 
 func (suite *KeeperTestSuite) TestGenesisParams() {

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

@@ -85,12 +85,10 @@ func (Keeper) Logger(ctx sdk.Context) log.Logger {
 	return ctx.Logger().With("module", fmt.Sprintf("x/%s-%s", exported.ModuleName, icatypes.ModuleName))
 }
 
-// BindPort stores the provided portID and binds to it, returning the associated capability
-func (k Keeper) BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability {
+// setPort sets the provided portID in state.
+func (k Keeper) setPort(ctx sdk.Context, portID string) {
 	store := ctx.KVStore(k.storeKey)
 	store.Set(icatypes.KeyPort(portID), []byte{0x01})
-
-	return k.portKeeper.BindPort(ctx, portID)
 }
 
 // hasCapability checks if the interchain account host module owns the port capability for the desired port

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

@@ -27,7 +27,6 @@ import (
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/simulation"
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
 	porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types"
-	ibchost "github.com/cosmos/ibc-go/v8/modules/core/24-host"
 )
 
 var (
@@ -126,19 +125,20 @@ func NewAppModule(controllerKeeper *controllerkeeper.Keeper, hostKeeper *hostkee
 // called once and as an alternative to InitGenesis.
 func (am AppModule) InitModule(ctx sdk.Context, controllerParams controllertypes.Params, hostParams hosttypes.Params) {
 	if am.controllerKeeper != nil {
-		am.controllerKeeper.SetParams(ctx, controllerParams)
+		controllerkeeper.InitGenesis(ctx, *am.controllerKeeper, genesistypes.ControllerGenesisState{
+			Params: controllerParams,
+		})
 	}
 
 	if am.hostKeeper != nil {
 		if err := hostParams.Validate(); err != nil {
 			panic(fmt.Sprintf("could not set ica host params at initialization: %v", err))
 		}
-		am.hostKeeper.SetParams(ctx, hostParams)
 
-		capability := am.hostKeeper.BindPort(ctx, types.HostPortID)
-		if err := am.hostKeeper.ClaimCapability(ctx, capability, ibchost.PortPath(types.HostPortID)); err != nil {
-			panic(fmt.Sprintf("could not claim port capability: %v", err))
-		}
+		hostkeeper.InitGenesis(ctx, *am.hostKeeper, genesistypes.HostGenesisState{
+			Params: hostParams,
+			Port:   types.HostPortID,
+		})
 	}
 }