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 ed25519 #8690

Merged
merged 7 commits into from
Feb 26, 2021
Merged

update ed25519 #8690

merged 7 commits into from
Feb 26, 2021

Conversation

tac0turtle
Copy link
Member

Description

use zip215 for ed25519 verification

closes: #7890


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #8690 (28cedcf) into master (92bc290) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8690      +/-   ##
==========================================
+ Coverage   61.36%   61.48%   +0.12%     
==========================================
  Files         671      659      -12     
  Lines       38324    38045     -279     
==========================================
- Hits        23516    23393     -123     
+ Misses      12350    12193     -157     
- Partials     2458     2459       +1     
Impacted Files Coverage Δ
crypto/keys/ed25519/ed25519.go 69.38% <100.00%> (ø)
x/bank/keeper/migrations.go 0.00% <0.00%> (-50.00%) ⬇️
x/bank/types/metadata.go 94.44% <0.00%> (-5.56%) ⬇️
x/bank/module.go 51.16% <0.00%> (-4.66%) ⬇️
x/slashing/module.go 54.76% <0.00%> (-2.06%) ⬇️
x/distribution/module.go 54.76% <0.00%> (-2.06%) ⬇️
x/staking/module.go 57.50% <0.00%> (-2.03%) ⬇️
x/bank/keeper/keeper.go 72.50% <0.00%> (-1.47%) ⬇️
x/bank/keeper/send.go 73.63% <0.00%> (-1.37%) ⬇️
types/address.go 65.99% <0.00%> (-0.94%) ⬇️
... and 17 more

@robert-zaremba
Copy link
Collaborator

Just for clarifiaction - this is only if SDK will like to verify Tendermint validators signatures, right? Do we have any use case for that in SDK?

@tac0turtle
Copy link
Member Author

Just for clarifiaction - this is only if SDK will like to verify Tendermint validators signatures, right? Do we have any use case for that in SDK?

I dont believe so. Its also the case in which user accounts are using ed25519. Not the case now but thought I might as well knock it out

@robert-zaremba
Copy link
Collaborator

Maybe we should also add a documentation that the package should be used only to validate compatible signatures. What do you think?

@tac0turtle
Copy link
Member Author

Maybe we should also add a documentation that the package should be used only to validate compatible signatures. What do you think?

yea will add in the am

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK. Pending changelog entry

crypto/keys/ed25519/ed25519.go Show resolved Hide resolved
@tac0turtle
Copy link
Member Author

Maybe we should also add a documentation that the package should be used only to validate compatible signatures. What do you think?

added a doc.go and changelog entry

@tac0turtle tac0turtle added A:automerge Automatically merge PR once all prerequisites pass. C:Crypto labels Feb 25, 2021
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Approving, but please update the docs to clarify that we are using hdevalence/ed25519consensus for compatibility with Tendermint.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

utACK

@mergify mergify bot merged commit fd482a2 into master Feb 26, 2021
@mergify mergify bot deleted the 7890-update-ed25519 branch February 26, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Crypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ed25519 backend
4 participants