-
Notifications
You must be signed in to change notification settings - Fork 231
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
Fix abci_query RPC endpoint #77
Conversation
@mappum I'm beginning to think it might be good to allow a (also note I'm the one to blame for disallowing a height of 0, so I assume there's probably not going to be a lot of debate if you do change it as part of this PR) |
@tarcieri I added the 0-height change. However, it's still hard to use the integration tests because there are various differences between responses on different chains. It would be nice if it could be tested against an easy-to-spin-up local testnet (e.g. |
Great!
Yeah it's been a big pain point in the past. If you can think of a better strategy (particularly one that works in CI) I'd be all for it. |
I would vote for either making the integration test run something like this: tendermint init --home=/some/tmp/dir
tendermint node --home=/some/tmp/dir --proxy_app=kvstore # (or counter or something) (Then remove the Tendermint data after the tests run). This could either run a Until then, we could change the integration tests to target this kind of network but still expect the user to run it manually. |
@mappum there's a bit of prior art for this with the test harness used by the KMS: You could potentially try to use the same Docker image on CircleCI? |
@tony-iqlusion I can make that change in a later PR, for now I changed the integration tests to pass when using a local node for a throwaway network. Are you OK with merging this change? (I'd very much like to use this fix so I can use the RPC client). |
I realized the encoding for the response should be changed too, since Tendermint always converts the Let me know if I should make any other changes. |
@mappum looks like it needs a GPG signature |
Signed last commit. |
@mappum looks like it requires all commits to be signed. Perhaps squash and sign the resulting commit. |
Made the changes, will wait to squash so it's easier to review. |
Now all comments are addressed and all commits signed. |
|
This is a fix for #75, although it's not passing the integration test yet since Tendermint is responding with "0" for the
height
field butblock::Height
enforces that height is >= 1. Other unrelated integration tests are also failing, other RPC request/response types are possibly out of date or just overly strict.