rpc: incompatibilities about tendermint-rs HOT 8 CLOSED

@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

from tendermint-rs.

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.

from tendermint-rs.

#169 surfaced new incompatibilities with the latest tendermint version.

from tendermint-rs.

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

from tendermint-rs.

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?

from tendermint-rs.

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.

from tendermint-rs.

@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.

from tendermint-rs.

Closing

from tendermint-rs.