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

Add pub key proto definitions #6217

Merged
merged 28 commits into from
May 14, 2020
Merged

Add pub key proto definitions #6217

merged 28 commits into from
May 14, 2020

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented May 14, 2020

ref: #6213, after #6214

This PR provides basic protobuf definitions for public keys. It does not implement any functionality beyond the protobuf definitions. That's for other PR's.

It also adds a "Public Key Encoding" section to ADR 020 to describe the approach.

@auto-comment
Copy link

auto-comment bot commented May 14, 2020

👋 Thanks for creating a PR!

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.


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Thank you for your contribution to the Cosmos-SDK! 🚀

@aaronc
Copy link
Member Author

aaronc commented May 14, 2020

@marbar3778 this is what I have in mind for the basic structure of PublicKey. What do you think?

@aaronc aaronc mentioned this pull request May 14, 2020
27 tasks
@codecov
Copy link

codecov bot commented May 14, 2020

Codecov Report

Merging #6217 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6217   +/-   ##
=======================================
  Coverage   54.84%   54.84%           
=======================================
  Files         444      444           
  Lines       26771    26771           
=======================================
  Hits        14682    14682           
  Misses      11046    11046           
  Partials     1043     1043           


// any_pubkey can be used for any pubkey that an app may use which is
// not explicitly defined in the oneof
google.protobuf.Any any_pubkey = 15;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How would this work for an external application? Eg: on Ethermint we use ethereum's secp256k1 implementation:

https://github.com/ChainSafe/ethermint/blob/542a25ff827036fe9305b154f23ec86f45f48947/crypto/secp256k1.go#L78-L80

It'd be nice to have a concrete example to ease the migration

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking there would be two ways to handle this.

One is we don't define the mapping to crypto.PubKey on this type and instead let that be a parameter for TxDecoder, i.e. func (types.PublicKey) (crypto.PubKey, error).

Or, you could use any_pubkey which would specify the full URL (ex. ethermint.crypto.PubKeySecp256k1).

@aaronc aaronc added T: ADR An issue or PR relating to an architectural decision record C:Crypto R4R labels May 14, 2020
@aaronc aaronc marked this pull request as ready for review May 14, 2020 19:39
@aaronc aaronc requested a review from tac0turtle May 14, 2020 19:39
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM, will work on bringing over the multisig

crypto/types/types.proto Outdated Show resolved Hide resolved
crypto/types/types.proto Outdated Show resolved Hide resolved
crypto/types/types.proto Outdated Show resolved Hide resolved
@aaronc aaronc requested a review from alexanderbez May 14, 2020 20:24
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronc aaronc merged commit 40082b8 into master May 14, 2020
@aaronc aaronc deleted the aaronc/6213-crypto-proto branch May 14, 2020 20:45
@clevinson clevinson added this to the v0.39 milestone Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Crypto T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants