Use gRPC to query connection end

[soareschen]

Partial fix for: #1001

Description

This PR changes the query_connection implementation from querying the Tendermint RPC to querying the Cosmos SDK GRPC directly.

Because of cosmos/ibc-go#149, the height parameter is removed from query_connection. However this is fine, as shown in the diff, since all calls to query_connection other than the command line always use the latest height of the blockchain. As a result the change should not break any existing behavior.

We will need to implement retry logic any way in case a target height of interest have not been reach on the blockchain. For querying into the past, we can wait for cosmos/ibc-go#149 to be fixed first.

This fix is in parallel to #1042. The change to use GRPC call should make it less urgent to the fix by #1042. But handling uninitialized fields in protobuf3 is still an outstanding problem that needs to be solved eventually.

---

For contributor use:

• Updated the Unreleased section of CHANGELOG.md with the issue.
• 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.

[romac]

Looks good! Just a small nitpick and a question for @ancazamfir.

[romac]

@ancazamfir Is that ok with you, or do you sometimes rely on being able to query the connection end at specific heights?

[ancazamfir]

yes, we need to query specific heights (coming in the connection worker PR). And for this reason we cannot use the gRPC query.

[ancazamfir]

Is there an issue that describes the problem we are trying to fix here? This might be ok to be used by the CLIs only. But for relaying we do need to be able to query connection at height, take proofs at given height, etc. This is currently not supported by SDK gRPC implementation and for this reason we cannot use gRPC at this point (see cosmos/ibc-go#149). Also we want to be able to test the APIs the relayer needs via the CLIs. This is why many queries have the -h parameter that might look awkward.

[ancazamfir]

yes, we need to query specific heights (coming in the connection worker PR). And for this reason we cannot use the gRPC query.

[soareschen]

Is there an issue that describes the problem we are trying to fix here?

As discussed in #1042, doing this query over Tendermint RPC is not very reliable, because when the connection is not found, the RPC returns empty buffer instead of proper error code. So the long term solution is still to do the query over GRPC when possible.

yes, we need to query specific heights (coming in the connection worker PR)

Perhaps we can come back to this PR again when the connection worker PR is ready, so that we have a good use case of what is currently lacking on the GRPC API and push for the fix to be done sooner.

[adizere]

Goal:

• revert the APIs to have height param
• use gRPC with height param using @andynog's proposed solution
• implement the whole method inside impl Chain for CosmosSDK
• fix Romain's comment
• leave method in the impl Chain for CosmosSDK
  • Chain trait will ideally evolve to rely on gRPC calls

Future:

• think about refactoring the block_on awkward parts into separate async fn blocks

[soareschen]

I have now added back the height parameter using the x-cosmos-block-height header as documented at https://docs.cosmos.network/master/run-node/interact-node.html#query-for-historical-state-using-grpcurl.

[ancazamfir]

Looks good! Thanks @soareschen!