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

Update to deprecations in ed25519 1.3 #1024

Merged
merged 2 commits into from
Nov 26, 2021
Merged

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Nov 22, 2021

Fixes: #1023

As of release 1.3.0, ed25519 deprecates the standalone SIGNATURE_LENGTH constant in favor of the Signature::BYTE_SIZE associated item. Also, the Signature::new constructor is deprecated in favor of Signature::from_bytes.
Update the code to not trigger the deprecation lints.

In turn, deprecate what was formerly the reexport of that constant in the tendermint API.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests - pure refactoring, exercised by existing tests.
  • Added entry in .changelog/

The most recent release of ed25519 API deprecates the standalone
SIGNATURE_LENGTH constant in favor of the Signature::BYTE_SIZE
associated item. Also, the Signature::new constructor is deprecated
in favor of Signature::from_bytes.
Update the code to not trigger the deprecation lints.

In turn, deprecate what was formerly the reexport of
that constant in the tendermint API.
@mzabaluev mzabaluev added the dependencies Pull requests that update a dependency file label Nov 22, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2021

Codecov Report

Merging #1024 (63b3aab) into master (9690eca) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1024     +/-   ##
========================================
- Coverage    66.1%   66.0%   -0.1%     
========================================
  Files         209     209             
  Lines       20764   20760      -4     
========================================
- Hits        13726   13719      -7     
- Misses       7038    7041      +3     
Impacted Files Coverage Δ
tendermint/src/signature.rs 55.0% <ø> (+0.5%) ⬆️
tendermint/src/proposal.rs 90.4% <100.0%> (-0.3%) ⬇️
tendermint/src/test.rs 100.0% <100.0%> (ø)
tendermint/src/vote.rs 76.6% <100.0%> (+0.3%) ⬆️
tendermint/src/vote/sign_vote.rs 96.5% <100.0%> (ø)
testgen/src/vote.rs 84.0% <100.0%> (ø)
config/src/config.rs 55.2% <0.0%> (-1.3%) ⬇️
testgen/src/validator.rs 86.7% <0.0%> (-0.8%) ⬇️
tendermint/src/node.rs 63.5% <0.0%> (ø)
... and 1 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 9690eca...63b3aab. Read the comment docs.

Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Sweet, thanks @mzabaluev! Will leave it to @thanethomson to take another look and do the merge.

@@ -159,6 +158,7 @@ impl Vote {
}

/// Default trait. Used in tests.
// FIXME: Does it need to be in public crate API? If not, replace with a helper fn in crate::test?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To expand on this: it's probably not useful for non-test API users to have a Default impl for Vote, and there is potential for inadvertent misuse that is hard to audit for.

For test users, a dedicated helper fn would serve just as well, unless there are some commonly used cases that are more ergonomic with a Default impl. The dedicated function will be easy to find in code.

Copy link
Member

@romac romac Nov 22, 2021

Choose a reason for hiding this comment

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

Agreed, a dedicated helper fn sounds like the way to go. As far as I can tell this Default instance is only used in this one test (both directly via Vote::default() and indirectly via ..Default::default):

fn test_sign_bytes_compatibility() {

Copy link
Member

Choose a reason for hiding this comment

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

Tracked in #1025

@karim-agha
Copy link

Can you please merge this? It's blocking my build.

@romac romac merged commit df231ed into master Nov 26, 2021
@romac romac deleted the mikhail/ed25519-deprecations branch November 26, 2021 17:12
mzabaluev added a commit that referenced this pull request Nov 26, 2021
thanethomson pushed a commit that referenced this pull request Nov 26, 2021
* Update to deprecations in ed25519 1.3 (#1024)

* Fix up deprecation for ED25519_SIGNATURE_SIZE

0.23.1 was released before #1023 or its backport to v0.23.x were merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation warnings with ed25519 1.3.0
4 participants