rpc: incompatibilities

[liamsi]

I've tested the (ignored) integration tests for rpc against a tendermint 0.32.7 version and the following tests fail:

abci_info and genesis

• genesis fails with: called Result::unwrap() on an Err value: Error { code: ParseError, message: "Parse error. Invalid JSON", data: Some("invalid digit found in string at line 11 column 26") }
• abci_info with: called Result::unwrap() on an Err value: Error { code: ParseError, message: "Parse error. Invalid JSON", data: Some("missing field last_block_height at line 9 column 5") }

#85 (comment)

I've also noted that example responses on https://docs.tendermint.com/master/rpc/#/ have a numeric id (every JSON rpc request comes with such an id):

{
  "jsonrpc": "2.0",
  "id": 0,
  "result": {

while on my local 0.32.7 returns a string based id "id": "",.

We should make sure to support both as the spec says:

An identifier established by the Client that MUST contain a String, Number, or NULL value if included.

https://www.jsonrpc.org/specification#request_object

Compatibility with some tendermint version is also relevant in the context of a light client making requests via rpc.

[tarcieri]

@liamsi can we figure out a way to run the currently #[ignore]d integration tests in CI? I think that'd be the ideal way to spot incompatibilities

[liamsi]

Yes, that would be ideal! A naive approach that should work: grab a docker image (with the latest / a pinned tendermint version), pull in tendermint-rs source, spin up a single tendermint node and run the ignored tests against this node (via cargo test -- --ignored). Wasn't there a script that done exactly this in the past?

It makes sense to sync with @greg-szabo and @thanethomson on this too. Not likely that I'll work on this this year but I I'll open a PR with a fix for the regressions.

[liamsi]

#169 surfaced new incompatibilities with the latest tendermint version.

[liamsi]

As mentioned above the spec defines:

An identifier established by the Client that MUST contain a String, Number, or NULL value if included.

https://www.jsonrpc.org/specification#request_object

I'll go on and change the Id to be a enum to match above behaviour instead of currently a String only id: https://github.com/interchainio/tendermint-rs/blob/471ac8faa79c3072a220e09228bffae157fab2f0/tendermint/src/rpc/id.rs#L8

This resurfaced with: #181

[greg-szabo]

I think this has been fixed with #183. The resolution (for 0.32) was to introduce the Rust version of omitempty for the structures.

Oh, I see the id thing is mentioned here too. That's not fixed yet. But CI works, so I'm guessing we're not testing for the id thingy?

[liamsi]

so I'm guessing we're not testing for the id thingy

Yes, the JSON-only tests still use a string based ID (see all files here). When I run a tendermint instance locally it returns an integer though (ad do all examples in https://docs.tendermint.com/master/rpc/). Interestingly, gaia seems to return a string based ID, e.g. see https://rpc.cosmos.network/commit?height=1

That said, we might not explicitly test for the ID thingy but it's effectively parsed on each test / rpc call.

[liamsi]

@liamsi Can we close this issue? Integration tests run and although they are not mandatory, they are a good indicator of issues.

As far as I can see all known issues are fixed. I have another comment on how to improve the integration test situation but I'll open a separate issue for this or use the #120 to track this.

[liamsi]

Closing