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

Added empty option to signature #635

Merged
merged 4 commits into from
Oct 14, 2020
Merged

Conversation

greg-szabo
Copy link
Member

Fixes #626 .

This is a proposed fix for optional signatures.

  • 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

@@ -27,6 +30,9 @@ impl TryFrom<Vec<u8>> for Signature {
type Error = Error;

fn try_from(value: Vec<u8>) -> Result<Self, Self::Error> {
if value.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The value sent from Go Tendermint over privval socket (for signing) in Vote/Proposal requests is actually not empty: #626 (comment) -- it has one element which is 0u8.
tmkms could do some preprocessing though and clear that value before creating a domain type

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies, I misunderstood your comment in the issue. And, come on, a single 0u8? Aaaargh.

I'll add that in the code. One more thing to raise to the Go team. Thanks!

Copy link
Contributor

@tomtau tomtau Oct 14, 2020

Choose a reason for hiding this comment

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

oops, my apologies -- the vec![0u8] is something I saw in a debugger when running the Go unit tests. I've just tried against the TM process (0.34.0-rc5) and it's indeed empty.
735c153 / 84bbbc3 is OK
(9ed45f0 can be reverted)

Copy link
Member Author

Choose a reason for hiding this comment

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

LOL, thanks for checking again! I'm happy to implement extra work if it leads to extra checks! I should have checked it myself...

@codecov-io
Copy link

codecov-io commented Oct 14, 2020

Codecov Report

Merging #635 into master will decrease coverage by 0.0%.
The diff coverage is 9.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #635     +/-   ##
========================================
- Coverage    42.7%   42.7%   -0.1%     
========================================
  Files         167     167             
  Lines       11602   11613     +11     
  Branches     2594    2599      +5     
========================================
- Hits         4964    4959      -5     
- Misses       6347    6362     +15     
- Partials      291     292      +1     
Impacted Files Coverage Δ
tendermint/src/public_key.rs 61.9% <0.0%> (-0.4%) ⬇️
tendermint/src/signature.rs 62.9% <10.0%> (-9.6%) ⬇️
rpc/src/client/sync.rs 50.0% <0.0%> (-10.0%) ⬇️
tendermint/tests/integration.rs 0.0% <0.0%> (ø)
rpc/tests/integration.rs
rpc/tests/parse_response.rs 98.7% <0.0%> (ø)
rpc/src/client/transport/websocket.rs 72.7% <0.0%> (+0.4%) ⬆️

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 89dc88e...ef76d6b. Read the comment docs.

@tomtau
Copy link
Contributor

tomtau commented Oct 14, 2020

Tested ACK -- #635 (comment)

@thanethomson thanethomson merged commit 5c45e61 into master Oct 14, 2020
@thanethomson thanethomson deleted the greg/optional-signature branch October 14, 2020 19:29
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.

Make the signature on Proposal optional field?
4 participants