e2e: ICS27 interchain accounts x/group integration #2195

damiannolan · 2022-09-06T10:56:31Z

Description

Adding e2e with x/group integration for interchain-accounts.

closes: #2039

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

…groups

…7-groups

e2e/tests/interchain_accounts/groups_test.go

Co-authored-by: Carlos Rodriguez <[email protected]>

…go into damian/2039-e2e-ics27-groups

e2e/testconfig/testconfig.go

e2e/tests/interchain_accounts/groups_test.go

damiannolan · 2022-09-26T14:34:17Z

e2e/tests/interchain_accounts/groups_test.go

+		members := []grouptypes.MemberRequest{
+			{
+				Address: chainAAddress,
+				Weight:  "1",


extract to const?

probably a good habit

what is a threshold decision policy? What does the weight do?

The ThresholdDecisionPolicy defines a weighted threshold that x amount of YES votes must meet in order to succeed. Each group member can carry a different weight which is accounted for in the tally.

I've moved a bunch of fields to consts at the top of file and added godocs (some copy pasted from sdk).

e2e/testsuite/testsuite.go

e2e/go.mod

chatton · 2022-09-28T09:40:54Z

All the e2e tests will fail as the ibctest ibc-go version needs to match the one in our e2e go mod. This is an sdk limitation at the moment as duplicate types are registered.

colin-axner

Woot woot!! Superb work! Left some suggestions, will review in detail once it is updated to an alpha tag

e2e/testconfig/testconfig.go

e2e/testsuite/testsuite.go

e2e/testsuite/grpc_query.go

colin-axner · 2022-09-28T12:24:33Z

e2e/tests/interchain_accounts/groups_test.go

+		members := []grouptypes.MemberRequest{
+			{
+				Address: chainAAddress,
+				Weight:  "1",


probably a good habit

colin-axner · 2022-09-28T12:25:00Z

e2e/tests/interchain_accounts/groups_test.go

+		members := []grouptypes.MemberRequest{
+			{
+				Address: chainAAddress,
+				Weight:  "1",


what is a threshold decision policy? What does the weight do?

e2e/tests/interchain_accounts/groups_test.go

…-groups

…go into damian/2039-e2e-ics27-groups

codecov-commenter · 2022-10-11T08:03:38Z

Codecov Report

Merging #2195 (c21fa95) into main (4bd05c6) will increase coverage by 0.04%.
The diff coverage is 66.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2195      +/-   ##
==========================================
+ Coverage   78.70%   78.74%   +0.04%     
==========================================
  Files         178      178              
  Lines       12311    12298      -13     
==========================================
- Hits         9689     9684       -5     
+ Misses       2195     2190       -5     
+ Partials      427      424       -3

Impacted Files	Coverage Δ
modules/apps/29-fee/types/msgs.go	`90.43% <ø> (-0.33%)`	⬇️
modules/core/02-client/types/msgs.go	`75.73% <ø> (ø)`
.../27-interchain-accounts/controller/types/params.go	`47.82% <33.33%> (ø)`
...s/apps/27-interchain-accounts/host/types/params.go	`42.10% <33.33%> (ø)`
modules/apps/transfer/types/params.go	`48.00% <40.00%> (ø)`
modules/apps/transfer/ibc_module.go	`64.96% <66.66%> (+0.03%)`	⬆️
modules/apps/transfer/keeper/msg_server.go	`100.00% <100.00%> (ø)`
modules/apps/transfer/keeper/relay.go	`90.63% <100.00%> (+2.56%)`	⬆️
modules/apps/transfer/types/msgs.go	`95.12% <100.00%> (-0.12%)`	⬇️
modules/apps/transfer/types/packet.go	`92.00% <100.00%> (+0.33%)`	⬆️
... and 1 more

crodriguezvega

LGTM. Thanks, @damiannolan!

colin-axner

Excellent! 😎 Super cool to see a test for this. Thanks for handling this work!

colin-axner · 2022-10-14T10:35:09Z

e2e/testsuite/testsuite.go

@@ -365,8 +379,10 @@ func (s *E2ETestSuite) initGRPCClients(chain *cosmos.CosmosChain) {
 		ClientQueryClient:  clienttypes.NewQueryClient(grpcConn),
 		ChannelQueryClient: channeltypes.NewQueryClient(grpcConn),
 		FeeQueryClient:     feetypes.NewQueryClient(grpcConn),
-		ICAQueryClient:     intertxtypes.NewQueryClient(grpcConn),
+		ICAQueryClient:     controllertypes.NewQueryClient(grpcConn),
+		InterTxQueryClient: intertxtypes.NewQueryClient(grpcConn),


Maybe we should open up an issue to remove all the intertx stuff once v5 is no longer supported?

Sure, I can open an issue. When is v5 no longer supported? Like 1 year from now?

colin-axner · 2022-10-14T10:41:51Z

e2e/tests/interchain_accounts/groups_test.go

+	})
+
+	t.Run("verify interchain account registration success", func(t *testing.T) {
+		interchainAccAddr, err = s.QueryInterchainAccount(ctx, chainA, groupPolicyAddr, ibctesting.FirstConnectionID)


I guess you could assert the addr is non empty

Added as an extra sanity check but I don't think its reachable. Something would be wrong with the query then I think

colin-axner · 2022-10-14T10:42:07Z

e2e/tests/interchain_accounts/groups_test.go

+		channels, err := relayer.GetChannels(ctx, s.GetRelayerExecReporter(), chainA.Config().ChainID)
+		s.Require().NoError(err)
+		s.Require().Equal(len(channels), 2)


1 transfer channel, 1 ica channel?

Yup! Added inline comment 👍

colin-axner · 2022-10-14T10:44:15Z

e2e/tests/interchain_accounts/groups_test.go

+		balance, err := chainB.GetBalance(ctx, chainBAddress, chainB.Config().Denom)
+		s.Require().NoError(err)
+
+		_, err = chainB.GetBalance(ctx, interchainAccAddr, chainB.Config().Denom)


should you check that this balance has be subtracted as a sanity check?

* adding x/group to simapp * [WIP] initial groups e2e scaffolding * work in progress * clean * adding interchain account address query to ica controller * adding basic cli query * satisfy linter, aligning recvr var naming * initial passing, register account proposal * successfully passing locally * updating default genesis state with allow all ica msgs, cleanup * reinstate num validators and num full nodes * unpin ibctest * Update e2e/tests/interchain_accounts/groups_test.go Co-authored-by: Carlos Rodriguez <[email protected]> * updating swagger docs * updating e2e to ibc-go/v6 * update deps, remove genesis code, extract consts * code comments and consts * readding intertx query client * fixing godoc * removing json entry in test matracies * adding sanity checks and in-line comment Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 8abcba0) # Conflicts: # e2e/go.mod # e2e/go.sum # e2e/tests/interchain_accounts/base_test.go # e2e/tests/interchain_accounts/incentivized_test.go # e2e/testsuite/grpc_query.go # e2e/testsuite/testsuite.go

damiannolan added 15 commits September 5, 2022 15:12

adding x/group to simapp

4651d79

[WIP] initial groups e2e scaffolding

38b1ae1

Merge branch 'damian/add-x-group-to-simd' into damian/2039-e2e-ics27-…

7faf51c

…groups

work in progress

69a4609

clean

9b7c415

adding interchain account address query to ica controller

6ca5333

adding basic cli query

758e99a

satisfy linter, aligning recvr var naming

62e1562

Merge branch 'damian/add-ica-address-query' into damian/2039-e2e-ics2…

c7ef852

…7-groups

initial passing, register account proposal

56075d4

successfully passing locally

324a095

updating default genesis state with allow all ica msgs, cleanup

1480a62

reinstate num validators and num full nodes

d1be3cb

unpin ibctest

3006b97

Merge branch 'main' into damian/2039-e2e-ics27-groups

ee10b76

crodriguezvega added the backport-to-v5.1.x label Sep 7, 2022

crodriguezvega reviewed Sep 7, 2022

View reviewed changes

e2e/tests/interchain_accounts/groups_test.go Outdated Show resolved Hide resolved

damiannolan and others added 2 commits September 7, 2022 18:33

Update e2e/tests/interchain_accounts/groups_test.go

b2064c6

Co-authored-by: Carlos Rodriguez <[email protected]>

Merge branch 'main' into damian/2039-e2e-ics27-groups

dbadf31

colin-axner added backport-to-v6.0.x and removed backport-to-v5.1.x labels Sep 8, 2022

damiannolan added 4 commits September 24, 2022 13:13

Merge branch 'main' into damian/2039-e2e-ics27-groups

66b3bff

Merge branch 'damian/2039-e2e-ics27-groups' of github.com:cosmos/ibc-…

edafd40

…go into damian/2039-e2e-ics27-groups

updating swagger docs

4d6f6e7

updating e2e to ibc-go/v6

44747df

damiannolan commented Sep 26, 2022

View reviewed changes

e2e/testconfig/testconfig.go Outdated Show resolved Hide resolved

damiannolan commented Sep 26, 2022

View reviewed changes

e2e/tests/interchain_accounts/groups_test.go Outdated Show resolved Hide resolved

damiannolan commented Sep 26, 2022

View reviewed changes

e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved

damiannolan commented Sep 26, 2022

View reviewed changes

e2e/go.mod Outdated Show resolved Hide resolved

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

Merge branch 'main' into damian/2039-e2e-ics27-groups

78eeb39

colin-axner reviewed Sep 28, 2022

View reviewed changes

damiannolan mentioned this pull request Oct 4, 2022

deps: upgrading to ibc-go/v6-alpha1 strangelove-ventures/interchaintest#315

Merged

damiannolan added 5 commits October 6, 2022 17:18

Merge remote-tracking branch 'origin/main' into damian/2039-e2e-ics27…

a7e539b

…-groups

Merge branch 'damian/2039-e2e-ics27-groups' of github.com:cosmos/ibc-…

b9152f3

…go into damian/2039-e2e-ics27-groups

Merge branch 'main' into damian/2039-e2e-ics27-groups

c7e6b04

update deps, remove genesis code, extract consts

44b6baf

Merge branch 'damian/2039-e2e-ics27-groups' of github.com:cosmos/ibc-…

b420a43

…go into damian/2039-e2e-ics27-groups

damiannolan and others added 6 commits October 11, 2022 10:14

code comments and consts

2126b4b

readding intertx query client

a8d806f

fixing godoc

3fe0582

removing json entry in test matracies

1f61ada

Merge branch 'main' into damian/2039-e2e-ics27-groups

13af0f5

Merge branch 'main' into damian/2039-e2e-ics27-groups

c21fa95

crodriguezvega approved these changes Oct 14, 2022

View reviewed changes

colin-axner approved these changes Oct 14, 2022

View reviewed changes

damiannolan added 2 commits October 14, 2022 15:36

adding sanity checks and in-line comment

5c81e17

Merge branch 'main' into damian/2039-e2e-ics27-groups

8894b23

damiannolan merged commit 8abcba0 into main Oct 14, 2022

damiannolan deleted the damian/2039-e2e-ics27-groups branch October 14, 2022 14:23

mergify bot mentioned this pull request Oct 14, 2022

e2e: ICS27 interchain accounts x/group integration (backport #2195) #2550

Closed

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: ICS27 interchain accounts x/group integration #2195

e2e: ICS27 interchain accounts x/group integration #2195

damiannolan commented Sep 6, 2022 •

edited

Loading

damiannolan Sep 26, 2022

colin-axner Sep 28, 2022

colin-axner Sep 28, 2022

damiannolan Oct 11, 2022

chatton commented Sep 28, 2022 •

edited

Loading

colin-axner left a comment

colin-axner Sep 28, 2022

colin-axner Sep 28, 2022

codecov-commenter commented Oct 11, 2022 •

edited

Loading

crodriguezvega left a comment

colin-axner left a comment

colin-axner Oct 14, 2022

damiannolan Oct 14, 2022

colin-axner Oct 14, 2022

damiannolan Oct 14, 2022

colin-axner Oct 14, 2022

damiannolan Oct 14, 2022

colin-axner Oct 14, 2022

damiannolan Oct 14, 2022

e2e: ICS27 interchain accounts x/group integration #2195

e2e: ICS27 interchain accounts x/group integration #2195

Conversation

damiannolan commented Sep 6, 2022 • edited Loading

Description

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 Sep 28, 2022 • edited Loading

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

codecov-commenter commented Oct 11, 2022 • edited Loading

Codecov Report

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

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

damiannolan commented Sep 6, 2022 •

edited

Loading

chatton commented Sep 28, 2022 •

edited

Loading

codecov-commenter commented Oct 11, 2022 •

edited

Loading