Skip to content

Commit 4b3d3b8

Browse files
colin-axnerdamiannolan
colin-axner
and
damiannolan
authored
Merge pull request from GHSA-832c-mq9v-367r
* 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 <[email protected]> Co-authored-by: Damian Nolan <[email protected]>
1 parent 2abfd4f commit 4b3d3b8

File tree

16 files changed

+280
-123
lines changed

16 files changed

+280
-123
lines changed

‎modules/apps/27-interchain-accounts/controller/ibc_module_test.go

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
sdk "github.com/cosmos/cosmos-sdk/types"
88
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
99
"github.com/stretchr/testify/suite"
10-
"github.com/tendermint/tendermint/crypto"
1110

1211
"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types"
1312
icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
@@ -18,12 +17,6 @@ import (
1817
)
1918

2019
var (
21-
// TODO: Cosmos-SDK ADR-28: Update crypto.AddressHash() when sdk uses address.Module()
22-
// https://github.com/cosmos/cosmos-sdk/issues/10225
23-
//
24-
// TestAccAddress defines a resuable bech32 address for testing purposes
25-
TestAccAddress = icatypes.GenerateAddress(sdk.AccAddress(crypto.AddressHash([]byte(icatypes.ModuleName))), ibctesting.FirstConnectionID, TestPortID)
26-
2720
// TestOwnerAddress defines a reusable bech32 address for testing purposes
2821
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
2922

@@ -478,7 +471,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
478471
0,
479472
)
480473

481-
ack := cbs.OnRecvPacket(suite.chainA.GetContext(), packet, TestAccAddress)
474+
ack := cbs.OnRecvPacket(suite.chainA.GetContext(), packet, nil)
482475
suite.Require().Equal(tc.expPass, ack.Success())
483476
})
484477
}

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
func (suite *KeeperTestSuite) TestInitGenesis() {
1111
suite.SetupTest()
1212

13+
interchainAccAddr := icatypes.GenerateUniqueAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, TestPortID)
1314
genesisState := icatypes.ControllerGenesisState{
1415
ActiveChannels: []icatypes.ActiveChannel{
1516
{
@@ -22,7 +23,7 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
2223
{
2324
ConnectionId: ibctesting.FirstConnectionID,
2425
PortId: TestPortID,
25-
AccountAddress: TestAccAddress.String(),
26+
AccountAddress: interchainAccAddr.String(),
2627
},
2728
},
2829
Ports: []string{TestPortID},
@@ -36,7 +37,7 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
3637

3738
accountAdrr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, TestPortID)
3839
suite.Require().True(found)
39-
suite.Require().Equal(TestAccAddress.String(), accountAdrr)
40+
suite.Require().Equal(interchainAccAddr.String(), accountAdrr)
4041

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

56+
interchainAccAddr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
57+
suite.Require().True(exists)
58+
5559
genesisState := keeper.ExportGenesis(suite.chainA.GetContext(), suite.chainA.GetSimApp().ICAControllerKeeper)
5660

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

60-
suite.Require().Equal(TestAccAddress.String(), genesisState.InterchainAccounts[0].AccountAddress)
64+
suite.Require().Equal(interchainAccAddr, genesisState.InterchainAccounts[0].AccountAddress)
6165
suite.Require().Equal(path.EndpointA.ChannelConfig.PortID, genesisState.InterchainAccounts[0].PortId)
6266

6367
suite.Require().Equal([]string{TestPortID}, genesisState.GetPorts())

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
sdk "github.com/cosmos/cosmos-sdk/types"
55

66
"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/controller/types"
7-
icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
87
ibctesting "github.com/cosmos/ibc-go/v3/testing"
98
)
109

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

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

6969
suite.Require().NoError(err)
70-
suite.Require().Equal(expAddress.String(), res.Address)
70+
suite.Require().Equal(expAddress, res.Address)
7171
} else {
7272
suite.Require().Error(err)
7373
}

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

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
2424
}{
2525
{
2626
"success",
27-
func() {
28-
path.EndpointA.SetChannel(*channel)
29-
},
27+
func() {},
3028
true,
3129
},
3230
{
@@ -47,30 +45,44 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
4745
},
4846
true,
4947
},
48+
{
49+
"success: channel reopening",
50+
func() {
51+
err := SetupICAPath(path, TestOwnerAddress)
52+
suite.Require().NoError(err)
53+
54+
err = path.EndpointA.SetChannelClosed()
55+
suite.Require().NoError(err)
56+
57+
err = path.EndpointB.SetChannelClosed()
58+
suite.Require().NoError(err)
59+
60+
path.EndpointA.ChannelID = ""
61+
path.EndpointB.ChannelID = ""
62+
},
63+
true,
64+
},
5065
{
5166
"invalid metadata - previous metadata is different",
5267
func() {
5368
// set active channel to closed
5469
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
5570

71+
// attempt to downgrade version by reinitializing channel with version 1, but setting channel to version 2
72+
metadata.Version = "ics27-2"
73+
74+
versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
75+
suite.Require().NoError(err)
76+
5677
counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
5778
closedChannel := channeltypes.Channel{
5879
State: channeltypes.CLOSED,
5980
Ordering: channeltypes.ORDERED,
6081
Counterparty: counterparty,
6182
ConnectionHops: []string{path.EndpointA.ConnectionID},
62-
Version: TestVersion,
83+
Version: string(versionBytes),
6384
}
64-
6585
path.EndpointA.SetChannel(closedChannel)
66-
67-
// modify metadata
68-
metadata.Version = "ics27-2"
69-
70-
versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
71-
suite.Require().NoError(err)
72-
73-
channel.Version = string(versionBytes)
7486
},
7587
false,
7688
},
@@ -368,7 +380,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() {
368380
err = path.EndpointB.ChanOpenTry()
369381
suite.Require().NoError(err)
370382

371-
metadata = icatypes.NewMetadata(icatypes.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, TestAccAddress.String(), icatypes.EncodingProtobuf, icatypes.TxTypeSDKMultiMsg)
383+
interchainAccAddr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
384+
suite.Require().True(exists)
385+
386+
metadata = icatypes.NewMetadata(icatypes.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, interchainAccAddr, icatypes.EncodingProtobuf, icatypes.TxTypeSDKMultiMsg)
372387
versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
373388
suite.Require().NoError(err)
374389

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

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,22 +3,14 @@ package keeper_test
33
import (
44
"testing"
55

6-
sdk "github.com/cosmos/cosmos-sdk/types"
76
"github.com/stretchr/testify/suite"
8-
"github.com/tendermint/tendermint/crypto"
97

108
icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
119
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
1210
ibctesting "github.com/cosmos/ibc-go/v3/testing"
1311
)
1412

1513
var (
16-
// TODO: Cosmos-SDK ADR-28: Update crypto.AddressHash() when sdk uses address.Module()
17-
// https://github.com/cosmos/cosmos-sdk/issues/10225
18-
//
19-
// TestAccAddress defines a resuable bech32 address for testing purposes
20-
TestAccAddress = icatypes.GenerateAddress(sdk.AccAddress(crypto.AddressHash([]byte(icatypes.ModuleName))), ibctesting.FirstConnectionID, TestPortID)
21-
2214
// TestOwnerAddress defines a reusable bech32 address for testing purposes
2315
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
2416

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

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

158149
retrievedAddr, found := suite.chainA.GetSimApp().ICAControllerKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, counterpartyPortID)
159150
suite.Require().True(found)
160-
suite.Require().Equal(expectedAddr.String(), retrievedAddr)
151+
suite.Require().NotEmpty(retrievedAddr)
161152

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

206+
interchainAccAddr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
207+
suite.Require().True(exists)
208+
215209
suite.chainA.GetSimApp().ICAControllerKeeper.SetInterchainAccountAddress(suite.chainA.GetContext(), ibctesting.FirstConnectionID, expectedPortID, expectedAccAddr)
216210

217211
expectedAccounts := []icatypes.RegisteredInterchainAccount{
218212
{
219213
ConnectionId: ibctesting.FirstConnectionID,
220214
PortId: TestPortID,
221-
AccountAddress: TestAccAddress.String(),
215+
AccountAddress: interchainAccAddr,
222216
},
223217
{
224218
ConnectionId: ibctesting.FirstConnectionID,

‎modules/apps/27-interchain-accounts/host/ibc_module_test.go

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"github.com/gogo/protobuf/proto"
1111
"github.com/stretchr/testify/suite"
1212
abcitypes "github.com/tendermint/tendermint/abci/types"
13-
"github.com/tendermint/tendermint/crypto"
1413
tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state"
1514
tmstate "github.com/tendermint/tendermint/state"
1615

@@ -24,12 +23,6 @@ import (
2423
)
2524

2625
var (
27-
// TODO: Cosmos-SDK ADR-28: Update crypto.AddressHash() when sdk uses address.Module()
28-
// https://github.com/cosmos/cosmos-sdk/issues/10225
29-
//
30-
// TestAccAddress defines a resuable bech32 address for testing purposes
31-
TestAccAddress = icatypes.GenerateAddress(sdk.AccAddress(crypto.AddressHash([]byte(icatypes.ModuleName))), ibctesting.FirstConnectionID, TestPortID)
32-
3326
// TestOwnerAddress defines a reusable bech32 address for testing purposes
3427
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
3528

@@ -151,6 +144,17 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
151144
{
152145
"success", func() {}, true,
153146
},
147+
{
148+
"account address generation is block dependent", func() {
149+
icaHostAccount := icatypes.GenerateUniqueAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
150+
err := suite.chainB.GetSimApp().BankKeeper.SendCoins(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), icaHostAccount, sdk.Coins{sdk.NewCoin("stake", sdk.NewInt(1))})
151+
suite.Require().NoError(err)
152+
suite.Require().True(suite.chainB.GetSimApp().AccountKeeper.HasAccount(suite.chainB.GetContext(), icaHostAccount))
153+
154+
// ensure account registration is simulated in a separate block
155+
suite.coordinator.CommitBlock(suite.chainB)
156+
}, true,
157+
},
154158
{
155159
"host submodule disabled", func() {
156160
suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), types.NewParams(false, []string{}))
@@ -217,6 +221,10 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
217221

218222
if tc.expPass {
219223
suite.Require().NoError(err)
224+
225+
addr, exists := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, counterparty.PortId)
226+
suite.Require().True(exists)
227+
suite.Require().NotNil(addr)
220228
} else {
221229
suite.Require().Error(err)
222230
suite.Require().Equal("", version)
@@ -536,7 +544,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
536544
0,
537545
)
538546

539-
err = cbs.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, []byte("ackBytes"), TestAccAddress)
547+
err = cbs.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, []byte("ackBytes"), nil)
540548

541549
if tc.expPass {
542550
suite.Require().NoError(err)
@@ -589,7 +597,7 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
589597
0,
590598
)
591599

592-
err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, TestAccAddress)
600+
err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, nil)
593601

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

618-
// TestControlAccountAfterChannelClose tests that a controller chain can control a registered interchain account after the currently active channel for that interchain account has been closed
619-
// by opening a new channel on the associated portID
626+
// TestControlAccountAfterChannelClose tests that a controller chain can control a registered interchain account after the currently active channel for that interchain account has been closed.
627+
// A new channel will be opened for the controller portID. The interchain account address should remain unchanged.
620628
func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose() {
621-
// create channel + init interchain account on a particular port
622629
path := NewICAPath(suite.chainA, suite.chainB)
623630
suite.coordinator.SetupConnections(path)
631+
624632
err := SetupICAPath(path, TestOwnerAddress)
625633
suite.Require().NoError(err)
626634

635+
// two sends will be performed, one after initial creation of the account and one after channel closure and reopening
636+
var (
637+
startingBal = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100000)))
638+
tokenAmt = sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(5000)))
639+
expBalAfterFirstSend = startingBal.Sub(tokenAmt)
640+
expBalAfterSecondSend = expBalAfterFirstSend.Sub(tokenAmt)
641+
)
642+
627643
// check that the account is working as expected
628-
suite.fundICAWallet(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(10000))))
629-
interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID)
644+
suite.fundICAWallet(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, startingBal)
645+
interchainAccountAddr, found := suite.chainB.GetSimApp().ICAHostKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointB.ConnectionID, path.EndpointA.ChannelConfig.PortID)
630646
suite.Require().True(found)
631647

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

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

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

693-
// check that the ica balance is updated
694-
hasBalance = suite.chainB.GetSimApp().BankKeeper.HasBalance(suite.chainB.GetContext(), icaAddr, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)})
695-
suite.Require().True(hasBalance)
707+
suite.assertBalance(icaAddr, expBalAfterSecondSend)
708+
}
709+
710+
// assertBalance asserts that the provided address has exactly the expected balance.
711+
// CONTRACT: the expected balance must only contain one coin denom.
712+
func (suite *InterchainAccountsTestSuite) assertBalance(addr sdk.AccAddress, expBalance sdk.Coins) {
713+
balance := suite.chainB.GetSimApp().BankKeeper.GetBalance(suite.chainB.GetContext(), addr, sdk.DefaultBondDenom)
714+
suite.Require().Equal(expBalance[0], balance)
696715
}
697716

698717
// The safety of including SDK MsgResponses in the acknowledgement rests
699718
// on the inclusion of the abcitypes.ResponseDeliverTx.Data in the
719+
700720
// abcitypes.ResposneDeliverTx hash. If the abcitypes.ResponseDeliverTx.Data
701721
// gets removed from consensus they must no longer be used in the packet
702722
// acknowledgement.

0 commit comments

Comments
 (0)