Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBC Relayer Integration Test Suite #1521

Merged
merged 118 commits into from
Nov 18, 2021
Merged

IBC Relayer Integration Test Suite #1521

merged 118 commits into from
Nov 18, 2021

Conversation

soareschen
Copy link
Contributor

@soareschen soareschen commented Oct 29, 2021

Closes #797

Description

A preview for the documentation is available here: https://lucid-hamilton-df13e0.netlify.app/ibc_integration_test/


For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@soareschen
Copy link
Contributor Author

I have added a command line wrapper test_setup_with_binary_channel that sets up the test chains as a CLI instead of test.

When the command is executed, you should see log messages such as
following near the end:

$ cargo run --bin test_setup_with_binary_channel
...
INFO ibc_integration_test::framework::binary::channel: written channel environment to /path/to/ibc-rs/data/test-3742758098/binary-channels.env
WARN ibc_integration_test::util::suspend: suspending the test indefinitely. you can still interact with any spawned chains and relayers

The binary-channels.env file generated contains the environment variables
that are essential for accessing the test chains. You can source it and
run the relayer commands in a separate terminal such as:

$ source /path/to/ibc-rs/data/test-1790156739/binary-channels.env
$ cargo run --bin hermes -- -c $RELAYER_CONFIG tx raw ft-transfer \
    $CHAIN_ID_B $CHAIN_ID_A $PORT_A $CHANNEL_ID_A 9999 -o 1000 \
    -k $NODE_A_WALLETS_USER1_KEY_ID -d $NODE_A_DENOM

More info at https://lucid-hamilton-df13e0.netlify.app/test_setup_with_binary_channel/index.html.

@@ -0,0 +1,13 @@
/*!
Experimental work to define test cases with N-ary chains.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the N-ary test chains implementation in this PR as well, but hide is behind an "experimental" flag so that we can still merge this even though it is not too ready for review yet due to documentation not yet being written.

Alternatively I can move the N-ary modules into a separate draft PR so that they can be reviewed separately.

@soareschen
Copy link
Contributor Author

soareschen commented Nov 16, 2021

I have just updated the bootstrap chain code so that the client/connection/channel IDs generated are random between 1-8. When you run the tests you should see the logs such as:

INFO ibc_integration_test::bootstrap::binary::channel: created new channel chain/client/connection/channel from ibc-alpha-a58a63ff/07-tendermint-3/connection-2/channel-4 to ibc-beta-c83842bf/07-tendermint-7/connection-3/channel-3
INFO ibc_integration_test::framework::binary::channel: written channel environment to /path/to/ibc-rs/tools/integration-test/data/test-2710114153/binary-channels.env

You can then run source /path/to/ibc-rs/tools/integration-test/data/test-2710114153/binary-channels.env to expose the environment variables on your terminal to further test on the relayer manually. The current exported environment variables look like:

CHAIN_ID_A=ibc-alpha-a58a63ff
CHAIN_ID_B=ibc-beta-c83842bf
CHANNEL_ID_A=channel-4
CHANNEL_ID_B=channel-3
CLIENT_ID_A=07-tendermint-3
CLIENT_ID_B=07-tendermint-7
CONNECTION_ID_A=connection-2
CONNECTION_ID_B=connection-3
NODE_A_CMD=gaiad
NODE_A_DENOM=coind964d90a
NODE_A_GRPC_ADDR=http://localhost:28406
NODE_A_HOME=/path/to/ibc-rs/tools/integration-test/data/test-2710114153/ibc-alpha-a58a63ff
NODE_A_RPC_ADDR=http://localhost:49311
NODE_A_WALLETS_RELAYER_ADDRESS=cosmos1ntzvftcteeqdxmfa9ayzyeefem4sna3s84yctm
NODE_A_WALLETS_RELAYER_KEY_ID=relayer-45936844
NODE_A_WALLETS_USER1_ADDRESS=cosmos1mk42l0707a4jqnv3d7ne6hx6yu55nv4t3cd8ua
NODE_A_WALLETS_USER1_KEY_ID=user1-c3e5547
NODE_A_WALLETS_USER2_ADDRESS=cosmos1nsql3jg9hn5aqv7v9a7hhax5dr6y5l0geesh3g
NODE_A_WALLETS_USER2_KEY_ID=user2-699178b4
NODE_A_WALLETS_VALIDATOR_ADDRESS=cosmos14z9fyeyp5yfz7jh4t0w58stxll0uyveeezfpat
NODE_A_WALLETS_VALIDATOR_KEY_ID=validator-2e639555
NODE_B_CMD=gaiad
NODE_B_DENOM=coin86c992ce
NODE_B_GRPC_ADDR=http://localhost:26583
NODE_B_HOME=/path/to/ibc-rs/tools/integration-test/data/test-2710114153/ibc-beta-c83842bf
NODE_B_RPC_ADDR=http://localhost:31734
NODE_B_WALLETS_RELAYER_ADDRESS=cosmos1xtm2x3x9xv2vg8n5cy3lr29nqzjjc77jxegfhe
NODE_B_WALLETS_RELAYER_KEY_ID=relayer-2335cf72
NODE_B_WALLETS_USER1_ADDRESS=cosmos1p2e6cvqv2f2u0hucn5rh2alap06mcwd7m2m0pl
NODE_B_WALLETS_USER1_KEY_ID=user1-227c4619
NODE_B_WALLETS_USER2_ADDRESS=cosmos15twlf3t6777qhwr0jpqfn7ukzc2rkvxv79ewuz
NODE_B_WALLETS_USER2_KEY_ID=user2-8e72a99e
NODE_B_WALLETS_VALIDATOR_ADDRESS=cosmos1zh66gag8nlq5mz8cs3h54mld5tv0r2r5y9ct2a
NODE_B_WALLETS_VALIDATOR_KEY_ID=validator-48c6bb76
PORT_A=transfer
PORT_B=transfer
RELAYER_CONFIG=/path/to/ibc-rs/tools/integration-test/data/test-2710114153/relayer-config.toml

@adizere adizere changed the title IBC Relayer End-to-End Test Suite IBC Relayer Integration Test Suite Nov 16, 2021
@adizere
Copy link
Member

adizere commented Nov 16, 2021

I have just updated the bootstrap chain code so that the client/connection/channel IDs generated are random between 1-8. When you run the tests you should see the logs such as:

Cool! I would propose we stop adding features here and focus on cleaning up & merging. New features to the testing framework would ideally be motivated directly by specific test cases.

@adizere adizere linked an issue Nov 16, 2021 that may be closed by this pull request
5 tasks
@soareschen soareschen mentioned this pull request Nov 16, 2021
5 tasks
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Incredible work, @soareschen! Such a beautiful infrastructure, and many thanks for the extensive comments and documentation :)

I have all but two very minor nitpicks:

  1. Binary is used in a bunch of places to mean two, and given that binary is already an overloaded term in CS, what do you think of just using two instead? Though this may be moot after N-ary Test Chains Support #1572 lands?

  2. In the rest of the ibc-rs project, we use /// as a doc comment prefix whereas most of the new code here uses /**, which introduces an inconsistency (and gives me Java flashbacks but that's not really the point). Can we harmonize this by switching over to ///?

Again, amazing work!

PS: Happy to see mononym being put to good use already

@soareschen
Copy link
Contributor Author

soareschen commented Nov 18, 2021

Binary is used in a bunch of places to mean two, and given that binary is already an overloaded term in CS, what do you think of just using two instead? Though this may be moot after #1572 lands?

Yeah I agree that using the word "binary" is not very fitting here, though I can't think of a better term especially considering that we need a good correspondence to the 'N-ary" setup as well.

The most common place where test writers are going to use it is through the traits like BinaryChannelTest or NaryChannelTest. Do you think referring them as TwoChannelTest and MultiChannelTest would be better?

In the rest of the ibc-rs project, we use /// as a doc comment prefix whereas most of the new code here uses /**, which introduces an inconsistency (and gives me Java flashbacks but that's not really the point). Can we harmonize this by switching over to ///?

I would be against using /// for doc comments, as they are very inconvenient for writing new comments, and even more annoying for editing existing comments. If I need to wrap around comment lines due to line limits, the /// blocks would get in the way of me combining the two lines. In the long term this would just discourage developers to write or edit comments, which is bad for the project.

Again, amazing work!

PS: Happy to see mononym being put to good use already

Thanks! Actually tagged identifiers are different from named values in mononym. In the case of ibc-rs, the tags we use are non-unique and are only there to prevent accidental mix ups. But the tagged constructs does not prevent programmers from assigning the wrong tags to the data types, and so there are still chance for mistakes to be made.

@romac romac dismissed adizere’s stale review November 18, 2021 14:27

Review comments have been addressed

@romac romac merged commit 64fb5e8 into master Nov 18, 2021
@romac romac deleted the soares/integration-test branch November 18, 2021 14:29
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* WIP relayer test

* Bootstrap chain almost working

* Refactor chain bootstrap code

* Trying to get create client work.

* Test create client is now working

* Fix formatting

* WIP IBC packet transfer

* Got IBC transfer working

* Refactor supervisor and denom tracing

* More refactoring

* More refactoring

* Refactor build_and_send_transfer_messages

* Round trip IBC token transfer is now working

* Refactor channel bootstrap code

* Add tagged chain handle wrapper

* Rename ChainCommand to ChainDriver

* Make bootstrap pair return impl ChainHandle

* Add tagged types

* Refactor bootstrap pair

* Tag ChainDriver

* Add mono tag, refactor single chain bootstrap

* More refactoring

* More refactoring

* Fix chain tags

* Channel creation ordering is now consistent

* Test IBC transfer in both directions

* Fix monitor shutdown error

* Refactor wait wallet amount

* Make init script return test config. Add hang test helper

* Draft memo testing working

* Add boostrap_chain_and_channel_pair

* Add memo test as separate test

* Disable health check in tests to suppress version errors from gaiad in Nix

* Add CI workflow

* Use experimental Nix with flakes

* Refactor test cases to use composable traits

* Use NoTestConfig wrapper type for test defaults

* Reorder code definitions

* Try out new way to override test config and implement override defaults

* Preemptively shutdown supervisor at the end of chain tests

* Rearrange type definitions

* Rename framework traits

* Rename and move override traits

* Rename ChainDeployment to ConnectedChains

* Refactor chain bootstrap

* Add BinaryNodeTest trait

* Add alpha- and beta- prefix to binary chain nodes

* Add HANG_ON_FAIL test hooks

* Add non-owned BinaryNodeTest

* Refactor Supervisor to use shared reference to chain registry

* Spawn supervisor in OwnedBinaryChainTest

* New test override approach with separate override argument

* Clean up overrides design

* Fix cargo doc

* Save relayer config onto file system

* Simplify test overrides

* Use absolute path for data directory path

* Adding documentation

* Add documentation for `framework` module

* Adding more documentation

* Reorganize test data directory

* Add documentation for `bootstrap` module

* Reorganize tagged module

* Add documentation for MonoTagged

* Refactor tagged methods into traits

* Add documentation for DualTagged

* Finish documenting most constructs

* Move example documentation

* Rename hang to suspend

* Fix example doctest

* Add more detailed documentation for DualTagged::contra_map

* Remove ChainClientServer and inline the handle and node fields directly

* Add documentation for connected chains and channels

* Finish documenting everything

* Only check for version in health check.

Failure in parsing chain version should not result in failure of
the entire relayer.

* Stop chain handle when it is dropped

* Add simulation manual test

* Add documentation for new constructs

* Move manual tests into the tests::manual module

* Add test framework for running tests with self-connected chains

* Add NodeConfigOverride for overriding full node config

* Draft definition for N-ary connected chains using const generics

* Implement N-ary bootstrap chains code

* Done initial bootstrap code for N-ary channels.

* Refactor to allow dynamic sized n-ary chains

* Running binary IBC transfer test as n-ary test successful

* Add environment exporter to test

* Separate drop handlers from chain and node types

* Remove reference version of test traits

* Remove "Owned" and "owned_" prefixes

* Put N-ary chain features behind "experimental" feature flag

* Clean up cargo configuration, address review feedback

* Fix build error

* Fix doctests

* Add custom Default instance for ModeConfig which enables everything by default

* Follow default ModeConfig as specified in informalsystems#1539

* Add test_setup_with_binary_channel command wrapper

* Fix clippy

* Fix unit test CI

* Randomize created client and connection ids

* Randomize channel IDs when bootstrapping channels

* Move bootstrap connection code to separate module

* Add back child process as a shared Arc pointer in FullNode

* Move experimental N-ary test chains to separate PR

* Fill out missing documentation

* Move N-ary chain constructs to informalsystems#1572

* Minor fix documentation

* Add a bit more documentation to tagged identifiers

* Slightly clean up bootstrap logs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port the Python end-to-end integration test suite to Rust
3 participants