informalsystems / tendermint-rs

Client libraries for Tendermint/CometBFT in Rust!
Apache License 2.0
588 stars 215 forks source link

Correct and optimize custom serializers for stringized values, hashes, nullable and optional fields #1351

Closed mzabaluev closed 9 months ago

mzabaluev commented 9 months ago

The custom field serializers in tendermint and tendermint-proto have a few problems. The modules where the deserializers expect the values to be an Option (i.e. nullable) do not consistently use Serializer::serialize_some when serializing a non-null value. This is not an issue with serde_json, but as the serde framework is generic, this implementation will break with other serializers, particularly for non-self-describing formats. Deserializer functions unconditionally allocate temporary strings where they can potentially be borrowed from the deserializer, achieving zero copies from source data.

These changes correct the serde schema issues and introduce optimization by deserializing strings into Cow, allowing use of borrowed string values from deserializers that support it.

codecov-commenter commented 9 months ago

Codecov Report

Merging #1351 (4de748b) into main (a73362d) will increase coverage by 0.9%. The diff coverage is 79.3%.

:exclamation: Current head 4de748b differs from pull request most recent head 185315e. Consider uploading reports for the commit 185315e to get more accurate results

@@           Coverage Diff           @@
##            main   #1351     +/-   ##
=======================================
+ Coverage   59.5%   60.4%   +0.9%     
=======================================
  Files        272     272             
  Lines      26974   26544    -430     
=======================================
- Hits       16068   16052     -16     
+ Misses     10906   10492    -414     
Files Changed Coverage Δ
tendermint/src/serializers/option_hash.rs 33.3% <46.1%> (+33.3%) :arrow_up:
tendermint/src/serializers/apphash.rs 46.6% <50.0%> (ø)
tendermint/src/serializers/hash.rs 46.6% <50.0%> (ø)
tendermint/src/serializers/apphash_base64.rs 94.4% <75.0%> (-5.6%) :arrow_down:
proto/src/serializers/from_str.rs 100.0% <100.0%> (ø)
proto/src/serializers/nullable.rs 100.0% <100.0%> (ø)
proto/src/serializers/optional.rs 100.0% <100.0%> (ø)
proto/src/serializers/optional_from_str.rs 67.7% <100.0%> (+67.7%) :arrow_up:

... and 12 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more