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

crypto: move keys to sdk #5832

Closed
wants to merge 1 commit into from
Closed

crypto: move keys to sdk #5832

wants to merge 1 commit into from

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Mar 19, 2020

Description

  • this is how I see moving keys to the cosmos-sdk
  • this PR is just to show, there is still work that needs to be done

ref #5819

Signed-off-by: Marko Baricevic [email protected]


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)

- this is how I see moving keys to the cosmos-sdk
- this PR is just to show, there is still work that needs to be done

Signed-off-by: Marko Baricevic <[email protected]>
@tac0turtle tac0turtle added C:Keys Keybase, KMS and HSMs C:Crypto labels Mar 19, 2020
@alessio
Copy link
Contributor

alessio commented Mar 19, 2020

This is a very good start indeed! How about splitting the old keybase and the new keyring implementations into two separate packages crypto/keybase and crypro/keyring? So that we would potentially end up having separate New constructors, one in each implementation-specific package. This would also smooth the future deprecation of crypto/keybase.

@tac0turtle
Copy link
Member Author

tac0turtle commented Mar 19, 2020

I will actually do that and the renaming of keys to keybase in a different pr so that people could easily review the key stuff in detail. this PR has ballooned 🎈

@tac0turtle
Copy link
Member Author

closing this as discussed with @alexanderbez. The cosmos-sdk will define its own proto type for keys

@tac0turtle tac0turtle closed this Mar 19, 2020
@tac0turtle tac0turtle deleted the crypto/keys branch March 19, 2020 17:06
@fedekunze
Copy link
Collaborator

How about splitting the old keybase and the new keyring implementations into two separate packages crypto/keybase and crypro/keyring? So that we would potentially end up having separate New constructors, one in each implementation-specific package. This would also smooth the future deprecation of crypto/keybase.

@alessio the keybase and keyring have a lot of common types that depend on each other. I'd just create a crypto/keyring directory to make the migration easier and avoid creating an extra /types or /common directory with the shared types.

@alessio
Copy link
Contributor

alessio commented Mar 24, 2020

@fedekunze sounds like a wise strategy 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Crypto C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants