e2e: transfer memo tests #2306

nicolaslara · 2022-09-19T16:08:46Z

Description

adds e2e tests for the transfer memo addition

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

modules/apps/transfer/keeper/relay.go

modules/apps/transfer/keeper/relay_test.go

modules/apps/transfer/keeper/msg_server_test.go

e2e/tests/transfer/metadata_test.go

chatton · 2022-09-28T12:39:02Z

Hi @nicolaslara thanks for the contribution!

Regarding CI, currently, the smallest unit of exclusion/inclusion is a test suite. Since we don't want to run this on PRs (just during compatibility testing as @colin-axner suggested) I recommend the following steps.

Create a new test suite with the test you are adding. (the test should be in the same file as the test suite)
Add an exclusion here and here Note: this is the name of the function that triggers the test suite, not the name of the test suite.

The compatibility tests are currently a WIP, so we can add your test to them once it is fully up and running!

…data-integration-and-tests

nicolaslara · 2022-10-04T08:44:46Z

@chatton @colin-axner I started modifying the tests the way you suggested, but after merging main I noticed that it got updated to v6 while the e2e tests are still on v5. Should I keep this PR on v5? or should I update the e2e tests to v6 (which probably should be done in a separate PR)?

colin-axner · 2022-10-05T11:29:33Z

Should I keep this PR on v5? or should I update the e2e tests to v6 (which probably should be done in a separate PR)?

I believe we need to update the ibc-go version used in the ibctest repository first. See strangelove-ventures/interchaintest#315. I'll be bumping this in a separate pr, you are welcome to use the commit in the linked pr in the meantime to get tests passing

…data-integration-and-tests

e2e/tests/transfer/metadata_test.go

…etadata-integration-and-tests

…b.com:nicolaslara/ibc-go into nicolas/packet-metadata-integration-and-tests

chatton

Really nice work, thanks @colin-axner & @nicolaslara ❤️

chatton · 2022-10-26T13:47:05Z

e2e/tests/transfer/base_test.go

+var memoFeatureRelease = testsuite.FeatureReleases{
+	MajorVersion:  "v6",
+	MinorVersions: []string{"v2.5, v3.4, v4.2, v5.1"},
+}


chatton · 2022-10-26T13:51:34Z

e2e/tests/transfer/base_test.go

+		s.Require().NoError(err)
+
+		if !memoFeatureRelease.IsSupported(chainAVersion) {
+			s.Require().Equal(transferTxResp.Code, uint32(2))


[nit] we should have arguments in the order of expected / actual

yup, nice catch

chatton · 2022-10-26T13:54:37Z

e2e/tests/transfer/base_test.go

+
+	chainBIBCToken := testsuite.GetIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID)
+
+	t.Run("packets relayed?", func(t *testing.T) {


[nit] this punctuation is a bit strange in test names, is this intentional?

don't think so

charleenfei

lgtm! just a nit on replacing with local ibc-go version

charleenfei · 2022-10-26T13:55:11Z

e2e/go.mod

@@ -214,4 +214,4 @@ replace (
 )

 // uncomment to use the local version of ibc-go, you will need to run `go mod tidy` in e2e directory.
-// replace github.com/cosmos/ibc-go/v6 => ../
+replace github.com/cosmos/ibc-go/v6 => ../


should we recomment this?

we want this commented out as this comment is simply here for convenience to allow for testing against local ibc-go changes.

ibc-go doesn't have a release with the memo changes yet so the constructors would break otherwise

colin-axner · 2022-10-26T13:35:57Z

e2e/testsuite/testsuite.go

@@ -304,9 +304,9 @@ func (s *E2ETestSuite) RecoverRelayerWallets(ctx context.Context, relayer ibc.Re

 // Transfer broadcasts a MsgTransfer message.
 func (s *E2ETestSuite) Transfer(ctx context.Context, chain *cosmos.CosmosChain, user *ibc.Wallet,
-	portID, channelID string, token sdk.Coin, sender, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64,
+	portID, channelID string, token sdk.Coin, sender, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, memo string,


decided to make this default behaviour going forward. In the future we should add a type to send default ibc transfers to cut down on args here. Think you could have something like endpoint.SendIBCTransfer(receiver)

colin-axner · 2022-10-26T13:36:22Z

e2e/tests/transfer/base_test.go

+// If the chains contain a version of FungibleTokenPacketData with memo, both send and receive should succeed.
+// If one of the chains contains a version of FungibleTokenPacketData without memo, then receiving a packet with
+// memo should fail in that chain
+func (s *TransferTestSuite) TestMsgTransfer_WithMemo() {


moved into base_test.go since this should work for any version of ibc-go

colin-axner · 2022-10-26T14:04:14Z

e2e/tests/transfer/base_test.go

+		s.Require().NoError(err)
+
+		if !memoFeatureRelease.IsSupported(chainAVersion) {
+			s.Require().Equal(transferTxResp.Code, uint32(2))


yup, nice catch

colin-axner · 2022-10-26T14:04:25Z

e2e/tests/transfer/base_test.go

+
+	chainBIBCToken := testsuite.GetIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID)
+
+	t.Run("packets relayed?", func(t *testing.T) {


don't think so

colin-axner · 2022-10-26T14:05:02Z

e2e/go.mod

@@ -214,4 +214,4 @@ replace (
 )

 // uncomment to use the local version of ibc-go, you will need to run `go mod tidy` in e2e directory.
-// replace github.com/cosmos/ibc-go/v6 => ../
+replace github.com/cosmos/ibc-go/v6 => ../


ibc-go doesn't have a release with the memo changes yet so the constructors would break otherwise

e2e/tests/transfer/base_test.go

chatton · 2022-10-26T14:11:05Z

@colin-axner the TestInterTxTestSuite wasn't yet added to test exclusions for forks, these will be expected to fail here as they should only work with icad. We can update the workflows to exclude them after this PR 👍

colin-axner · 2022-10-26T14:29:25Z

e2e/tests/transfer/base_test.go

+	if !memoFeatureRelease.IsSupported(chainAVersion) {
+		// transfer not sent, end test
+		return


since the checks are run in t.Run(), we need a check here to properly return. I left the code as is because I think it is nice doing assertions in t.Run()

damiannolan

Looks good to me! Thanks a lot @nicolaslara and @colin-axner 🚀

damiannolan · 2022-10-26T14:56:59Z

e2e/tests/transfer/base_test.go

+	chainAVersion := chainA.Config().Images[0].Version
+	chainBVersion := chainB.Config().Images[0].Version
+
+	t.Logf("Running memo tests versions chainA: %s, chainB: %s", chainAVersion, chainBVersion)


Should we also remove? :D

damiannolan · 2022-10-26T14:59:24Z

e2e/tests/transfer/base_test.go

+// memoFeatureReleases represents the releases the memo field was released in.
+var memoFeatureRelease = testsuite.FeatureReleases{


mega nit: should the var be the same as in the godoc or vice versa?

nicolaslara added 3 commits September 19, 2022 17:54

added optional packet metadata to the packet and message types

a9f4f40

added docs

2254b3d

packet metadata integration and tests

ca6b1d1

nicolaslara mentioned this pull request Sep 22, 2022

CrossChainSwaps: Add a middleware to handle IBC metadata for the swap osmosis-labs/osmosis#2661

Closed

crodriguezvega mentioned this pull request Sep 28, 2022

chore: add memo in ics20 packet data (cosmos#2411) #2415

Closed

9 tasks

colin-axner reviewed Sep 28, 2022

View reviewed changes

nicolaslara added 8 commits October 3, 2022 15:36

Merge branch 'main' into nicolas/packet-metadata-proto

ed80695

breaking the api (backports should add a utility function for this)

7c925b9

adding nil metadata on all the calls

1448633

added metadata to the cli

71eae26

added events

2480e13

Merge branch 'nicolas/packet-metadata-proto' into nicolas/packet-meta…

f754572

…data-integration-and-tests

updated types to the api breaking versions

2ed648f

added tests with metadata

c43e5ad

nicolaslara added 8 commits October 5, 2022 15:41

breaking api for FungibleTokenPacketData

bbd1d91

hex encoding metadata

9db4f98

added abstraction

40709af

Merge branch 'main' into nicolas/packet-metadata-proto

415cc40

fixed bad merge

1497a4a

added tests with metadata

5faf1db

added missing metadata to packet for recv

8eb9bc8

Merge branch 'nicolas/packet-metadata-proto' into nicolas/packet-meta…

389bb8a

…data-integration-and-tests

crodriguezvega reviewed Oct 6, 2022

View reviewed changes

e2e/tests/transfer/metadata_test.go Outdated Show resolved Hide resolved

e2e/tests/transfer/metadata_test.go Outdated Show resolved Hide resolved

colin-axner added 4 commits October 26, 2022 14:55

Merge branch 'main' of github.com:cosmos/ibc-go into nicolas/packet-m…

47df4c6

…etadata-integration-and-tests

fix tests from merge conflict

f14fe9f

chore: update tests to use semver

c466942

Merge branch 'main' of github.com:cosmos/ibc-go into nicolas/packet-m…

4737852

…etadata-integration-and-tests

colin-axner added 4 commits October 26, 2022 15:31

chore: fix e2e tests

4c2e228

Merge branch 'main' into nicolas/packet-metadata-integration-and-tests

40bdac4

chore: revert unnecessary change

a6587eb

Merge branch 'nicolas/packet-metadata-integration-and-tests' of githu…

915b68b

…b.com:nicolaslara/ibc-go into nicolas/packet-metadata-integration-and-tests

colin-axner marked this pull request as ready for review October 26, 2022 13:38

colin-axner requested review from AdityaSripal, seantking, charleenfei, damiannolan and chatton as code owners October 26, 2022 13:38

colin-axner changed the title ~~Packet metadata integration and tests~~ e2e: transfer memo tests Oct 26, 2022

chatton approved these changes Oct 26, 2022

View reviewed changes

charleenfei approved these changes Oct 26, 2022

View reviewed changes

colin-axner reviewed Oct 26, 2022

View reviewed changes

chore: apply review suggestions

85d6f20

colin-axner reviewed Oct 26, 2022

View reviewed changes

e2e/tests/transfer/base_test.go Outdated Show resolved Hide resolved

fix: exit test upon unsupported chainA

c939043

colin-axner reviewed Oct 26, 2022

View reviewed changes

damiannolan approved these changes Oct 26, 2022

View reviewed changes

AdityaSripal approved these changes Oct 26, 2022

View reviewed changes

colin-axner added 2 commits October 26, 2022 17:20

chore: apply review suggestions

05fed7b

Merge branch 'main' into nicolas/packet-metadata-integration-and-tests

f22779b

colin-axner merged commit 8991eaa into cosmos:main Oct 26, 2022

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

e2e: transfer memo tests #2306

e2e: transfer memo tests #2306

nicolaslara commented Sep 19, 2022 •

edited by colin-axner

Loading

chatton commented Sep 28, 2022 •

edited

Loading

nicolaslara commented Oct 4, 2022

colin-axner commented Oct 5, 2022

chatton left a comment

chatton Oct 26, 2022

chatton Oct 26, 2022

colin-axner Oct 26, 2022

chatton Oct 26, 2022

colin-axner Oct 26, 2022

charleenfei left a comment

charleenfei Oct 26, 2022

chatton Oct 26, 2022

colin-axner Oct 26, 2022

colin-axner Oct 26, 2022

colin-axner Oct 26, 2022

colin-axner Oct 26, 2022

colin-axner Oct 26, 2022

colin-axner Oct 26, 2022

chatton commented Oct 26, 2022

colin-axner Oct 26, 2022

damiannolan left a comment

damiannolan Oct 26, 2022

colin-axner Oct 26, 2022

damiannolan Oct 26, 2022


		chainBIBCToken := testsuite.GetIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID)

		t.Run("packets relayed?", func(t *testing.T) {

		// memoFeatureReleases represents the releases the memo field was released in.
		var memoFeatureRelease = testsuite.FeatureReleases{

e2e: transfer memo tests #2306

e2e: transfer memo tests #2306

Conversation

nicolaslara commented Sep 19, 2022 • edited by colin-axner Loading

Description

chatton commented Sep 28, 2022 • edited Loading

nicolaslara commented Oct 4, 2022

colin-axner commented Oct 5, 2022

chatton left a 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

charleenfei left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Oct 26, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaslara commented Sep 19, 2022 •

edited by colin-axner

Loading

chatton commented Sep 28, 2022 •

edited

Loading