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

rpc: Add support for consensus_state endpoint #719

Merged
merged 9 commits into from
Dec 16, 2020

Conversation

thanethomson
Copy link
Contributor

Closes #674

  • 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

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #719 (ac4887d) into master (25f7ba5) will increase coverage by 1.5%.
The diff coverage is 61.3%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #719     +/-   ##
========================================
+ Coverage    40.0%   41.5%   +1.5%     
========================================
  Files         202     205      +3     
  Lines       12805   13628    +823     
  Branches     3204    3307    +103     
========================================
+ Hits         5130    5667    +537     
- Misses       7348    7599    +251     
- Partials      327     362     +35     
Impacted Files Coverage Δ
rpc-probe/src/kvstore.rs 0.0% <0.0%> (ø)
rpc-probe/src/quick.rs 0.0% <0.0%> (ø)
rpc/src/client.rs 7.2% <0.0%> (-0.3%) ⬇️
rpc/src/serialization.rs 0.0% <0.0%> (ø)
tendermint/tests/integration.rs 0.3% <0.0%> (-0.1%) ⬇️
rpc/src/method.rs 12.8% <33.3%> (+0.5%) ⬆️
tendermint/src/vote.rs 68.4% <41.6%> (+0.7%) ⬆️
rpc/src/endpoint/consensus_state.rs 53.8% <53.8%> (ø)
rpc/tests/parse_response.rs 98.3% <98.0%> (-0.6%) ⬇️
proto/src/serializers/timestamp.rs 88.6% <100.0%> (ø)
... and 63 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 25f7ba5...ac4887d. Read the comment docs.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson marked this pull request as ready for review December 3, 2020 12:26
@thanethomson thanethomson marked this pull request as draft December 3, 2020 12:31
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson marked this pull request as ready for review December 7, 2020 15:20
rpc/src/client.rs Outdated Show resolved Hide resolved
pub step: i8,
}

impl Serialize for HeightRoundStep {
Copy link
Member

Choose a reason for hiding this comment

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

I know we deprioritized separating JSON serialization in the code, but would it make sense to add one-off JSON structs when we implement new domain types?

This seems like a prime example where we can create a HeightRoundStep JSON type that has i64/i64/i8 fields and then add a #[serde(try_from="json-type", into="json-type")] to the top of this struct.

I'm asking because tracking custom serialization implementations can become tricky and this implementation has public fields (described as a Type 0 struct in issue #661 ) but domain knowledge restrictions in the serialization (which I consider no-no and should be either in a TryFrom trait or a constructor as described by Type 1 and Type 2 structs in the same issue). I would guess that this doesn't cause issues in regular cases, because most of the fields are well guarded (Height and Round are domain types), but I can for example create a HeightRoundStep with step == -2 which I think is invalid.

So, even if we keep the custom serialization, it seems that a constructor might be warranted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could separate the JSON struct from the domain type for HeightRoundStep, but where would we put it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chatted to @greg-szabo about this, and we'll follow up here in a separate issue (#749).

melekes
melekes previously approved these changes Dec 11, 2020
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

Signed-off-by: Thane Thomson <connect@thanethomson.com>
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.

Support consensus state in tendermint rpc
5 participants