RPC: Update hash to 32 byte value. #14
Conversation
Update tests to new standard
amanusk
force-pushed
the
rpc-client-hash-32
branch
from
dd2ea8b
to
ce4ebc1
Compare
|
Running the simple kvstore example, I am getting an error on parsing the result It seems hashes have been update from 20 to 32 bytes. https://github.com/tendermint/tendermint/blob/master/CHANGELOG.md#v0260 This PR updates the hash and the tests |
|
@ebuchman what are your thoughts on compatibility here? should we just target the latest Tendermint release only? |
Yes, probably for now. We'll figure out a backwards compatibility story a bit later on ... |
| @@ -87,8 +87,8 @@ impl<'de> Deserialize<'de> for Code { | |||
| where | |||
| E: serde::de::Error, | |||
| { | |||
| match val.parse::<u32>() { | |||
| Ok(val) => self.visit_u32(val), | |||
| match val.parse::<u64>() { | |||
There was a problem hiding this comment.
For better readability.
This is related to a previous PR #13
In either cases visit_u64 method is called, so thought it would be better if it is explicit in the code.
| @@ -11,7 +11,7 @@ use subtle::{self, ConstantTimeEq}; | |||
| use subtle_encoding::hex; | |||
|
|
|||
| /// Size of a transaction hash in bytes | |||
| pub const LENGTH: usize = 20; | |||
| pub const LENGTH: usize = 32; | |||
There was a problem hiding this comment.
I introduced HASH_SIZE in https://github.com/interchainio/tendermint-rs/blob/master/src/merkle.rs#L6 ... we'll need to de-duplicate eventually but for now it's fine ....
| @@ -155,7 +155,7 @@ mod endpoints { | |||
|
|
|||
| assert_eq!( | |||
| &response.hash.to_string(), | |||
| "75CA0F856A4DA078FC4911580360E70CEFB2EBEE" | |||
| "88D4266FD4E6338D13B845FCF289579D209C897823B9217DA3E161936F031589" | |||
There was a problem hiding this comment.
Do the values of these not matter at all?
There was a problem hiding this comment.
As I understand, these tests only check that parsing work correctly, and do not test the actual correctness of the hash.
Also, the hash is of the sent transaction and not of the response right?
There was a problem hiding this comment.
Also, the hash is of the sent transaction and not of the response right?
That's right
|
Any other fixes required? |
Update tests to new standard