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

colin-axner · 2022-08-10T14:58:11Z

Description

closes: #1963

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

colin-axner · 2022-08-10T15:00:52Z

e2e/transfer_test.go

+// 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).


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

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!

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

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

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.

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

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

Thanks for this thread!

chatton · 2022-08-11T11:56:41Z

e2e/transfer_test.go

+	t.Run("relayer wallets recovered", func(t *testing.T) {
+		err := s.RecoverRelayerWallets(ctx, relayer)
+		s.Require().NoError(err)
+	})


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.

…ransfer-test

seantking

I think we should also add an entry here for manual testing.

chatton

LGTM! Great work @colin-axner

chatton · 2022-08-12T09:01:59Z

e2e/testsuite/testsuite.go

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


damiannolan

Looks great! I just left some very minor suggestions but feel free to disregard them! LGTM 🚀

damiannolan · 2022-08-12T13:28:14Z

e2e/transfer_test.go

+
+	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) {


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) {

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

damiannolan · 2022-08-12T13:38:44Z

e2e/transfer_test.go

+		opts.Version = "ics20-1"
+		opts.SourcePortName = "transfer"
+		opts.DestPortName = "transfer"


Should we use transfertypes.Version and transfertypes.PortID here?

great call!

damiannolan · 2022-08-12T13:39:08Z

e2e/transfer_test.go

+// 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).


Thanks for this thread!

crodriguezvega

😎

…ransfer-test

add successful transfer test

bb44e68

colin-axner commented Aug 10, 2022

View reviewed changes

colin-axner added 4 commits August 10, 2022 17:07

add back start relayer

c37c716

use one wallet, fix various bugs

4a56830

fix various bugs, except last issue

f7de720

fix test

acda77b

chatton reviewed Aug 11, 2022

View reviewed changes

colin-axner added 2 commits August 11, 2022 15:53

Merge branch 'main' of github.com:cosmos/ibc-go into colin/1963-e2e-t…

cdb5ed6

…ransfer-test

add packet relayed assertion

44a47ee

colin-axner marked this pull request as ready for review August 11, 2022 14:10

colin-axner requested review from AdityaSripal, crodriguezvega, seantking, charleenfei and damiannolan as code owners August 11, 2022 14:10

seantking suggested changes Aug 11, 2022

View reviewed changes

add transfer test suite as option to e2e manual

16660bb

seantking approved these changes Aug 12, 2022

View reviewed changes

chatton approved these changes Aug 12, 2022

View reviewed changes

damiannolan approved these changes Aug 12, 2022

View reviewed changes

crodriguezvega approved these changes Aug 12, 2022

View reviewed changes

colin-axner added 3 commits August 15, 2022 12:04

apply review suggestions

c75f57d

Merge branch 'main' of github.com:cosmos/ibc-go into colin/1963-e2e-t…

7f70937

…ransfer-test

Merge branch 'main' into colin/1963-e2e-transfer-test

326a7a2

colin-axner added the automerge label Aug 15, 2022

mergify bot merged commit 8201ef9 into main Aug 15, 2022

mergify bot deleted the colin/1963-e2e-transfer-test branch August 15, 2022 10:59

colin-axner mentioned this pull request Aug 15, 2022

Add packet relayed assertion to testsuite.go #1988

Closed

3 tasks

chatton mentioned this pull request Aug 24, 2022

Consider using git-cliff to generate CHANGELOG.md #2106

Closed

3 tasks

chatton mentioned this pull request Nov 7, 2022

Add git cliff toml to support generation of changelog #2697

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

colin-axner commented Aug 10, 2022 •

edited

Loading

colin-axner Aug 10, 2022

chatton Aug 11, 2022

colin-axner Aug 11, 2022 •

edited

Loading

colin-axner Aug 11, 2022

chatton Aug 11, 2022

colin-axner Aug 11, 2022

chatton Aug 11, 2022

damiannolan Aug 12, 2022

chatton Aug 11, 2022

seantking left a comment

chatton left a comment

chatton Aug 12, 2022

damiannolan left a comment

damiannolan Aug 12, 2022

colin-axner Aug 15, 2022 •

edited

Loading

damiannolan Aug 12, 2022

colin-axner Aug 15, 2022

damiannolan Aug 12, 2022

crodriguezvega left a comment


		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) {

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

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

Conversation

colin-axner commented Aug 10, 2022 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seantking left a comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

colin-axner commented Aug 10, 2022 •

edited

Loading

colin-axner Aug 11, 2022 •

edited

Loading

colin-axner Aug 15, 2022 •

edited

Loading