E2E: Add ICA MsgSubmitTx tests (success + failure) #2021

chatton · 2022-08-16T13:19:15Z

Description

This PR adds an E2E test which tests the happy path for in interchain accounts transaction using the icad docker images.

Thank you @charleenfei and @damiannolan for all the help with this one 🥇

closes: #1941

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

…ity-tests

https://github.com/cosmos/ibc-go into cian/issue#1911-support-backwards-compatibility-tests

…terchain-accounts-sample-test

charleenfei · 2022-08-16T14:10:06Z

e2e/interchain_accounts_test.go

+	return err
+}
+
+func (s *InterchainAccountsTestSuite) TestMsgSubmitTx_SuccessfulTransfer() {


ive got the fail case /unhappy case below as well, so maybe we can keep this naming but break out the unhappy case into a separate test function

separated out into 2 tests as you suggested!

colin-axner

LGTM! Excellent work

colin-axner · 2022-08-17T10:03:23Z

e2e/interchain_accounts_test.go

+			_, err = chainB.GetBalance(ctx, hostAccount, chainB.Config().Denom)
+			s.Require().NoError(err)
+
+			expected := testvalues.IBCTransferAmount + testvalues.StartingTokenAmount


I find it a little confusing that we use IBCTransferAmount in a bank send. Maybe we could add a short comment explaining we use that var in the bank send. I don't have a strong preference though

e2e/interchain_accounts_test.go

damiannolan · 2022-08-17T10:27:07Z

e2e/interchain_accounts_test.go

+}
+
+// RegisterICA will attempt to register an interchain account on the counterparty chain.
+func (s *InterchainAccountsTestSuite) RegisterICA(ctx context.Context, chain *cosmos.CosmosChain, user *ibctest.User, fromAddress, connectionID string) error {


I think I'd prefer RegisterInterchainAccount to be explicit.

We could refactor this function to take the MsgRegisterAccount as an arg in place of the fromAddr and connnectionID. Then we can built the message with an explicit version for both incentivized and non-incentivized channels.

good idea makes sense

damiannolan · 2022-08-17T10:30:15Z

e2e/interchain_accounts_test.go

+			transferMsg := &banktypes.MsgSend{
+				FromAddress: hostAccount,
+				ToAddress:   chainBAccount.Bech32Address(chainB.Config().Bech32Prefix),
+				Amount:      sdk.NewCoins(testvalues.DefaultTransferAmount(chainB.Config().Denom)),
+			}
+
+			// assemble submitMessage tx for intertx
+			submitMsg, err := intertxtypes.NewMsgSubmitTx(
+				transferMsg,
+				connectionId,
+				controllerAccount.Bech32Address(chainA.Config().Bech32Prefix),
+			)


naming nits: I think we should name vars to correspond with the msg type. I got confused with transferMsg as when I read it I think ibc transfer (ics20).

What do you think about using msgSend and msgSubmitTx, I find it reads a little easier.

seantking · 2022-08-17T11:13:20Z

e2e/interchain_accounts_test.go

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


wdyt about renaming controllerAccount to ownerAccount and the hostAccount as interchainAccount?

cc: @damiannolan

i personally think that this change would be more confusing -- but that's just my opinion because of the language from the spec.

I personally like host/controller terminology

seantking · 2022-08-17T11:16:09Z

e2e/interchain_accounts_test.go

+		s.Require().Equal(len(channels), 2)
+	})
+
+	t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) {


Suggested change

t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) {

t.Run("interchain account executes a bank transfer on behalf of the corresponding owner account", func(t *testing.T) {

seantking · 2022-08-17T11:16:44Z

e2e/interchain_accounts_test.go

+
+	t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) {
+
+		t.Run("fund host wallet", func(t *testing.T) {


Suggested change

t.Run("fund host wallet", func(t *testing.T) {

t.Run("fund interchain account wallet", func(t *testing.T) {

seantking · 2022-08-17T11:17:47Z

e2e/testconfig/testconfig.go

@@ -8,6 +8,7 @@ import (
 )

 const (
+


Suggested change

seantking

Left a few nits. Nice work :-)

chatton and others added 30 commits August 8, 2022 15:37

feat(test): adding support for backwards compatibility testing

8f44301

Merge branch 'main' into cian/issue#1911-support-backwards-compatibil…

f470bcf

…ity-tests

chore: made names of variables more descriptive

2297280

Merge branch 'cian/issue#1911-support-backwards-compatibility-tests' of

4c30485

https://github.com/cosmos/ibc-go into cian/issue#1911-support-backwards-compatibility-tests

wip: adding demo interchain accounts test

cd6363c

wip

d87a711

merge main

a9ceda1

wip

2ec4c73

chore: merge main

39eff2f

chore: pass -p events to relayer

17d5840

wip: temporarily setting env vars in the test

1c79319

chore: adding Make command to run test

1db8103

work in progress ICA test using s.BroadcastMessages

57067fb

chore: refactoring register ica account to broadcast message

6890de3

wip update to test host account

d147af9

chore: converting query interchain account to grpc query call

917a6a5

chore: specify empty version string explicitly

27cd4f8

merge sample-test branch

4afd01e

wip

f452e8b

Merge branch 'cian/refactor-register-ica' into cian/issue#1941-add-in…

e0e33f1

…terchain-accounts-sample-test

wip bank transfer

eb4bcb5

wip

85370a8

cleanup tests

d96c0d5

rm init env versions, handle unhappy path

83eaa6f

setup env

5328362

chore: adding comment to outline temprorary usage of setting env vars

289af25

chore: merge main

0094b36

chore: removing unused code

bd9ebc4

chore: exporting GetRelayerExecReporter method

61a444e

chore: adding wait for blocks

50c9375

chatton added 3 commits August 16, 2022 14:54

chore: updating names of test functions

bb08d13

chore: updating test name

f9de6e6

chore: corrected typo

e688082

charleenfei reviewed Aug 16, 2022

View reviewed changes

charleenfei mentioned this pull request Aug 16, 2022

E2E Test Coverage #1607

Closed

3 tasks

chrore: split test into two separate test cases

cf8a8bc

chatton changed the title ~~E2E: Add ICA happy path test~~ E2E: Add ICA MsgSubmitTx tests (success + failure) Aug 16, 2022

chatton marked this pull request as ready for review August 16, 2022 15:19

chatton requested review from colin-axner, AdityaSripal, crodriguezvega, seantking and damiannolan as code owners August 16, 2022 15:19

colin-axner approved these changes Aug 17, 2022

View reviewed changes

damiannolan reviewed Aug 17, 2022

View reviewed changes

seantking reviewed Aug 17, 2022

View reviewed changes

e2e/testconfig/testconfig.go Outdated

@@ -8,6 +8,7 @@ import (

)

const (

Copy link

Contributor

seantking Aug 17, 2022

Choose a reason for hiding this comment

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

Suggested change

seantking approved these changes Aug 17, 2022

View reviewed changes

chore: addressing PR feedback

609d93d

chatton merged commit a9bc591 into main Aug 17, 2022

chatton deleted the cian/issue#1941-add-interchain-accounts-sample-test branch August 17, 2022 13:46

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

E2E: Add ICA MsgSubmitTx tests (success + failure) #2021

E2E: Add ICA MsgSubmitTx tests (success + failure) #2021

chatton commented Aug 16, 2022 •

edited

Loading

charleenfei Aug 16, 2022

chatton Aug 16, 2022

colin-axner left a comment

colin-axner Aug 17, 2022

damiannolan Aug 17, 2022

chatton Aug 17, 2022

damiannolan Aug 17, 2022

seantking Aug 17, 2022 •

edited

Loading

charleenfei Aug 17, 2022 •

edited

Loading

colin-axner Aug 17, 2022

seantking Aug 17, 2022

seantking Aug 17, 2022

seantking Aug 17, 2022

seantking left a comment

	t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) {
	t.Run("interchain account executes a bank transfer on behalf of the corresponding owner account", func(t *testing.T) {


		t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) {

		t.Run("fund host wallet", func(t *testing.T) {

	t.Run("fund host wallet", func(t *testing.T) {
	t.Run("fund interchain account wallet", func(t *testing.T) {

E2E: Add ICA MsgSubmitTx tests (success + failure) #2021

E2E: Add ICA MsgSubmitTx tests (success + failure) #2021

Conversation

chatton commented Aug 16, 2022 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner 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

seantking Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

charleenfei Aug 17, 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

seantking left a comment

Choose a reason for hiding this comment

chatton commented Aug 16, 2022 •

edited

Loading

seantking Aug 17, 2022 •

edited

Loading

charleenfei Aug 17, 2022 •

edited

Loading