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

Introduce ICA Keeper (deduplicating host/controller code) #565

Closed
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanCloseConfirm() {
err = cbs.OnChanCloseConfirm(
suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

activeChannelID, found := suite.chainA.GetSimApp().ICAHostKeeper.GetActiveChannelID(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
activeChannelID, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetActiveChannelID(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)

if tc.expPass {
suite.Require().NoError(err)
Expand Down
117 changes: 27 additions & 90 deletions modules/apps/27-interchain-accounts/controller/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 All @@ -11,19 +10,20 @@ import (
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
"github.com/tendermint/tendermint/libs/log"

icakeeper "github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/keeper"
"github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types"
host "github.com/cosmos/ibc-go/v2/modules/core/24-host"
)

// Keeper defines the IBC interchain accounts controller keeper
type Keeper struct {
storeKey sdk.StoreKey
cdc codec.BinaryCodec
cdc codec.BinaryCodec
storePrefix string

icaKeeper icakeeper.Keeper
ics4Wrapper types.ICS4Wrapper
channelKeeper types.ChannelKeeper
portKeeper types.PortKeeper
accountKeeper types.AccountKeeper

scopedKeeper capabilitykeeper.ScopedKeeper

Expand All @@ -32,49 +32,25 @@ type Keeper struct {

// NewKeeper creates a new interchain accounts controller Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key sdk.StoreKey,
cdc codec.BinaryCodec, storePrefix string, icaKeeper icakeeper.Keeper,
ics4Wrapper types.ICS4Wrapper, channelKeeper types.ChannelKeeper, portKeeper types.PortKeeper,
accountKeeper types.AccountKeeper, scopedKeeper capabilitykeeper.ScopedKeeper, msgRouter *baseapp.MsgServiceRouter,
scopedKeeper capabilitykeeper.ScopedKeeper, msgRouter *baseapp.MsgServiceRouter,
) Keeper {
return Keeper{
storeKey: key,
cdc: cdc,
storePrefix: storePrefix,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prefix used to create a prefixed store from the ICAKeeper

So, one store, but each ica submodule has its own namespace. This is actual quite useful. Now we can create many controller modules (currently require a new keeper for each)

icaKeeper: icaKeeper,
ics4Wrapper: ics4Wrapper,
channelKeeper: channelKeeper,
portKeeper: portKeeper,
accountKeeper: accountKeeper,
scopedKeeper: scopedKeeper,
msgRouter: msgRouter,
}
}

// Logger returns the application logger, scoped to the associated module
func (k Keeper) Logger(ctx sdk.Context) log.Logger {
return ctx.Logger().With("module", fmt.Sprintf("x/%s-%s", host.ModuleName, types.ModuleName))
}

// GetAllPorts returns all ports to which the interchain accounts controller module is bound. Used in ExportGenesis
func (k Keeper) GetAllPorts(ctx sdk.Context) []string {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, []byte(types.PortKeyPrefix))
defer iterator.Close()

var ports []string
for ; iterator.Valid(); iterator.Next() {
keySplit := strings.Split(string(iterator.Key()), "/")

ports = append(ports, keySplit[1])
}

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 {
store := ctx.KVStore(k.storeKey)
store.Set(types.KeyPort(portID), []byte{0x01})

return k.portKeeper.BindPort(ctx, portID)
return ctx.Logger().With("module", fmt.Sprintf("x/%s-%s", host.ModuleName, types.ControllerModuleName))
}

// IsBound checks if the interchain account controller module is already bound to the desired port
Expand All @@ -93,91 +69,52 @@ func (k Keeper) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability
return k.scopedKeeper.ClaimCapability(ctx, cap, name)
}

// GetActiveChannelID retrieves the active channelID from the store keyed by the provided portID
func (k Keeper) GetActiveChannelID(ctx sdk.Context, portID string) (string, bool) {
store := ctx.KVStore(k.storeKey)
key := types.KeyActiveChannel(portID)
// GetAllPorts returns all ports to which the interchain accounts controller module is bound. Used in ExportGenesis
func (k Keeper) GetAllPorts(ctx sdk.Context) []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.

All duplicate keeper funcs in controller/host are now wrappers

return k.icaKeeper.GetAllPorts(ctx, k.storePrefix)
}

if !store.Has(key) {
return "", false
}
// BindPort stores the provided portID and binds to it, returning the associated capability
func (k Keeper) BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability {
return k.icaKeeper.BindPort(ctx, k.storePrefix, portID)
}

return string(store.Get(key)), true
// GetActiveChannelID retrieves the active channelID from the store keyed by the provided portID
func (k Keeper) GetActiveChannelID(ctx sdk.Context, portID string) (string, bool) {
return k.icaKeeper.GetActiveChannelID(ctx, k.storePrefix, portID)
}

// GetAllActiveChannels returns a list of all active interchain accounts controller channels and their associated port identifiers
func (k Keeper) GetAllActiveChannels(ctx sdk.Context) []types.ActiveChannel {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, []byte(types.ActiveChannelKeyPrefix))
defer iterator.Close()

var activeChannels []types.ActiveChannel
for ; iterator.Valid(); iterator.Next() {
keySplit := strings.Split(string(iterator.Key()), "/")

ch := types.ActiveChannel{
PortId: keySplit[1],
ChannelId: string(iterator.Value()),
}

activeChannels = append(activeChannels, ch)
}

return activeChannels
return k.icaKeeper.GetAllActiveChannels(ctx, k.storePrefix)
}

// SetActiveChannelID stores the active channelID, keyed by the provided portID
func (k Keeper) SetActiveChannelID(ctx sdk.Context, portID, channelID string) {
store := ctx.KVStore(k.storeKey)
store.Set(types.KeyActiveChannel(portID), []byte(channelID))
k.icaKeeper.SetActiveChannelID(ctx, k.storePrefix, portID, channelID)
}

// DeleteActiveChannelID removes the active channel keyed by the provided portID stored in state
func (k Keeper) DeleteActiveChannelID(ctx sdk.Context, portID string) {
store := ctx.KVStore(k.storeKey)
store.Delete(types.KeyActiveChannel(portID))
k.icaKeeper.DeleteActiveChannelID(ctx, k.storePrefix, portID)
}

// IsActiveChannel returns true if there exists an active channel for the provided portID, otherwise false
func (k Keeper) IsActiveChannel(ctx sdk.Context, portID string) bool {
_, ok := k.GetActiveChannelID(ctx, portID)
return ok
return k.icaKeeper.IsActiveChannel(ctx, k.storePrefix, portID)
}

// GetInterchainAccountAddress retrieves the InterchainAccount address from the store keyed by the provided portID
func (k Keeper) GetInterchainAccountAddress(ctx sdk.Context, portID string) (string, bool) {
store := ctx.KVStore(k.storeKey)
key := types.KeyOwnerAccount(portID)

if !store.Has(key) {
return "", false
}

return string(store.Get(key)), true
return k.icaKeeper.GetInterchainAccountAddress(ctx, k.storePrefix, portID)
}

// GetAllInterchainAccounts returns a list of all registered interchain account addresses and their associated controller port identifiers
func (k Keeper) GetAllInterchainAccounts(ctx sdk.Context) []types.RegisteredInterchainAccount {
store := ctx.KVStore(k.storeKey)
iterator := sdk.KVStorePrefixIterator(store, []byte(types.OwnerKeyPrefix))

var interchainAccounts []types.RegisteredInterchainAccount
for ; iterator.Valid(); iterator.Next() {
keySplit := strings.Split(string(iterator.Key()), "/")

acc := types.RegisteredInterchainAccount{
PortId: keySplit[1],
AccountAddress: string(iterator.Value()),
}

interchainAccounts = append(interchainAccounts, acc)
}

return interchainAccounts
return k.icaKeeper.GetAllInterchainAccounts(ctx, k.storePrefix)
}

// SetInterchainAccountAddress stores the InterchainAccount address, keyed by the associated portID
func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, portID string, address string) {
store := ctx.KVStore(k.storeKey)
store.Set(types.KeyOwnerAccount(portID), []byte(address))
k.icaKeeper.SetInterchainAccountAddress(ctx, k.storePrefix, portID, address)
}
Original file line number Diff line number Diff line change
Expand Up @@ -117,22 +117,6 @@ func (suite *KeeperTestSuite) TestIsBound() {
suite.Require().True(isBound)
}

func (suite *KeeperTestSuite) TestGetAllPorts() {
suite.SetupTest()

path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

expectedPorts := []string{TestPortID}

ports := suite.chainA.GetSimApp().ICAControllerKeeper.GetAllPorts(suite.chainA.GetContext())
suite.Require().Len(ports, len(expectedPorts))
suite.Require().Equal(expectedPorts, ports)
}

func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
suite.SetupTest()

Expand All @@ -154,70 +138,6 @@ func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
suite.Require().Empty(retrievedAddr)
}

func (suite *KeeperTestSuite) TestGetAllActiveChannels() {
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 all the GetAll... tests from each controller/host. These have been added to the ica keeper. Testing this functionality at the controller/host level doesn't make sense and it will be tested by the genesis import/export functions

var (
expectedChannelID string = "test-channel"
expectedPortID string = "test-port"
)

suite.SetupTest()

path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), expectedPortID, expectedChannelID)

expectedChannels := []types.ActiveChannel{
{
PortId: TestPortID,
ChannelId: path.EndpointA.ChannelID,
},
{
PortId: expectedPortID,
ChannelId: expectedChannelID,
},
}

activeChannels := suite.chainA.GetSimApp().ICAControllerKeeper.GetAllActiveChannels(suite.chainA.GetContext())
suite.Require().Len(activeChannels, len(expectedChannels))
suite.Require().Equal(expectedChannels, activeChannels)
}

func (suite *KeeperTestSuite) TestGetAllInterchainAccounts() {
var (
expectedAccAddr string = "test-acc-addr"
expectedPortID string = "test-port"
)

suite.SetupTest()

path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

suite.chainA.GetSimApp().ICAControllerKeeper.SetInterchainAccountAddress(suite.chainA.GetContext(), expectedPortID, expectedAccAddr)

expectedAccounts := []types.RegisteredInterchainAccount{
{
PortId: TestPortID,
AccountAddress: TestAccAddress.String(),
},
{
PortId: expectedPortID,
AccountAddress: expectedAccAddr,
},
}

interchainAccounts := suite.chainA.GetSimApp().ICAControllerKeeper.GetAllInterchainAccounts(suite.chainA.GetContext())
suite.Require().Len(interchainAccounts, len(expectedAccounts))
suite.Require().Equal(expectedAccounts, interchainAccounts)
}

func (suite *KeeperTestSuite) TestIsActiveChannel() {
suite.SetupTest()

Expand Down
9 changes: 0 additions & 9 deletions modules/apps/27-interchain-accounts/controller/types/keys.go

This file was deleted.

Loading