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

ci: Add ibc-rs/Hermes integration test #35

Merged
merged 8 commits into from
Jan 5, 2022

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Dec 22, 2021

Related to informalsystems/hermes#17.

See the README for details as to what this integration test does.

One major drawback to the generalized approach I've taken is that it's pretty slow to execute. In follow-up issues we can try to figure out ways to accelerate its execution (especially i.t.o. stashing build artifacts).

In terms of the large PR, half of it's just a Tendermint config.toml file that I bake into the image.

@thanethomson thanethomson marked this pull request as ready for review December 22, 2021 23:05
@thanethomson thanethomson changed the title ci: Add first draft integration testing config ci: Add ibc-rs/Hermes integration test Dec 22, 2021
@@ -0,0 +1,11 @@
#!/bin/bash
set -euo pipefail
Copy link
Member

Choose a reason for hiding this comment

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

Should this be called update-client (instead of channel)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I named it that. I think I may have been half asleep 😅 I've updated the whole test now to be called create-connection.sh, and it instead executes a whole create connection operation as per the work in #20.

ci/README.md Outdated
ID `basecoin-0`).
7. If no `CMD` arguments are provided for the container, it will automatically
execute the [`update-channel.sh`](./tests/update-channel.sh) script, which
creates and updates an IBC channel between `basecoin-0` and `ibc-0`. If `CMD`
Copy link
Member

Choose a reason for hiding this comment

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

I guess the plan is to eventually go all the way up to channels, but at the moment we stop at clients because connection handshake fails (cf. https://github.com/informalsystems/ibc-rs/issues/1710)?

Copy link
Member

Choose a reason for hiding this comment

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

This is a good point, but ATM we've been pinning ibc deps to specific branches (e.g. basecoin/phase-4-1 or basecoin/phase-5) in Cargo.toml. This was done to account for missing non-SDK chain support in ibc-rs. Now that proof verification and other things like custom proof-spec support have been implemented, we can finally start to depend on ibc-rs releases (once the fix for https://github.com/informalsystems/ibc-rs/issues/1710 is merged).
That said, we still haven't merged #27 yet, so I suggest we only run hermes create connection for now.

ci/Dockerfile Show resolved Hide resolved
HERMES_BIN=${HERMES_BIN:-${HOME}/build/ibc-rs/release/hermes}

echo "Creating client..."
"${HERMES_BIN}" tx raw create-client basecoin-0 ibc-0
Copy link
Member

Choose a reason for hiding this comment

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

Can we also create a client in the opposite direction, namely a client hosted on ibc-0 for chain basecoin-0?

Probably:

"${HERMES_BIN}" tx raw create-client ibc-0 basecoin-0
"${HERMES_BIN}" tx raw update-client ibc-0 07-tendermint-0

Copy link
Contributor Author

@thanethomson thanethomson Dec 23, 2021

Choose a reason for hiding this comment

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

I've just added a commit now to create a connection in the opposite direction, but @hu55a1n1 mentioned here that it may intermittently fail. Does that still hold true @hu55a1n1?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, that comment is outdated. 😅 The connection creation shouldn't fail now. 🤞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, looks like the bidirectional connection integration test passes 😁 👍

ci/Dockerfile Outdated
@@ -0,0 +1,45 @@
ARG TENDERMINT_VERSION=0.34.9
ARG GAIA_VERSION=4.2.1
Copy link
Member

Choose a reason for hiding this comment

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

It may be better to use Gaiad v5 or v6 (mainnet hub-4 is at v6).

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 can currently only get this working for Gaiad v5.0.5 (that's the latest image we have from cephalopodequipment in the v5.x series). v6.0.0 fails at the point where I try to add the generated user key to the basecoin-0 chain via Hermes:

Configuring Hermes...
2021-12-23T14:10:10.979891Z TRACE ThreadId(01) deserialized the encoded pub key into a ProtoAny of type '/cosmos.crypto.secp256k1.PubKey'
Success: Added key 'testkey' (cosmos1va7pcr6gt6p8vvyexrmzwrm6k8uuc7rq2z96w0) on chain ibc-0
Generating user key...
{"name":"user","type":"local","address":"cosmos177vu4gpafpzd9f39kwc64m3tevk628dr3xe6d0","pubkey":"{\"@type\":\"/cosmos.crypto.secp256k1.PubKey\",\"key\":\"A1QTDg0L9pZeHs0WHf73tTjyUBa8UI2Z931CHjUCSCVN\"}","mnemonic":"safe defense winter glide ladder belt clip inform they forest utility record feature cannon deliver stomach will mean destroy border matter sleep gospel involve"}
Adding user key to basecoin-0 chain...
Error: error encoding key: EOF while parsing a value at line 1 column 0

I'm assuming the serialization format changed between Gaiad v5 and v6?

Copy link
Member

Choose a reason for hiding this comment

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

Just tested this and it seems gaiad-v0.6.0 writes the output of the keys add command to stderr instead of stdout, so the user_seed.json file is empty leading to this problem.

@adizere
Copy link
Member

adizere commented Dec 23, 2021

I tried to run locally

docker run --rm -it \
     -v `pwd`:/src/basecoin-rs \
     -v /Users/adi/Hammers/ibc-rs/:/src/ibc-rs \
     informaldev/basecoin-rs-ci

and got this error:

Creating client...
2021-12-23T12:03:25.432795Z INFO ThreadId(01) using default configuration from '/basecoin/.hermes/config.toml'
2021-12-23T12:03:25.453362Z TRACE ThreadId(04) light client verification trusted=0-8 target=0-8
2021-12-23T12:03:25.461979Z DEBUG ThreadId(07) send_messages_and_wait_commit with 1 messages
2021-12-23T12:03:25.485075Z DEBUG ThreadId(07) [basecoin-0] send_tx: retrieved account sequence=0 number=0
2021-12-23T12:03:25.485991Z DEBUG ThreadId(07) [basecoin-0] send_tx: sending 1 messages using account sequence 0
2021-12-23T12:03:25.486916Z DEBUG ThreadId(07) [basecoin-0] send_tx: max fee, for use in tx simulation: Fee { amount: "401stake", gas_limit: 400000 }
2021-12-23T12:03:25.506339Z WARN ThreadId(07) [basecoin-0] estimate_gas: tx simulation successful but no gas amount used was returned, falling back on default gas: 400000
2021-12-23T12:03:25.506765Z DEBUG ThreadId(07) [basecoin-0] send_tx: using 400000 gas, fee Fee { amount: "401stake", gas_limit: 400000 }
2021-12-23T12:03:25.512568Z DEBUG ThreadId(07) [basecoin-0] send_tx: broadcast_tx_sync: Response { code: Ok, data: Data([]), log: Log(""), hash: transaction::Hash(225031418EE1D77A587E7C345BF92133D78752AD6A629CBF29370833F723A774) }
2021-12-23T12:03:25.512664Z INFO ThreadId(07) [basecoin-0] waiting for commit of tx hashes(s) 225031418EE1D77A587E7C345BF92133D78752AD6A629CBF29370833F723A774
2021-12-23T12:03:26.020330Z TRACE ThreadId(07) [basecoin-0] wait_for_block_commits: retrieved 1 tx results after 2 tries (306ms)
Success: ChainError(
"deliver_tx for 225031418EE1D77A587E7C345BF92133D78752AD6A629CBF29370833F723A774 reports error: code=Err(2), log=Log("deliver failed with error: no module could handle specified message\n\nLocation:\n /usr/local/cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/flex-error-0.4.4/src/tracer_impl/eyre.rs:10:9")",
)

Note that my ibc-rs directory was checked-out on master. I wonder what did I do wrong.

@thanethomson
Copy link
Contributor Author

thanethomson commented Dec 23, 2021

Note that my ibc-rs directory was checked-out on master. I wonder what did I do wrong.

Check out branch basecoin/phase-4-1 first and try again? This is the one I currently use in the integration test in CI until such time that all the basecoin-related work's been merged into ibc-rs.

echo "Generating user key..."
gaiad keys add user --keyring-backend="test" --output json > "${HOME}/user_seed.json"
echo "Adding user key to basecoin-0 chain..."
"${HERMES_BIN}" -c "${HERMES_CONFIG}" keys add basecoin-0 -f "${HOME}/user_seed.json"
Copy link
Contributor Author

@thanethomson thanethomson Dec 23, 2021

Choose a reason for hiding this comment

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

This command specifically fails for Gaiad v6.0.0, and where I suspect the serialization in ibc-rs may need to be updated. cc @adizere @hu55a1n1

Copy link
Member

@hu55a1n1 hu55a1n1 left a comment

Choose a reason for hiding this comment

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

Thanks, @thanethomson! Awesome stuff! 💯👌

@hu55a1n1 hu55a1n1 merged commit 920a7c4 into main Jan 5, 2022
@hu55a1n1 hu55a1n1 deleted the thane/17-integration-testing branch January 5, 2022 15:40
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.

3 participants