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

Fix consensus state vote summary deserialization #837

Merged
merged 6 commits into from
Apr 6, 2021

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Mar 25, 2021

Closes #836

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md

@thanethomson thanethomson marked this pull request as ready for review March 25, 2021 12:59
@codecov-io
Copy link

Codecov Report

Merging #837 (9f03313) into master (1cca3ac) will decrease coverage by 0.0%.
The diff coverage is 18.1%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #837     +/-   ##
========================================
- Coverage    29.1%   29.1%   -0.1%     
========================================
  Files         194     194             
  Lines       10766   10771      +5     
  Branches     4443    4448      +5     
========================================
- Hits         3139    3135      -4     
+ Misses       4653    4651      -2     
- Partials     2974    2985     +11     
Impacted Files Coverage Δ
rpc/src/endpoint/consensus_state.rs 22.3% <18.1%> (+0.4%) ⬆️
proto/src/serializers/timestamp.rs 32.5% <0.0%> (-5.0%) ⬇️
tendermint/src/block/commit.rs 43.3% <0.0%> (-3.4%) ⬇️
tendermint/src/signature.rs 46.3% <0.0%> (-2.5%) ⬇️
tendermint/src/block/commit_sig.rs 40.4% <0.0%> (-2.4%) ⬇️
testgen/src/validator.rs 36.5% <0.0%> (-1.8%) ⬇️
tendermint/src/hash.rs 47.7% <0.0%> (-1.0%) ⬇️
testgen/src/commit.rs 39.8% <0.0%> (-0.8%) ⬇️
testgen/src/header.rs 33.8% <0.0%> (ø)
tendermint/src/block/header.rs 29.6% <0.0%> (+0.9%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cca3ac...9f03313. Read the comment docs.

Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Good stuff :)

rpc/src/endpoint/consensus_state.rs Outdated Show resolved Hide resolved
@thanethomson thanethomson requested a review from romac March 29, 2021 17:47
Copy link
Contributor

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

Looks like the requested change her was addressed, so I'll provide the green check :)

Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Looks good!

We might want to revisit this code altogether at some point, perhaps to rewrite it to use parser combinators or something like that as the format is a bit too involved for manual parsing. But in the meantime, with the new tests, we have some confidence that this works as expected and can refactor later.

@thanethomson thanethomson merged commit 1c5c850 into master Apr 6, 2021
@thanethomson thanethomson deleted the thane/rpc-vote-type branch April 6, 2021 14:08
@thanethomson
Copy link
Contributor Author

We might want to revisit this code altogether at some point, perhaps to rewrite it to use parser combinators or something like that as the format is a bit too involved for manual parsing. But in the meantime, with the new tests, we have some confidence that this works as expected and can refactor later.

Ideally we'd avoid this kind of custom serialization format altogether from the server side 😁 It should be something that's encoded properly in JSON/Protobuf, and not a custom string format.

@romac
Copy link
Member

romac commented Apr 6, 2021

Ideally we'd avoid this kind of custom serialization format altogether from the server side 😁 It should be something that's encoded properly in JSON/Protobuf, and not a custom string format.

💯

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.

Intermittent failure in CI when deserializing consensus state vote summaries
4 participants