Replace tmcrypto.PubKey by our own cryptotypes.PubKey #7419
Conversation
|
This pull request introduces 1 alert when merging 51a353f into 71166c8 - view on LGTM.com new alerts:
|
amaury1093
added
Type: Code Hygiene
| } | ||
|
|
||
| return encoding.PubKeyFromProto(tmProtoPk) | ||
| } |
There was a problem hiding this comment.
Why don't we use here the tendermint/crypto/ed25519? Public key is bytes we don't need to go through encoding here. We can just cast bytes to the right type using select.
There was a problem hiding this comment.
TM supports secp256k1 too now. I could add a switch .(type) here, and do what you suggest. However, I feel it's a bit cleaner to have a central function to do conversion between sdk pubkey <-> tm pubkey (aka FromTmPublicKey). And other conversions (here between tm pubkey <-> tm proto public key) use tm's own functions.
crypto/types/types.go
Outdated
| } | ||
|
|
||
| // BasePrivKey defines a private key | ||
| type BasePrivKey interface { |
There was a problem hiding this comment.
BasePrivKey sounds strange. How about doing other way around and renaming:
- PrivKey -> ProtoPrivKey
- BasePrivKey -> PrivKey
There was a problem hiding this comment.
I renamed BasePrivKey to LedgerPrivKey. See comments in 0048063 if they make sense to you.
There was a problem hiding this comment.
NOTE: this will be removed or changed when PrivKeyLedgerSecp256k1 will get converted to proto
|
Awesome. I also need it. This PR turned out to be a dependency to few other tasks. Thanks for doing it. 👍 |
* WIP on removing tm pub/privkey * Fix part of crypto tests * Add PrivKeyLedgerSecp256K1 proto type * Use BasePrivKey for ledger priv key type * Refacto continued * First round * x/staking * Continue porting * x/* done * Make build pass * More conversion * Remove IntoTmPubKey * Fix test * Remove crypto.PubKey in some other places * Revert ledger changes * Fix comment * Remove useless function * Add To/FromTmPublicKey * Add migrate tests * Fix test * Fix another test * Rename tm conversion functions * Less code * Rename BasePrivKey to LedgerPrivKey * Add changelog * Rename functions Co-authored-by: Amaury Martiny <amaury.martiny@protonmail.com> Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Description
ref: #7357
This PR aims to untangle the usage of TM's
tmcrypto.PubKeyin the SDK's codebase, and use the SDK's owncryptotypes.PubKeyinstead.(note: whether or not
cryptotypes.PubKeyshould itself extendtmcrypto.PubKeyis another orthogonal discussion, and can be tracked in #7357)Occurrences where we use TM's
crypto.PubKey:It also removes the
IntoTmPubKeyinterfaces, and uses instead two centralized functionsFromTmProtoPublicKeyandToTmProtoPublicKeywhenever we need to interface with TM/ABCI (as per Marko's idea).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.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes