Update E2E infra to support more than 2 chains #6524

chatton · 2024-06-06T10:16:51Z

Description

This PR removes a lot of the ChainA/ChainB assumptions within our E2Es to allow for tests with more than 2 chains to be spun up.

Note: this is not exhaustive, there are still some helper/utility functions that assume a 2 chain environment. This PR enables N chains to be created, and for them to be linked via connections/channels by the relayer.

A stub test has been added which verifies that this works, and can be reworked as part of the transfer forwarding work.

closes: #6242

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.

…a test

…based on arbitrary number of chains

coderabbitai · 2024-06-06T10:17:52Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

chatton · 2024-06-06T10:23:59Z

e2e/tests/transfer/forwarding_test.go

+
+	relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, s.TransferChannelOptions(), func(options *testsuite.ChainOptions) {
+		// copy all values of existing chains and tweak to make unique to new chain.
+		chainCSpec := *options.ChainSpecs[0] // nolint


it was complaining about copying a lock, I think we can ignore since we'll be refactoring this entire test later.

I am wondering about this API to set up chains. Rather than having to do this manipulation of the options, couldn't we simply have a couple of configs that we could use (two-chain, three-chain, whatever) where we use two-chain by default and we can just have something like s.UseTestConfig(testsuite.ThreeChainConfig). It would make the intent a little clearer (and shorter). I guess it depends how much we will use different configs, but just an idea.

(This is more on side of this PR, just wanted to write it while I had it in mind...)

Yeah that's a nice idea, I think we can do something similar to where we have s.TransferChannelOptions(). The 3rd chain will likely be the same in every test that needs it, so we can default to 2. Then provide a standard default for the 3 chain setup.

…-be-able-to-spin-up-3-or-more-chains

gjermundgaraba · 2024-06-11T09:13:11Z

e2e/tests/transfer/forwarding_test.go

+
+	relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, s.TransferChannelOptions(), func(options *testsuite.ChainOptions) {
+		// copy all values of existing chains and tweak to make unique to new chain.
+		chainCSpec := *options.ChainSpecs[0] // nolint


I am wondering about this API to set up chains. Rather than having to do this manipulation of the options, couldn't we simply have a couple of configs that we could use (two-chain, three-chain, whatever) where we use two-chain by default and we can just have something like s.UseTestConfig(testsuite.ThreeChainConfig). It would make the intent a little clearer (and shorter). I guess it depends how much we will use different configs, but just an idea.

(This is more on side of this PR, just wanted to write it while I had it in mind...)

chatton · 2024-06-11T09:28:52Z

@gjermundgaraba I created a re-usable function ThreeChainSetup that can be passed in like

relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, s.TransferChannelOptions(), testsuite.ThreeChainSetup())

How does this look to you?

gjermundgaraba · 2024-06-11T09:50:49Z

@gjermundgaraba I created a re-usable function ThreeChainSetup that can be passed in like
relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, s.TransferChannelOptions(), testsuite.ThreeChainSetup())
How does this look to you?

Yeah, that reads much better :)

chatton · 2024-06-11T10:28:50Z

the new way broke the setup, looking into it now

…-be-able-to-spin-up-3-or-more-chains

colin-axner

ACK, thanks @chatton for updating this flow! Happy with the changes and we can iterate as needed

colin-axner · 2024-06-11T15:15:39Z

e2e/testsuite/testsuite.go

@@ -240,8 +292,9 @@ func (s *E2ETestSuite) GetPathName(idx int64) string {
 }

 // generatePath generates the path name using the test suites name
-func (s *E2ETestSuite) generatePath(ctx context.Context, ibcrelayer ibc.Relayer) string {
-	chainA, chainB := s.GetChains()
+func (s *E2ETestSuite) generatePath(ctx context.Context, ibcrelayer ibc.Relayer, chainAIdx, chainBIdx int) string {


Could we update godoc? The idea here is that if we want to generate a path between chainA and chainC, we provide the args 0, 2?

colin-axner · 2024-06-11T15:17:05Z

e2e/testsuite/testsuite.go

@@ -143,13 +131,76 @@ func (s *E2ETestSuite) GetRelayerUsers(ctx context.Context, chainOpts ...ChainOp
 // with E2ETestSuite.StartRelayer if needed.
 // This should be called at the start of every test, unless fine grained control is required.
 func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channelOpts func(*ibc.CreateChannelOptions), chainSpecOpts ...ChainOptionConfiguration) (ibc.Relayer, ibc.ChannelOutput) {
-	chainA, chainB := s.GetChains(chainSpecOpts...)
-	r := s.ConfigureRelayer(ctx, chainA, chainB, channelOpts)


do we still need the ConfigureRelayer function if its contents have been moved here?

we still need it for now as it's used in the wasm tests here

This can be refactored in a follow up I think.

colin-axner · 2024-06-11T15:18:25Z

e2e/testsuite/testsuite.go

+	channelOptions := ibc.DefaultChannelOpts()
+	// For now, set the version to the latest transfer module version
+	// DefaultChannelOpts uses V1 at the moment
+	channelOptions.Version = transfertypes.V2


the previous code path handled backwards compatibility, this is still needed?

good catch, will pull in those changes here

colin-axner · 2024-06-11T15:20:14Z

e2e/testsuite/testsuite.go

+		var chainHeighters []test.ChainHeighter
+		for _, c := range chains {
+			chainHeighters = append(chainHeighters, c)
+		}
+


what are chain heighters?

it's a type defined in interchain test which is an argument for the WaitForBlocks function. the ibc.Chain interface implemnts the ChainHeighter interface, however it does not seem to be possible to pass in a list of ibc.Chain.

For example this is valid

s.Require().NoError(test.WaitForBlocks(ctx, 10, chains[0], chains[1]), "failed to wait for blocks")

but this in invalid

s.Require().NoError(test.WaitForBlocks(ctx, 10, chains...), "failed to wait for blocks")

A conversion to the interface seems to be required for variable argument functions.

colin-axner · 2024-06-11T15:24:06Z

e2e/tests/transfer/forwarding_test.go

+	testsuite.E2ETestSuite
+}
+
+// TODO: replace this with actual tests


issue link?

colin-axner · 2024-06-11T15:24:39Z

e2e/tests/transfer/forwarding_test.go

+	ctx := context.TODO()
+	t := s.T()
+
+	// TODO: note the ThreeChainSetup fn needs to be passed to TransferChannelOptions since it calls


issue link?

…-be-able-to-spin-up-3-or-more-chains

chatton · 2024-06-12T11:01:10Z

e2e/testsuite/testsuite.go

+func defaultChannelOpts(chains []ibc.Chain) ibc.CreateChannelOptions {
+	channelOptions := ibc.DefaultChannelOpts()
+	channelOptions.Version = determineDefaultTransferVersion(chains)
+	return channelOptions
+}
+
+// determineDefaultTransferVersion determines the version of transfer that should be used with an arbitrary number of chains.
+// the default is V2, but if any chain does not support V2, then V1 is used.
+func determineDefaultTransferVersion(chains []ibc.Chain) string {
+	for _, chain := range chains {
+		chainVersion := chain.Config().Images[0].Version
+		if !testvalues.ICS20v2FeatureReleases.IsSupported(chainVersion) {
+			return transfertypes.V1
+		}
+	}
+	return transfertypes.V2
+}


I refactored the code which determines the default transfer v2 version to account for multiple chains. I think this reads a lot nicer and is easier to reason about.

cc @colin-axner

DimitrisJim · 2024-06-12T11:03:00Z

e2e/testsuite/testsuite.go

+	chains := s.GetAllChains()
+	chainA, chainB := chains[0], chains[1]


isn't this still equivalent to s.GetChains?

yes, I think I refactored this to remove get chains then actually re-added it to cause less disruption in existing tests

ah, idea is to completely rm afterwards? Fine with that approach, the flow of GetAllChains and then grabbing what you need serves as a guide of how many chains you'll use.

DimitrisJim

wizardly as always! fine to address minor things in follow ups!

…-be-able-to-spin-up-3-or-more-chains

sonarqubecloud · 2024-06-12T13:26:43Z

Quality Gate passed for 'ibc-go'

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

chatton added 5 commits June 5, 2024 17:01

wip: adding additional function to return all chains associated with …

5b53d18

…a test

chore: existing tests passing

123b4f7

chore: replace chain spec references with chain spec list indexing

31c5ff4

chore: refactor SetupChainsRelayerAndChannel to construct interchain …

0b3a7f4

…based on arbitrary number of chains

chore: add basic test to verify 3 chain setup

4495ffb

chatton commented Jun 6, 2024

View reviewed changes

chatton added 3 commits June 6, 2024 11:49

chore: add e2e build flag

7b590c0

Merge branch 'main' into cian/issue#6242-update-e2e-infrastructure-to…

d54eeee

…-be-able-to-spin-up-3-or-more-chains

Merge branch 'main' into cian/issue#6242-update-e2e-infrastructure-to…

c639653

…-be-able-to-spin-up-3-or-more-chains

chatton marked this pull request as ready for review June 6, 2024 14:52

chatton requested review from colin-axner, AdityaSripal, crodriguezvega, damiannolan and DimitrisJim as code owners June 6, 2024 14:52

chatton changed the title ~~Update E2E infra to support more than 2 chains~~ [DONT REVIEW] Update E2E infra to support more than 2 chains Jun 6, 2024

chore: corrected proposal id init

726347f

chatton changed the title ~~[DONT REVIEW] Update E2E infra to support more than 2 chains~~ Update E2E infra to support more than 2 chains Jun 6, 2024

Merge branch 'main' into cian/issue#6242-update-e2e-infrastructure-to…

8d82134

…-be-able-to-spin-up-3-or-more-chains

chatton requested a review from gjermundgaraba as a code owner June 11, 2024 08:20

gjermundgaraba approved these changes Jun 11, 2024

View reviewed changes

chatton added 2 commits June 11, 2024 10:17

chore: fix build errors

8ec8ebc

chore: extract setup fn into testsuite package for re-use

ad05c78

chatton added 2 commits June 11, 2024 11:48

chore: fix chain construction error

e014890

Merge branch 'main' into cian/issue#6242-update-e2e-infrastructure-to…

9720cca

…-be-able-to-spin-up-3-or-more-chains

chatton added the priority PRs that need prompt reviews label Jun 11, 2024

colin-axner approved these changes Jun 11, 2024

View reviewed changes

chatton added 2 commits June 12, 2024 11:37

Merge branch 'main' into cian/issue#6242-update-e2e-infrastructure-to…

f696eed

…-be-able-to-spin-up-3-or-more-chains

chore: addressing PR feedback

be5a86e

chatton commented Jun 12, 2024

View reviewed changes

DimitrisJim reviewed Jun 12, 2024

View reviewed changes

DimitrisJim approved these changes Jun 12, 2024

View reviewed changes

Merge branch 'main' into cian/issue#6242-update-e2e-infrastructure-to…

dbc1c14

…-be-able-to-spin-up-3-or-more-chains

chatton added this pull request to the merge queue Jun 12, 2024

Merged via the queue into main with commit 2b887d7 Jun 12, 2024
87 of 89 checks passed

chatton deleted the cian/issue#6242-update-e2e-infrastructure-to-be-able-to-spin-up-3-or-more-chains branch June 12, 2024 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update E2E infra to support more than 2 chains #6524

Update E2E infra to support more than 2 chains #6524

chatton commented Jun 6, 2024 •

edited

Loading

coderabbitai bot commented Jun 6, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

chatton Jun 6, 2024 •

edited

Loading

gjermundgaraba Jun 11, 2024

chatton Jun 11, 2024

gjermundgaraba Jun 11, 2024

chatton commented Jun 11, 2024

gjermundgaraba commented Jun 11, 2024

chatton commented Jun 11, 2024

colin-axner left a comment

colin-axner Jun 11, 2024

colin-axner Jun 11, 2024

chatton Jun 12, 2024

colin-axner Jun 11, 2024

chatton Jun 12, 2024

colin-axner Jun 11, 2024

chatton Jun 12, 2024

colin-axner Jun 11, 2024

chatton Jun 12, 2024

colin-axner Jun 11, 2024

chatton Jun 12, 2024

chatton Jun 12, 2024

DimitrisJim Jun 12, 2024

chatton Jun 12, 2024

DimitrisJim Jun 12, 2024

DimitrisJim left a comment

sonarqubecloud bot commented Jun 12, 2024

		chains := s.GetAllChains()
		chainA, chainB := chains[0], chains[1]

Update E2E infra to support more than 2 chains #6524

Update E2E infra to support more than 2 chains #6524

Conversation

chatton commented Jun 6, 2024 • edited Loading

Description

coderabbitai bot commented Jun 6, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

chatton Jun 6, 2024 • 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

chatton commented Jun 11, 2024

gjermundgaraba commented Jun 11, 2024

chatton commented Jun 11, 2024

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

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

DimitrisJim left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 12, 2024

Quality Gate passed for 'ibc-go'

chatton commented Jun 6, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

chatton Jun 6, 2024 •

edited

Loading