CanonicalVote and CanonicalProposal now correctly encode/decode BlockId

[greg-szabo]

Closes #663.

Correctly encode/decode CanonicalBlockId for CanonicalVote and CanonicalProposal. See issue for more details.

• 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

[codecov-io]

Codecov Report

Merging #664 (6093b57) into master (9b6239d) will increase coverage by 0.5%.
The diff coverage is 91.1%.

@@           Coverage Diff            @@
##           master    #664     +/-   ##
========================================
+ Coverage    39.0%   39.5%   +0.5%     
========================================
  Files         183     183             
  Lines       12804   12871     +67     
  Branches     2924    2943     +19     
========================================
+ Hits         5003    5095     +92     
+ Misses       7557    7528     -29     
- Partials      244     248      +4     

Impacted Files	Coverage Δ
tendermint/src/block/id.rs	73.6% <ø> (ø)
tendermint/src/proposal/canonical_proposal.rs	82.0% <77.7%> (+25.2%)	⬆️
tendermint/src/vote/canonical_vote.rs	91.5% <85.7%> (+33.1%)	⬆️
tendermint/src/proposal.rs	78.5% <100.0%> (+4.9%)	⬆️
tendermint/src/vote/sign_vote.rs	87.3% <100.0%> (+1.9%)	⬆️
tendermint/src/hash.rs	53.7% <0.0%> (ø)

---

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 9b6239d...6093b57. Read the comment docs.

[thanethomson]

I'd recommend adding regression tests when fixing compatibility issues like this, but otherwise LGTM 👍

[greg-szabo]

All right, regression tests were added.
The first problem with regression tests is that there is supposed to be an original test case at the "before" state of the change that you compare to and shame on me: the CanonicalVote and CanonicalProposal structs didn't have tests at all.
So I added a few unit tests to check the domain knowledge about these types. As we implement more domain knowledge, hopefully this gets extended more and possibly used for regression testing.
The second problem was that regression tests are supposed to prove that the rest of the system behaves similarly as before and only the area of change are affected. CanonicalVote and CanonicalProposal are used in signing and although signing has tests (which still pass), but not the specific case where the block ID is present but with an empty hash. So I added this specific test to the signing tests. The proof is that all signing still passes and builds the sameprotobuf bytestream as they do in Go.

[thanethomson]

Nice tests! 😁