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

chore: remove unused capabilities methods and clean up tests. #7239

Merged
merged 5 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -3,7 +3,6 @@ package keeper_test
import (
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
ibctesting "github.com/cosmos/ibc-go/v9/testing"
)

Expand All @@ -23,15 +22,6 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount() {
{
"success", func() {}, nil,
},
{
"port is already bound for owner but capability is claimed by another module",
func() {
capability := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), TestPortID)
err := suite.chainA.GetSimApp().TransferKeeper.ClaimCapability(suite.chainA.GetContext(), capability, host.PortPath(TestPortID))
suite.Require().NoError(err)
},
icatypes.ErrPortAlreadyBound,
},
{
"fails to generate port-id",
func() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
connectiontypes "github.com/cosmos/ibc-go/v9/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
ibctesting "github.com/cosmos/ibc-go/v9/testing"
)
Expand Down Expand Up @@ -64,15 +63,6 @@ func (suite *KeeperTestSuite) TestRegisterInterchainAccount_MsgServer() {
},
icatypes.ErrInvalidAccountAddress,
},
{
"port is already bound for owner but capability is claimed by another module",
func() {
capability := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), TestPortID)
err := suite.chainA.GetSimApp().TransferKeeper.ClaimCapability(suite.chainA.GetContext(), capability, host.PortPath(TestPortID))
suite.Require().NoError(err)
},
icatypes.ErrPortAlreadyBound,
},
}

for _, ordering := range []channeltypes.Order{channeltypes.UNORDERED, channeltypes.ORDERED} {
Expand Down
49 changes: 10 additions & 39 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
// ensure channel on chainB is set in state
suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, *channel)

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)

version, err := cbs.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.ConnectionHops,
Expand Down Expand Up @@ -312,10 +309,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() {

tc.malleate()

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)

err = cbs.OnChanOpenConfirm(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
Expand All @@ -341,10 +335,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanCloseInit() {
err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)

err = cbs.OnChanCloseInit(
Expand Down Expand Up @@ -382,10 +373,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanCloseConfirm() {
suite.Require().NoError(err)

tc.malleate() // malleate mutates test data
module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)

err = cbs.OnChanCloseConfirm(
Expand Down Expand Up @@ -493,10 +482,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {

tc.malleate()

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)

ctx := suite.chainB.GetContext()
Expand Down Expand Up @@ -567,10 +553,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)

packet := channeltypes.NewPacket(
Expand Down Expand Up @@ -622,10 +605,7 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointA.ChannelConfig.PortID)

will tests pass?

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 doesn't because the error message is different... though this is one of the test that I refactored so maybe I put the wrong error msg there. I'll add some more details later

suite.Require().True(ok)

packet := channeltypes.NewPacket(
Expand Down Expand Up @@ -663,10 +643,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() {
suite.Require().NoError(err)

// call application callback directly
module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down Expand Up @@ -724,10 +701,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeTry() {

tc.malleate() // malleate mutates test data

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down Expand Up @@ -764,10 +738,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeAck() {
suite.Require().NoError(err)

// call application callback directly
module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)
suite.Require().True(ok)
cbs, ok := app.(porttypes.UpgradableModule)
suite.Require().True(ok)
Expand Down
12 changes: 0 additions & 12 deletions modules/apps/27-interchain-accounts/host/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,12 @@ import (

genesistypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/genesis/types"
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
)

// InitGenesis initializes the interchain accounts host application state from a provided genesis state
func InitGenesis(ctx context.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) {
// 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.Errorf("could not claim port capability: %v", err))
}
}

for _, ch := range state.ActiveChannels {
keeper.SetActiveChannelID(ctx, ch.ConnectionId, ch.PortId, ch.ChannelId)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
ibctesting "github.com/cosmos/ibc-go/v9/testing"
)

Expand Down Expand Up @@ -46,10 +45,6 @@ func (suite *KeeperTestSuite) TestInitGenesis() {

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() {
Expand Down
18 changes: 0 additions & 18 deletions modules/apps/27-interchain-accounts/host/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@ import (
"github.com/cosmos/cosmos-sdk/runtime"
sdk "github.com/cosmos/cosmos-sdk/types"

capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
genesistypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/genesis/types"
"github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v9/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
"github.com/cosmos/ibc-go/v9/modules/core/exported"
)
Expand Down Expand Up @@ -124,22 +122,6 @@ func (k Keeper) setPort(ctx context.Context, portID string) {
}
}

// hasCapability checks if the interchain account host module owns the port capability for the desired port
func (k Keeper) hasCapability(ctx context.Context, portID string) bool {
_, ok := k.scopedKeeper.GetCapability(ctx, host.PortPath(portID))
return ok
}

// AuthenticateCapability wraps the scopedKeeper's AuthenticateCapability function
func (k Keeper) AuthenticateCapability(ctx context.Context, cap *capabilitytypes.Capability, name string) bool {
return k.scopedKeeper.AuthenticateCapability(ctx, cap, name)
}

// ClaimCapability wraps the scopedKeeper's ClaimCapability function
func (k Keeper) ClaimCapability(ctx context.Context, cap *capabilitytypes.Capability, name string) error {
return k.scopedKeeper.ClaimCapability(ctx, cap, name)
}

// GetAppVersion calls the ICS4Wrapper GetAppVersion function.
func (k Keeper) GetAppVersion(ctx context.Context, portID, channelID string) (string, bool) {
return k.ics4Wrapper.GetAppVersion(ctx, portID, channelID)
Expand Down
37 changes: 8 additions & 29 deletions modules/apps/transfer/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,7 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
}
counterpartyVersion = types.V2

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(ibctesting.TransferPort)
suite.Require().True(ok)

tc.malleate() // explicitly change fields in channel and testChannel
Expand Down Expand Up @@ -232,15 +229,12 @@ func (suite *TransferTestSuite) TestOnChanOpenAck() {
path.EndpointA.ChannelID = ibctesting.FirstChannelID
counterpartyVersion = types.V2

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(ibctesting.TransferPort)
suite.Require().True(ok)

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

err = cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointA.Counterparty.ChannelID, counterpartyVersion)
err := cbs.OnChanOpenAck(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointA.Counterparty.ChannelID, counterpartyVersion)

expPass := tc.expError == nil
if expPass {
Expand Down Expand Up @@ -372,10 +366,7 @@ func (suite *TransferTestSuite) TestOnRecvPacket() {
packet = channeltypes.NewPacket(packetData.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.ZeroHeight(), suite.chainA.GetTimeoutTimestamp())

ctx := suite.chainB.GetContext()
module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(ctx, ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(ibctesting.TransferPort)
suite.Require().True(ok)

tc.malleate() // change fields in packet
Expand Down Expand Up @@ -439,10 +430,7 @@ func (suite *TransferTestSuite) TestOnTimeoutPacket() {
"already timed-out packet",
sdk.NewCoins(ibctesting.TestCoin),
func() {
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(ibctesting.TransferPort)
suite.Require().True(ok)

suite.Require().NoError(cbs.OnTimeoutPacket(suite.chainA.GetContext(), path.EndpointA.GetChannel().Version, packet, suite.chainA.SenderAccount.GetAddress()))
Expand Down Expand Up @@ -477,10 +465,7 @@ func (suite *TransferTestSuite) TestOnTimeoutPacket() {
packet, err = ibctesting.ParsePacketFromEvents(res.Events)
suite.Require().NoError(err)

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(ibctesting.TransferPort)
suite.Require().True(ok)

tc.malleate() // change fields in packet
Expand Down Expand Up @@ -627,10 +612,7 @@ func (suite *TransferTestSuite) TestOnChanUpgradeTry() {

tc.malleate()

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), types.PortID)
suite.Require().NoError(err)

app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(types.PortID)
suite.Require().True(ok)

cbs, ok := app.(porttypes.UpgradableModule)
Expand Down Expand Up @@ -698,10 +680,7 @@ func (suite *TransferTestSuite) TestOnChanUpgradeAck() {

tc.malleate()

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID)
suite.Require().NoError(err)

app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
app, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(types.PortID)
suite.Require().True(ok)

cbs, ok := app.(porttypes.UpgradableModule)
Expand Down
13 changes: 0 additions & 13 deletions modules/apps/transfer/keeper/genesis.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package keeper

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v9/modules/apps/transfer/types"
Expand All @@ -17,17 +15,6 @@ func (k Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) {
k.setDenomMetadata(ctx, denom)
}

// Only try to bind to port if it is not already bound, since we may already own
// port capability from capability InitGenesis
if !k.hasCapability(ctx, state.PortId) {
// transfer module binds to the transfer port on InitChain
// and claims the returned capability
err := k.BindPort(ctx, state.PortId)
if err != nil {
panic(fmt.Errorf("could not claim port capability: %v", err))
}
}

k.SetParams(ctx, state.Params)

// Every denom will have only one total escrow amount, since any
Expand Down
18 changes: 0 additions & 18 deletions modules/apps/transfer/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,19 +104,6 @@ func (Keeper) Logger(ctx context.Context) log.Logger {
return sdkCtx.Logger().With("module", "x/"+exported.ModuleName+"-"+types.ModuleName)
}

// hasCapability checks if the transfer module owns the port capability for the desired port
func (k Keeper) hasCapability(ctx context.Context, portID string) bool {
_, ok := k.scopedKeeper.GetCapability(ctx, host.PortPath(portID))
return ok
}

// BindPort defines a wrapper function for the port Keeper's function in
// order to expose it to module's InitGenesis function
func (k Keeper) BindPort(ctx context.Context, portID string) error {
capability := k.portKeeper.BindPort(ctx, portID)
return k.ClaimCapability(ctx, capability, host.PortPath(portID))
}

// GetPort returns the portID for the transfer module. Used in ExportGenesis
func (k Keeper) GetPort(ctx context.Context) string {
store := k.storeService.OpenKVStore(ctx)
Expand Down Expand Up @@ -319,11 +306,6 @@ func (k Keeper) IterateTokensInEscrow(ctx context.Context, storeprefix []byte, c
}
}

// AuthenticateCapability wraps the scopedKeeper's AuthenticateCapability function
func (k Keeper) AuthenticateCapability(ctx context.Context, cap *capabilitytypes.Capability, name string) bool {
return k.scopedKeeper.AuthenticateCapability(ctx, cap, name)
}

// ClaimCapability allows the transfer module that can claim a capability that IBC module
// passes to it
func (k Keeper) ClaimCapability(ctx context.Context, cap *capabilitytypes.Capability, name string) error {
Expand Down
Loading
Loading