Skip to content
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

Allow lower case for abci::data #17

Merged
merged 3 commits into from
Sep 23, 2019

Conversation

amanusk
Copy link
Contributor

@amanusk amanusk commented Aug 19, 2019

While for fmt both lower case and upper case encoding are possible:
https://github.com/interchainio/tendermint-rs/blob/b88febc173ece044da043b98469f3faa110ef41e/src/abci/data.rs#L44

When decoding upper case from rpc response, there is an error:

---- endpoints::broadcast_tx_sync stdout ----
thread 'endpoints::broadcast_tx_sync' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: ParseError, message: "Parse error. Invalid JSON", data: Some("bad encoding at line 6 column 24") }', src/libcore/result.rs:1084:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

@ebuchman
Copy link
Member

ebuchman commented Sep 7, 2019

Do we need to be using subtle_encoding for the hex here? The hex crate handles upper case and lower case: https://docs.rs/hex/0.3.2/hex/fn.decode.html

@ebuchman
Copy link
Member

ebuchman commented Sep 7, 2019

Oh I see, subtle_encoding is Tony's crate - @tony-iqlusion would it be better to only use it when we're dealing with keys ?

@tarcieri
Copy link
Contributor

tarcieri commented Sep 7, 2019

@ebuchman you can add another crate if you want, but note that subtle-encoding also handles upper and lower case hex, and already supports either via the Data type's FromStr impl:

https://github.com/interchainio/tendermint-rs/blob/566dfb6/src/abci/data.rs#L42

To use it with serde, change the Deserialize impl to something like:

impl<'de> Deserialize<'de> for Data {
    fn deserialize<D: de::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
        use de::Error;
        String::deserialize(deserializer)?.parse().map_err(D::Error::custom)
    }
}

@amanusk
Copy link
Contributor Author

amanusk commented Sep 10, 2019

Thanks @tarcieri, updated the method as you suggested.
Although unlikely, there can still be an error if the data is encoded with both lower and upper case chars.

@ebuchman ebuchman merged commit 06eab13 into informalsystems:master Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants