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

feat(test): add successful e2e transfer test #1973

Merged
merged 11 commits into from
Aug 15, 2022
1 change: 1 addition & 0 deletions .github/workflows/e2e-manual.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ on:
required: true
type: choice
options:
- TestTransferTestSuite
- TestFeeMiddlewareTestSuite
chain-a-image:
description: 'The image to use for chain A'
Expand Down
7 changes: 7 additions & 0 deletions e2e/testsuite/testsuite.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,13 @@ func (s *E2ETestSuite) AssertValidTxResponse(resp sdk.TxResponse) {
s.Require().NotEmpty(resp.Data, respLogsMsg)
}

// AssertPacketRelayed asserts that the packet commitment does not exist on the sending chain.
// The packet commitment will be deleted upon a packet acknowledgement or timeout.
func (s *E2ETestSuite) AssertPacketRelayed(ctx context.Context, chain *cosmos.CosmosChain, portID, channelID string, sequence uint64) {
commitment, _ := s.QueryPacketCommitment(ctx, chain, portID, channelID, sequence)
s.Require().Empty(commitment)
}
Comment on lines +297 to +300
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


// createCosmosChains creates two separate chains in docker containers.
// test and can be retrieved with GetChains.
func (s *E2ETestSuite) createCosmosChains(chainOptions testconfig.ChainOptions) (*cosmos.CosmosChain, *cosmos.CosmosChain) {
Expand Down
129 changes: 129 additions & 0 deletions e2e/transfer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
package e2e

/*
The TransferTestSuite assumes both chainA and chainB support version ics20-1.
*/

import (
"context"
"fmt"
"testing"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/strangelove-ventures/ibctest"
"github.com/strangelove-ventures/ibctest/chain/cosmos"
"github.com/strangelove-ventures/ibctest/ibc"
"github.com/strangelove-ventures/ibctest/test"
"github.com/stretchr/testify/suite"

"github.com/cosmos/ibc-go/e2e/testsuite"
"github.com/cosmos/ibc-go/e2e/testvalues"
transfertypes "github.com/cosmos/ibc-go/v5/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v5/modules/core/02-client/types"
)

func TestTransferTestSuite(t *testing.T) {
suite.Run(t, new(TransferTestSuite))
}

type TransferTestSuite struct {
testsuite.E2ETestSuite
}

// Transfer broadcasts a MsgTransfer message.
func (s *TransferTestSuite) Transfer(ctx context.Context, chain *cosmos.CosmosChain, user *ibctest.User,
portID, channelID string, token sdk.Coin, sender, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64,
) (sdk.TxResponse, error) {
msg := transfertypes.NewMsgTransfer(portID, channelID, token, sender, receiver, timeoutHeight, timeoutTimestamp)
return s.BroadcastMessages(ctx, chain, user, msg)
}

// TestMsgTransfer_Succeeds_Nonincentivized will test sending successful IBC transfers from chainA to chainB.
// The transfer will occur over a basic transfer channel (non incentivized) and both native and non-native tokens
// will be sent forwards and backwards in the IBC transfer timeline (both chains will act as source and receiver chains).
Comment on lines +41 to +43
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 thought about writing a test where chainA and chainB both send do the native denomination and non-native denomination send back, but I think the test suite is currently setup to expect tests to happen via chainA -> chainB interaction and then we should ensure during cross compatiblity that we test both sides ie
v5 -> v2
v2 -> v5

Copy link
Contributor

Choose a reason for hiding this comment

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

there's nothing stopping us from doing it whichever way we want, in this case it would just mean that we would run one test and it would cover both directions.

We should be able to send from chainB -> chainA just as easily as chainA -> chainB unless I'm missing something!

Copy link
Contributor Author

@colin-axner colin-axner Aug 11, 2022

Choose a reason for hiding this comment

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

Yup, I think the main area of uncertainty for me is why channelB isn't returned. I think that's what made me think the test suite was meant to be unidirectional rather than bidirectional.

I think it might be best to aim for all tests to be written bidirectional. It's a bit extra work, but then we won't miss a bug because we forget to run an e2e test
v2 -> v5
v5 -> v2

Do we think we could modify the e2e test suite to have some sort of structure that wraps channelA/channelB. In the testing package we have path which contains references to an endpoint which has the (client/connection/channel info)

Edit: it isn't immediately clear whether we should write unidirectional or bidirectional tests given potential future considerations (gracefully handling situations where the counterparty doesn't have the latest version as you). Will give this some thought

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some more thought, I think it is better to write a unidirectional testing pattern. By that I mean, focusing on the interaction of chainA to chainB and forcing the tests to be run in both directions (if relevant)

So for this test:
v2 -> v5 should be run
v5 -> v2 should be run

If we add a ics20-2 version in the future. We will likely add two sorts of tests with this v2

  • tests expecting chainB to support ics20-2
  • tests expecting chainB not to support ics20-2

I think we could introduce logic to automatically configure this when writing the test suite (ie indicate the version range chainB should use, try running both sides with the v2/v5 binaries and skip the test if the version range check isn't met), but we could also do this manually in the short term

I think it makes more sense to indicate what versions chainA should have and what versions chainB should have and then run every combination rather than to perform the chainA/chainB logic twice, as it gets more convoluted when you trying adding a third chain or more specific version handling

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think I like the idea of each test focusing on one direction. Re the channelB not being returned, the use cases I had found so far had been satisfied by just using channelA.CounterParty but I think it's completely reasonable to return both channelA and channelB if that is needed.

If tests are unidirectional, we can do any sort of configuration of permutations outside of the tests themselves which is quite powerful. We can define the rules however we like and run the same tests with different versions as appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Access to channelB just allows us to use ibc.ChannelOutputs as a function argument instead of passing channelA.Counterparty.PortID, channelA.Counterparty.ChannelID

Copy link
Contributor

Choose a reason for hiding this comment

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

Great point, I'm happy to have the signature updated to return both ibc.ChannelOutputs

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this thread!

func (s *TransferTestSuite) TestMsgTransfer_Succeeds_Nonincentivized() {
t := s.T()
ctx := context.TODO()

relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, transferChannelOptions())
chainA, chainB := s.GetChains()

chainADenom := chainA.Config().Denom

chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
chainAAddress := chainAWallet.Bech32Address(chainA.Config().Bech32Prefix)

chainBWallet := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount)
chainBAddress := chainBWallet.Bech32Address(chainB.Config().Bech32Prefix)

s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks")

t.Run("native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
transferTxResp, err := s.Transfer(ctx, chainA, chainAWallet, channelA.PortID, channelA.ChannelID, testvalues.DefaultTransferAmount(chainADenom), chainAAddress, chainBAddress, s.GetTimeoutHeight(ctx, chainB), 0)
s.Require().NoError(err)
s.AssertValidTxResponse(transferTxResp)
})

t.Run("tokens are escrowed", func(t *testing.T) {
actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet)
s.Require().NoError(err)

expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance)
})

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

chainBIBCToken := s.getIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID)

t.Run("packets are relayed", func(t *testing.T) {
s.AssertPacketRelayed(ctx, chainA, channelA.PortID, channelA.ChannelID, 1)

actualBalance, err := chainB.GetBalance(ctx, chainBAddress, chainBIBCToken.IBCDenom())
s.Require().NoError(err)

expected := testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance)
})

t.Run("non-native IBC token transfer from chainB to chainA, receiver is source of tokens", func(t *testing.T) {
transferTxResp, err := s.Transfer(ctx, chainB, chainBWallet, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, testvalues.DefaultTransferAmount(chainBIBCToken.IBCDenom()), chainBAddress, chainAAddress, s.GetTimeoutHeight(ctx, chainA), 0)
s.Require().NoError(err)
s.AssertValidTxResponse(transferTxResp)
})

t.Run("tokens are escrowed", func(t *testing.T) {
actualBalance, err := chainB.GetBalance(ctx, chainBAddress, chainBIBCToken.IBCDenom())
s.Require().NoError(err)

s.Require().Equal(int64(0), actualBalance)
})

s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")

t.Run("packets are relayed", func(t *testing.T) {
s.AssertPacketRelayed(ctx, chainB, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, 1)

actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet)
s.Require().NoError(err)

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

// transferChannelOptions configures both of the chains to have non-incentivized transfer channels.
func transferChannelOptions() func(options *ibc.CreateChannelOptions) {
return func(opts *ibc.CreateChannelOptions) {
opts.Version = transfertypes.Version
opts.SourcePortName = transfertypes.PortID
opts.DestPortName = transfertypes.PortID
}
}

// getIBCToken returns the denomination of the full token denom sent to the receiving channel
func (s *TransferTestSuite) getIBCToken(fullTokenDenom string, portID, channelID string) transfertypes.DenomTrace {
return transfertypes.ParseDenomTrace(fmt.Sprintf("%s/%s/%s", portID, channelID, fullTokenDenom))
}