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

abci: fix rpc response deserialization #432

Merged
merged 1 commit into from
Jul 11, 2020

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jul 10, 2020

Fix rpc response deserialization issues. fix #423

By the way, the null_as_default deserializer might be good to use on some other fields.

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the concise change-set. Could you explain why the null as default behaviour is needed/desired and if it is treated like that somewhere else in tendermint like that.

@@ -59,9 +60,11 @@ pub struct DeliverTx {
pub info: Info,

/// Amount of gas wanted
#[serde(rename = "gasWanted")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid all of these field attribute by putting a #[serde(rename_all = "camelCase")] on the struct. See the section in the container attribute documentation.

Copy link
Contributor Author

@yihuang yihuang Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the camelCase here is a special case, because other places tendermint actually use a_b format.

@xla xla self-assigned this Jul 10, 2020
@xla xla requested a review from liamsi July 10, 2020 08:11
@xla xla changed the title Fix rpc response deserialization abci: fix rpc response deserialization Jul 10, 2020
@xla xla added abci bug Something isn't working labels Jul 10, 2020
@yihuang
Copy link
Contributor Author

yihuang commented Jul 10, 2020

Thanks for the concise change-set. Could you explain why the null as default behaviour is needed/desired and if it is treated like that somewhere else in tendermint like that.

  • The "data":null, is what I observed from the response return from tendermint 0.33.6 (also other versions), I pasted that response here: rpc response decode failed with recent released version of tendermint #423 (comment)
  • I don't know a proper spec on protobuf json format conversion, but from my understanding, protobuf don't distinguish between default value and non-exist ones(non-exist value can be skipped in wire message).
    So for example, Option<Vec<_>> might be simplified with a Vec<_> with null_as_default semantic.

@yihuang yihuang force-pushed the fix-serialization branch 2 times, most recently from b937b5e to a300e5f Compare July 10, 2020 09:54
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It be great, if this PR also comes with an (updated?) test-case. Note that currently, there is a test-failure:


---- endpoints::block_results stdout ----
thread 'endpoints::block_results' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: ParseError, message: "Parse error. Invalid JSON", data: Some("missing field `gasWanted` at line 34 column 7") }', rpc/tests/integration.rs:113:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@yihuang
Copy link
Contributor Author

yihuang commented Jul 11, 2020

It be great, if this PR also comes with an (updated?) test-case. Note that currently, there is a test-failure:

The test case is fixed now.

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚕 📸 🚯 🔭

@liamsi liamsi merged commit 337b052 into informalsystems:master Jul 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abci bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpc response decode failed with recent released version of tendermint
3 participants