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: e2e for ICA reopening #2720

Merged
merged 26 commits into from
Nov 14, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
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
2 changes: 2 additions & 0 deletions .github/workflows/e2e-manual-icad.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ on:
- v0.3.4
- v0.2.3
- v0.1.4
- v0.1.5
chain-b-tag:
default: master
description: 'The tag to use for chain B'
Expand All @@ -42,6 +43,7 @@ on:
- v0.3.4
- v0.2.3
- v0.1.4
- v0.1.5
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the tag with the 1-minute packet timeout, right? Maybe we don't need to use it anymore, if we plan to test the timeout/reopen scenario only with simd >= 6.0.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is manual only, I don't think there's much harm in having this additional drop down option. I'm happy to have it removed though if people don't think it will ever be used!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have to check, but I think v0.1.5 is basically the same as v0.1.4 but with a shorter timeout, so I think it doesn't really add much to have both...

Copy link
Contributor

Choose a reason for hiding this comment

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

okay sounds good to me to remove it 👍

relayer-tag:
description: 'The tag to use for the relayer'
required: true
Expand Down
184 changes: 184 additions & 0 deletions e2e/tests/interchain_accounts/base_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"time"

ibctest "github.com/strangelove-ventures/ibctest/v6"
"github.com/strangelove-ventures/ibctest/v6/chain/cosmos"
"github.com/strangelove-ventures/ibctest/v6/ibc"
"github.com/strangelove-ventures/ibctest/v6/test"
"github.com/stretchr/testify/suite"
Expand All @@ -21,6 +22,7 @@ import (

controllertypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/controller/types"
icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
ibctesting "github.com/cosmos/ibc-go/v6/testing"
)

Expand All @@ -44,6 +46,14 @@ func getICAVersion(chainAVersion, chainBVersion string) string {
return icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID)
}

// RegisterInterchainAccount will attempt to register an interchain account on the counterparty chain.
func (s *InterchainAccountsTestSuite) RegisterInterchainAccount(ctx context.Context, chain *cosmos.CosmosChain, user *ibc.Wallet, msgRegisterAccount *controllertypes.MsgRegisterInterchainAccount) error {
txResp, err := s.BroadcastMessages(ctx, chain, user, msgRegisterAccount)
s.Require().NoError(err)
s.AssertValidTxResponse(txResp)
return err
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

func (s *InterchainAccountsTestSuite) TestMsgSendTx_SuccessfulTransfer() {
t := s.T()
ctx := context.TODO()
Expand Down Expand Up @@ -228,3 +238,177 @@ func (s *InterchainAccountsTestSuite) TestMsgSendTx_FailedTransfer_InsufficientF
})
})
}

func (s *InterchainAccountsTestSuite) TestMsgSubmitTx_SuccessfulTransfer_AfterReopeningICA() {
t := s.T()
ctx := context.TODO()

// setup relayers and connection-0 between two chains
// channel-0 is a transfer channel but it will not be used in this test case
relayer, _ := s.SetupChainsRelayerAndChannel(ctx)
chainA, chainB := s.GetChains()

// 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)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
chainBAccount := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount)

var (
portID string
hostAccount string
)

t.Run("register interchain account", func(t *testing.T) {
version := getICAVersion(testconfig.GetChainATag(), testconfig.GetChainBTag())
msgRegisterInterchainAccount := controllertypes.NewMsgRegisterInterchainAccount(ibctesting.FirstConnectionID, controllerAccount.Bech32Address(chainA.Config().Bech32Prefix), version)
err := s.RegisterInterchainAccount(ctx, chainA, controllerAccount, msgRegisterInterchainAccount)
s.Require().NoError(err)

portID, err = icatypes.NewControllerPortID(controllerAccount.Bech32Address(chainA.Config().Bech32Prefix))
s.Require().NoError(err)
})

t.Run("start relayer", func(t *testing.T) {
s.StartRelayer(relayer)
})

t.Run("verify interchain account", func(t *testing.T) {
var err error
hostAccount, err = s.QueryInterchainAccount(ctx, chainA, controllerAccount.Bech32Address(chainA.Config().Bech32Prefix), 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)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
})

// stop the relayer to let the submit tx message time out
t.Run("stop relayer", func(t *testing.T) {
s.StopRelayer(ctx, relayer)
})

t.Run("submit tx message with bank transfer message times out", func(t *testing.T) {
t.Run("fund interchain account wallet", func(t *testing.T) {
// fund the host account account so it has some $$ to send
err := chainB.SendFunds(ctx, ibctest.FaucetAccountKeyName, ibc.WalletAmount{
Address: hostAccount,
Amount: testvalues.StartingTokenAmount,
Denom: chainB.Config().Denom,
})
s.Require().NoError(err)
})

t.Run("broadcast MsgSendTx", func(t *testing.T) {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// assemble bank transfer message from host account to user account on host chain
msgSend := &banktypes.MsgSend{
FromAddress: hostAccount,
ToAddress: chainBAccount.Bech32Address(chainB.Config().Bech32Prefix),
Amount: sdk.NewCoins(testvalues.DefaultTransferAmount(chainB.Config().Denom)),
}

cfg := simappparams.MakeTestEncodingConfig()
banktypes.RegisterInterfaces(cfg.InterfaceRegistry)
cdc := codec.NewProtoCodec(cfg.InterfaceRegistry)

bz, err := icatypes.SerializeCosmosTx(cdc, []proto.Message{msgSend})
s.Require().NoError(err)

packetData := icatypes.InterchainAccountPacketData{
Type: icatypes.EXECUTE_TX,
Data: bz,
Memo: "e2e",
}

msgSendTx := controllertypes.NewMsgSendTx(controllerAccount.Bech32Address(chainA.Config().Bech32Prefix), ibctesting.FirstConnectionID, uint64(1), packetData)

resp, err := s.BroadcastMessages(
ctx,
chainA,
controllerAccount,
msgSendTx,
)

s.AssertValidTxResponse(resp)
s.Require().NoError(err)

time.Sleep(1 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should indicate that this is to cause a packet timeout

})
})

t.Run("start relayer", func(t *testing.T) {
s.StartRelayer(relayer)
})

t.Run("verify channel is closed due to timeout on ordered channel", func(t *testing.T) {
channel, err := s.QueryChannel(ctx, chainA, portID, "channel-1")
s.Require().NoError(err)

t.Logf("channel-1: %+v", channel)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we need to log the full channel here. I think we can get by with assertions on the fields for values that are deterministic, and we can include the full channel as output in the error message if the assertion fails.

e.g.

// additional assertions if needed 
s.Require().Equal(channeltypes.CLOSED, channel.State, "channel was not in expected state", channel)

Would have to double check on syntax for passing additional args to Require().Equal() though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true

s.Require().Equal(channeltypes.CLOSED, channel.State)
})

t.Run("verify tokens not transferred", func(t *testing.T) {
balance, err := chainB.GetBalance(ctx, chainBAccount.Bech32Address(chainB.Config().Bech32Prefix), chainB.Config().Denom)
s.Require().NoError(err)

_, err = chainB.GetBalance(ctx, hostAccount, chainB.Config().Denom)
s.Require().NoError(err)

expected := testvalues.StartingTokenAmount
s.Require().Equal(expected, balance)
})

// re-register interchain account to reopen the channel now that it has been closed due to timeout
// on an ordered channel
t.Run("register interchain account", func(t *testing.T) {
version := getICAVersion(testconfig.GetChainATag(), testconfig.GetChainBTag())
msgRegisterInterchainAccount := controllertypes.NewMsgRegisterInterchainAccount(ibctesting.FirstConnectionID, controllerAccount.Bech32Address(chainA.Config().Bech32Prefix), version)
err := s.RegisterInterchainAccount(ctx, chainA, controllerAccount, msgRegisterInterchainAccount)
s.Require().NoError(err)

test.WaitForBlocks(ctx, 3, chainA, chainB)
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
test.WaitForBlocks(ctx, 3, chainA, chainB)
s.Require().NoError(test.WaitForBlocks(ctx, 3, chainA, chainB))

})

t.Run("verify new channel is now open and interchain account has been reregistered with the same portID", func(t *testing.T) {
channel, err := s.QueryChannel(ctx, chainA, portID, "channel-2")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can use constants for channel-1 and channel-2 to with names that make it clear that we're talking about the same channel?

initialChannelName := "channel-1"
channelNameAfterReopening := "channel-2"

or something

s.Require().NoError(err)

t.Logf("channel-2: %+v", channel)
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

s.Require().Equal(channeltypes.OPEN, channel.State)
})

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.Bech32Address(chainB.Config().Bech32Prefix),
Amount: sdk.NewCoins(testvalues.DefaultTransferAmount(chainB.Config().Denom)),
}

cfg := simappparams.MakeTestEncodingConfig()
banktypes.RegisterInterfaces(cfg.InterfaceRegistry)
cdc := codec.NewProtoCodec(cfg.InterfaceRegistry)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use this function to fetch the codec.


bz, err := icatypes.SerializeCosmosTx(cdc, []proto.Message{msgSend})
s.Require().NoError(err)

packetData := icatypes.InterchainAccountPacketData{
Type: icatypes.EXECUTE_TX,
Data: bz,
Memo: "e2e",
}

msgSendTx := controllertypes.NewMsgSendTx(controllerAccount.Bech32Address(chainA.Config().Bech32Prefix), ibctesting.FirstConnectionID, uint64(5*time.Minute), packetData)

resp, err := s.BroadcastMessages(
ctx,
chainA,
controllerAccount,
msgSendTx,
)

s.AssertValidTxResponse(resp)
s.Require().NoError(err)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we verify the bank send transferred funds correctly?

14 changes: 14 additions & 0 deletions e2e/testsuite/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,20 @@ func (s *E2ETestSuite) QueryClientState(ctx context.Context, chain ibc.Chain, cl
return clientState, nil
}

// QueryChannel queries the channel on a given chain for the provided portID and channelID
func (s *E2ETestSuite) QueryChannel(ctx context.Context, chain ibc.Chain, portID, channelID string) (channeltypes.Channel, error) {
queryClient := s.GetChainGRCPClients(chain).ChannelQueryClient
res, err := queryClient.Channel(ctx, &channeltypes.QueryChannelRequest{
PortId: portID,
ChannelId: channelID,
})
if err != nil {
return channeltypes.Channel{}, err
}

return *res.Channel, nil
}

Comment on lines +37 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

great addition!

// QueryPacketCommitment queries the packet commitment on the given chain for the provided channel and sequence.
func (s *E2ETestSuite) QueryPacketCommitment(ctx context.Context, chain ibc.Chain, portID, channelID string, sequence uint64) ([]byte, error) {
queryClient := s.GetChainGRCPClients(chain).ChannelQueryClient
Expand Down
1 change: 1 addition & 0 deletions e2e/testsuite/testsuite.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ func (s *E2ETestSuite) AssertValidTxResponse(resp sdk.TxResponse) {
s.Require().NotEqual(int64(0), resp.GasWanted, respLogsMsg)
s.Require().NotEmpty(resp.Events, respLogsMsg)
s.Require().NotEmpty(resp.Data, respLogsMsg)
s.T().Logf("tx response: %s", resp)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we include this this will be quite verbose for all tests. I think the response logs being included should be enough. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea...true. it was useful for checking the tx because checking that the tx is a valid response doesn't actually tell us if the tx succeeded or not when i was running the test locally, but we can verify the bank send transferred funds correctly as you indicated below.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we assert a status code? Or maybe accept an additional argument which is the expected code.

}

// AssertPacketRelayed asserts that the packet commitment does not exist on the sending chain.
Expand Down