Skip to content
132 changes: 132 additions & 0 deletions e2e/transfer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package e2e

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
Contributor

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.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
chainBAddress := chainBWallet.Bech32Address(chainB.Config().Bech32Prefix)

t.Run("relayer wallets recovered", func(t *testing.T) {
err := s.RecoverRelayerWallets(ctx, relayer)
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.

I believe we don't need this. In the Fee Middleware tests, we recover the relayer wallets as we want to broadcast messages on behalf of relayer users recovering wallets adds the corresponding entries to the test keychain and so enables this.

In this test we don't need to do that as we only broadcast transactions from the created wallets, not the relayer ones.


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

t.Run("native IBC token transfer from chainA (source) to chainB, sender is source", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: minor improvement maybe? feel free to disregard

Suggested change
t.Run("native IBC token transfer from chainA (source) to chainB, sender is source", func(t *testing.T) {
t.Run("native IBC token transfer from chainA (source) to chainB (destination), sender is source", func(t *testing.T) {

Copy link
Contributor Author

@colin-axner colin-axner Aug 15, 2022

Choose a reason for hiding this comment

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

source was in reference to the tokens origination, not where the tokens were being sent from. I've updated it to be a little more specific

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) {
actualBalance, err := chainA.GetBalance(ctx, chainAAddress, 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 (source), receiver is source", 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 := chainA.GetBalance(ctx, chainAAddress, chainBIBCToken.IBCDenom())
s.Require().NoError(err)

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

t.Run("packets are relayed", func(t *testing.T) {
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 = "ics20-1"
opts.SourcePortName = "transfer"
opts.DestPortName = "transfer"
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 use transfertypes.Version and transfertypes.PortID here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great call!

}
}

// 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))
}

// getTimeoutHeight returns a timeout height of 1000 blocks above the current block height
// this function should be used when the timeout is never expected to be reached
func (s *TransferTestSuite) getTimeoutHeight(ctx context.Context, chain *cosmos.CosmosChain) clienttypes.Height {
height, err := chain.Height(ctx)
s.Require().NoError(err)
return clienttypes.NewHeight(clienttypes.ParseChainID(chain.Config().ChainID), uint64(height)+1000)
}