Skip to content

Commit

Permalink
Merge pull request from GHSA-832c-mq9v-367r
Browse files Browse the repository at this point in the history 
* fix: use block app hash and tx list to generate interchain account address

Generate interchain account addresses using host connection ID, controller PortID, block app hash, and block data hash
Update tests to handle non-determinstic address creation
Add test case to ensure address generation is block dependent

* fix: return error on existing non-interchainaccounts for generated address

If an account exists and is not an interchain account return an error
Add test cases for existing accounts, both interchain and non interchain account
Refactor account tests to be table tests

* fix: refactor handshake code to account for block dependent address generation

* add more test cases, update error messaging

* self review fix

* increase test readability

* remove msg_server_test.go

* fix API breaking changes

* self nit

* fix tests

* fix naming GenerateAddress naming

* add test cases for controller side channel reopening

* fix cherry-pick conflict

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

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

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
  • Loading branch information
@colin-axner @damiannolan @dudong2
2 people authored and dudong2 committed Jan 19, 2023
1 parent fed1d76 commit 74ae18c
Show file tree
Hide file tree
Showing 16 changed files with 280 additions and 122 deletions.
8 changes: 1 addition & 7 deletions modules/apps/27-interchain-accounts/controller/ibc_module_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@ import (
)

var (
// TODO: Cosmos-SDK ADR-28: Update crypto.AddressHash() when sdk uses address.Module()
// https://github.com/cosmos/cosmos-sdk/issues/10225
//
// TestAccAddress defines a resuable bech32 address for testing purposes
TestAccAddress = icatypes.GenerateAddress(sdk.AccAddress(crypto.AddressHash([]byte(icatypes.ModuleName))), ibctesting.FirstConnectionID, TestPortID)

// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "link17dtl0mjt3t77kpuhg2edqzjpszulwhgzfu9afc"

Expand Down Expand Up @@ -478,7 +472,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
0,
)

ack := cbs.OnRecvPacket(suite.chainA.GetContext(), packet, TestAccAddress)
ack := cbs.OnRecvPacket(suite.chainA.GetContext(), packet, nil)
suite.Require().Equal(tc.expPass, ack.Success())
})
}
Expand Down
10 changes: 7 additions & 3 deletions modules/apps/27-interchain-accounts/controller/keeper/genesis_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
func (suite *KeeperTestSuite) TestInitGenesis() {
suite.SetupTest()

interchainAccAddr := icatypes.GenerateUniqueAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, TestPortID)
genesisState := icatypes.ControllerGenesisState{
ActiveChannels: []icatypes.ActiveChannel{
{
Expand All @@ -22,7 +23,7 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
{
ConnectionId: ibctesting.FirstConnectionID,
PortId: TestPortID,
AccountAddress: TestAccAddress.String(),
AccountAddress: interchainAccAddr.String(),
},
},
Ports: []string{TestPortID},
Expand All @@ -36,7 +37,7 @@ func (suite *KeeperTestSuite) TestInitGenesis() {

accountAdrr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
suite.Require().True(found)
suite.Require().Equal(TestAccAddress.String(), accountAdrr)
suite.Require().Equal(interchainAccAddr.String(), accountAdrr)

expParams := types.NewParams(false)
params := suite.chainA.GetSimApp().ICAControllerKeeper.GetParams(suite.chainA.GetContext())
Expand All @@ -52,12 +53,15 @@ func (suite *KeeperTestSuite) TestExportGenesis() {
err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

interchainAccAddr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(exists)

genesisState := keeper.ExportGenesis(suite.chainA.GetContext(), suite.chainA.GetSimApp().ICAControllerKeeper)

suite.Require().Equal(path.EndpointA.ChannelID, genesisState.ActiveChannels[0].ChannelId)
suite.Require().Equal(path.EndpointA.ChannelConfig.PortID, genesisState.ActiveChannels[0].PortId)

suite.Require().Equal(TestAccAddress.String(), genesisState.InterchainAccounts[0].AccountAddress)
suite.Require().Equal(interchainAccAddr, genesisState.InterchainAccounts[0].AccountAddress)
suite.Require().Equal(path.EndpointA.ChannelConfig.PortID, genesisState.InterchainAccounts[0].PortId)

suite.Require().Equal([]string{TestPortID}, genesisState.GetPorts())
Expand Down
6 changes: 3 additions & 3 deletions modules/apps/27-interchain-accounts/controller/keeper/grpc_query_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
sdk "github.com/line/lbm-sdk/types"

"github.com/line/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types"
icatypes "github.com/line/ibc-go/v3/modules/apps/27-interchain-accounts/types"
ibctesting "github.com/line/ibc-go/v3/testing"
)

Expand Down Expand Up @@ -64,10 +63,11 @@ func (suite *KeeperTestSuite) TestQueryInterchainAccount() {
res, err := suite.chainA.GetSimApp().ICAControllerKeeper.InterchainAccount(sdk.WrapSDKContext(suite.chainA.GetContext()), req)

if tc.expPass {
expAddress := icatypes.GenerateAddress(suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(icatypes.ModuleName), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
expAddress, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(exists)

suite.Require().NoError(err)
suite.Require().Equal(expAddress.String(), res.Address)
suite.Require().Equal(expAddress, res.Address)
} else {
suite.Require().Error(err)
}
Expand Down
43 changes: 29 additions & 14 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
}{
{
"success",
func() {
path.EndpointA.SetChannel(*channel)
},
func() {},
true,
},
{
Expand All @@ -47,30 +45,44 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
},
true,
},
{
"success: channel reopening",
func() {
err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

err = path.EndpointA.SetChannelClosed()
suite.Require().NoError(err)

err = path.EndpointB.SetChannelClosed()
suite.Require().NoError(err)

path.EndpointA.ChannelID = ""
path.EndpointB.ChannelID = ""
},
true,
},
{
"invalid metadata - previous metadata is different",
func() {
// set active channel to closed
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

// attempt to downgrade version by reinitializing channel with version 1, but setting channel to version 2
metadata.Version = "ics27-2"

versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)

counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
closedChannel := channeltypes.Channel{
State: channeltypes.CLOSED,
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointA.ConnectionID},
Version: TestVersion,
Version: string(versionBytes),
}

path.EndpointA.SetChannel(closedChannel)

// modify metadata
metadata.Version = "ics27-2"

versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)

channel.Version = string(versionBytes)
},
false,
},
Expand Down Expand Up @@ -368,7 +380,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() {
err = path.EndpointB.ChanOpenTry()
suite.Require().NoError(err)

metadata = icatypes.NewMetadata(icatypes.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, TestAccAddress.String(), icatypes.EncodingProtobuf, icatypes.TxTypeSDKMultiMsg)
interchainAccAddr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(exists)

metadata = icatypes.NewMetadata(icatypes.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, interchainAccAddr, icatypes.EncodingProtobuf, icatypes.TxTypeSDKMultiMsg)
versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)

Expand Down
16 changes: 5 additions & 11 deletions modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package keeper_test
import (
"testing"

sdk "github.com/line/lbm-sdk/types"
"github.com/line/ostracon/crypto"
"github.com/stretchr/testify/suite"

icatypes "github.com/line/ibc-go/v3/modules/apps/27-interchain-accounts/types"
Expand All @@ -13,12 +11,6 @@ import (
)

var (
// TODO: Cosmos-SDK ADR-28: Update crypto.AddressHash() when sdk uses address.Module()
// https://github.com/cosmos/cosmos-sdk/issues/10225
//
// TestAccAddress defines a resuable bech32 address for testing purposes
TestAccAddress = icatypes.GenerateAddress(sdk.AccAddress(crypto.AddressHash([]byte(icatypes.ModuleName))), ibctesting.FirstConnectionID, TestPortID)

// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "link17dtl0mjt3t77kpuhg2edqzjpszulwhgzfu9afc"

Expand Down Expand Up @@ -153,11 +145,10 @@ func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
suite.Require().NoError(err)

counterpartyPortID := path.EndpointA.ChannelConfig.PortID
expectedAddr := icatypes.GenerateAddress(suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(icatypes.ModuleName), ibctesting.FirstConnectionID, counterpartyPortID)

retrievedAddr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, counterpartyPortID)
suite.Require().True(found)
suite.Require().Equal(expectedAddr.String(), retrievedAddr)
suite.Require().NotEmpty(retrievedAddr)

retrievedAddr, found = suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), "invalid conn", "invalid port")
suite.Require().False(found)
Expand Down Expand Up @@ -212,13 +203,16 @@ func (suite *KeeperTestSuite) TestGetAllInterchainAccounts() {
err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

interchainAccAddr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(exists)

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

expectedAccounts := []icatypes.RegisteredInterchainAccount{
{
ConnectionId: ibctesting.FirstConnectionID,
PortId: TestPortID,
AccountAddress: TestAccAddress.String(),
AccountAddress: interchainAccAddr,
},
{
ConnectionId: ibctesting.FirstConnectionID,
Expand Down
60 changes: 40 additions & 20 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
banktypes "github.com/line/lbm-sdk/x/bank/types"
capabilitytypes "github.com/line/lbm-sdk/x/capability/types"
abcitypes "github.com/line/ostracon/abci/types"
"github.com/line/ostracon/crypto"
ocprotostate "github.com/line/ostracon/proto/ostracon/state"
ocstate "github.com/line/ostracon/state"
"github.com/stretchr/testify/suite"
Expand All @@ -24,12 +23,6 @@ import (
)

var (
// TODO: Cosmos-SDK ADR-28: Update crypto.AddressHash() when sdk uses address.Module()
// https://github.com/cosmos/cosmos-sdk/issues/10225
//
// TestAccAddress defines a resuable bech32 address for testing purposes
TestAccAddress = icatypes.GenerateAddress(sdk.AccAddress(crypto.AddressHash([]byte(icatypes.ModuleName))), ibctesting.FirstConnectionID, TestPortID)

// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "link17dtl0mjt3t77kpuhg2edqzjpszulwhgzfu9afc"

Expand Down Expand Up @@ -151,6 +144,17 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
{
"success", func() {}, true,
},
{
"account address generation is block dependent", func() {
icaHostAccount := icatypes.GenerateUniqueAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
err := suite.chainB.GetSimApp().BankKeeper.SendCoins(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), icaHostAccount, sdk.Coins{sdk.NewCoin("stake", sdk.NewInt(1))})
suite.Require().NoError(err)
suite.Require().True(suite.chainB.GetSimApp().AccountKeeper.HasAccount(suite.chainB.GetContext(), icaHostAccount))

// ensure account registration is simulated in a separate block
suite.coordinator.CommitBlock(suite.chainB)
}, true,
},
{
"host submodule disabled", func() {
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
Expand Down Expand Up @@ -217,6 +221,10 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {

if tc.expPass {
suite.Require().NoError(err)

addr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, counterparty.PortId)
suite.Require().True(exists)
suite.Require().NotNil(addr)
} else {
suite.Require().Error(err)
suite.Require().Equal("", version)
Expand Down Expand Up @@ -536,7 +544,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
0,
)

err = cbs.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, []byte("ackBytes"), TestAccAddress)
err = cbs.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, []byte("ackBytes"), nil)

if tc.expPass {
suite.Require().NoError(err)
Expand Down Expand Up @@ -589,7 +597,7 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
0,
)

err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, TestAccAddress)
err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, nil)

if tc.expPass {
suite.Require().NoError(err)
Expand All @@ -615,21 +623,28 @@ func (suite *InterchainAccountsTestSuite) fundICAWallet(ctx sdk.Context, portID
suite.Require().NoError(err)
}

// TestControlAccountAfterChannelClose tests that a controller chain can control a registered interchain account after the currently active channel for that interchain account has been closed
// by opening a new channel on the associated portID
// TestControlAccountAfterChannelClose tests that a controller chain can control a registered interchain account after the currently active channel for that interchain account has been closed.
// A new channel will be opened for the controller portID. The interchain account address should remain unchanged.
func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose() {
// create channel + init interchain account on a particular port
path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

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

// two sends will be performed, one after initial creation of the account and one after channel closure and reopening
var (
startingBal = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100000)))
tokenAmt = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(5000)))
expBalAfterFirstSend = startingBal.Sub(tokenAmt)
expBalAfterSecondSend = expBalAfterFirstSend.Sub(tokenAmt)
)

// check that the account is working as expected
suite.fundICAWallet(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(10000))))
interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.fundICAWallet(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, startingBal)
interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
suite.Require().True(found)

tokenAmt := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(5000)))
msg := &banktypes.MsgSend{
FromAddress: interchainAccountAddr,
ToAddress: suite.chainB.SenderAccount.GetAddress().String(),
Expand Down Expand Up @@ -663,8 +678,7 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose()
icaAddr, err := sdk.AccAddressFromBech32(interchainAccountAddr)
suite.Require().NoError(err)

hasBalance := suite.chainB.GetSimApp().BankKeeper.HasBalance(suite.chainB.GetContext(), icaAddr, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(5000)})
suite.Require().True(hasBalance)
suite.assertBalance(icaAddr, expBalAfterFirstSend)

// close the channel
err = path.EndpointA.SetChannelClosed()
Expand All @@ -690,13 +704,19 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose()
err = path.RelayPacket(packetRelay)
suite.Require().NoError(err) // relay committed

// check that the ica balance is updated
hasBalance = suite.chainB.GetSimApp().BankKeeper.HasBalance(suite.chainB.GetContext(), icaAddr, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)})
suite.Require().True(hasBalance)
suite.assertBalance(icaAddr, expBalAfterSecondSend)
}

// assertBalance asserts that the provided address has exactly the expected balance.
// CONTRACT: the expected balance must only contain one coin denom.
func (suite *InterchainAccountsTestSuite) assertBalance(addr sdk.AccAddress, expBalance sdk.Coins) {
balance := suite.chainB.GetSimApp().BankKeeper.GetBalance(suite.chainB.GetContext(), addr, sdk.DefaultBondDenom)
suite.Require().Equal(expBalance[0], balance)
}

// The safety of including SDK MsgResponses in the acknowledgement rests
// on the inclusion of the abcitypes.ResponseDeliverTx.Data in the

// abcitypes.ResposneDeliverTx hash. If the abcitypes.ResponseDeliverTx.Data
// gets removed from consensus they must no longer be used in the packet
// acknowledgement.
Expand Down
25 changes: 25 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/account.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper

import (
sdk "github.com/line/lbm-sdk/types"
sdkerrors "github.com/line/lbm-sdk/types/errors"
authtypes "github.com/line/lbm-sdk/x/auth/types"

icatypes "github.com/line/ibc-go/v3/modules/apps/27-interchain-accounts/types"
Expand All @@ -10,6 +11,7 @@ import (
// RegisterInterchainAccount attempts to create a new account using the provided address and
// stores it in state keyed by the provided connection and port identifiers
// If an account for the provided address already exists this function returns early (no-op)
// NOTE: This function is deprecated!
func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, controllerPortID string, accAddress sdk.AccAddress) {
if acc := k.accountKeeper.GetAccount(ctx, accAddress); acc != nil {
return
Expand All @@ -25,3 +27,26 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, control

k.SetInterchainAccountAddress(ctx, connectionID, controllerPortID, interchainAccount.Address)
}

// createInterchainAccount creates a new interchain account. An address is generated using the host connectionID, the controller portID,
// and block dependent information. An error is returned if an account already exists for the generated account.
// An interchain account type is set in the account keeper and the interchain account address mapping is updated.
func (k Keeper) createInterchainAccount(ctx sdk.Context, connectionID, controllerPortID string) (sdk.AccAddress, error) {
accAddress := icatypes.GenerateUniqueAddress(ctx, connectionID, controllerPortID)

if acc := k.accountKeeper.GetAccount(ctx, accAddress); acc != nil {
return nil, sdkerrors.Wrapf(icatypes.ErrAccountAlreadyExist, "existing account for newly generated interchain account address %s", accAddress)
}

interchainAccount := icatypes.NewInterchainAccount(
authtypes.NewBaseAccountWithAddress(accAddress),
controllerPortID,
)

k.accountKeeper.NewAccount(ctx, interchainAccount)
k.accountKeeper.SetAccount(ctx, interchainAccount)

k.SetInterchainAccountAddress(ctx, connectionID, controllerPortID, interchainAccount.Address)

return accAddress, nil
}

0 comments on commit 74ae18c

Please sign in to comment.