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

Automatically de/serialize ABCI event attributes from/to base64 #718

Merged
merged 6 commits into from
Dec 4, 2020

Conversation

thanethomson
Copy link
Member

@thanethomson thanethomson commented Dec 1, 2020

Closes #717

  • 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>
…roadcast_tx_commit

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 2, 2020

Codecov Report

Merging #718 (98ca62a) into master (25f7ba5) will increase coverage by 0.1%.
The diff coverage is 79.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #718     +/-   ##
========================================
+ Coverage    40.0%   40.1%   +0.1%     
========================================
  Files         202     202             
  Lines       12805   12838     +33     
  Branches     3204    3214     +10     
========================================
+ Hits         5130    5160     +30     
+ Misses       7348    7331     -17     
- Partials      327     347     +20     
Impacted Files Coverage Δ
rpc/src/endpoint/broadcast/tx_commit.rs 0.0% <0.0%> (ø)
tendermint/src/abci/tag.rs 46.8% <75.0%> (+23.7%) ⬆️
proto/src/serializers/bytes.rs 92.8% <100.0%> (-2.8%) ⬇️
rpc/tests/parse_response.rs 98.9% <100.0%> (+<0.1%) ⬆️
tendermint/src/abci/info.rs 27.2% <100.0%> (+27.2%) ⬆️
tendermint/src/abci/responses.rs 27.0% <100.0%> (+6.4%) ⬆️
rpc/src/order.rs 0.0% <0.0%> (ø)
rpc/src/query.rs 72.6% <0.0%> (ø)
rpc/src/client.rs 7.5% <0.0%> (ø)
... and 38 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...98ca62a. Read the comment docs.

@thanethomson thanethomson marked this pull request as ready for review December 2, 2020 00:06
Copy link
Member

@greg-szabo greg-szabo left a comment

Choose a reason for hiding this comment

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

The code is technically correct and I like that the serialization is done using serde attributes (instead of custom derives) but I have some reservations about where the serialization module is located.

While trying to separate serialization from domain types, there were a few places where serialization code was gathered:

The tendermint_proto::serializers module contains most of the "raw" serializer/deserializer code in a reusable format. Things like serializing from a string that is supposed to be an integer, etc.
The tendermint::serializers module contains serializers that have specific validation requirements on the Tendermint domain-type level: for example a Hash is just a hexstring (serializer implemented in tendermint_proto) but it also has a length requirement that is validated in the domain type, hence the serializer needs to call that validation (in the form of a TryFrom trait in this case, which is implemented in tendermint).

Technically the base64string module would belong into tendermint_proto since it doesn't have any domain-type specific restrictions. I understand that this sounds a bit weird, since nothing in tendermint-proto is actually using the module.

So I leave it to you to decide if it's worth putting it there or keep it in tendermint, but I would really like to move it into one of the serializers modules, both for reusability and to keep the idea of a slow but constant separation of serialization from domain types.

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

So I leave it to you to decide if it's worth putting it there or keep it in tendermint, but I would really like to move it into one of the serializers modules, both for reusability and to keep the idea of a slow but constant separation of serialization from domain types.

Cool, I've moved just the deserializer into the tendermint-proto crate for now, since the serializer works for Strings. If we need a completely separate serialize/deserialize method then I can abstract it out into a separate module in the bytes.rs file, but it doesn't seem necessary right now.

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.

👍

@thanethomson thanethomson merged commit 68bb2e8 into master Dec 4, 2020
@thanethomson thanethomson deleted the s11n/abci-tag-deser branch December 4, 2020 12:20
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.

abci Tag deserializer
4 participants