PubKey proto types

[blushi]

Description

ref: #7109

• adds new proto pub keys Secp256K1PubKey and LegacyAminoMultisigThresholdPubKey (which still uses legacy amino address rules).
• introduces a new PubKey interface internal to cosmos-sdk that extends proto.Message and tendermint crypto.PubKey, we could in a follow-up PR stop relying on tendermint crypto.PubKey and redefine its methods signatures so they only handle proto.Message pub keys.
• there will be a follow up PR to clean up old PubKeyMultisigThreshold Remove old PubKeyMultisigThreshold #7284.
• Added Un/MarshalAmino(JSON) for secp256k1 keys (ref: Create PubKey proto types #7109 (comment)).

---

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]

Codecov Report

Merging #7147 into master will increase coverage by 1.17%.
The diff coverage is 51.33%.

@@            Coverage Diff             @@
##           master    #7147      +/-   ##
==========================================
+ Coverage   55.60%   56.77%   +1.17%     
==========================================
  Files         457      585     +128     
  Lines       27440    40678   +13238     
==========================================
+ Hits        15257    23094    +7837     
- Misses      11083    15644    +4561     
- Partials     1100     1940     +840     

[clevinson]

lgtm. Seems easy for this just be merged upstream to Aaron's PR.

[amaury1093]

This looks good to me, except the question I have below.

[alexanderbez]

Left some comments, but looks good. One question I have is how are addresses and keypairs going to be handled when coming from a live network on a previous version of the SDK? Will there need to be any migration? Will things just work "out-of-the-box" (i.e. addresses remain the same)?

[alexanderbez]

Does this actually return JSON?

[amaury1093]

Yes, AminoJSON prints bytes as base64, so it returns a JSON string here. Same behavior as current hub, e.g. https://api.cosmos.network/auth/accounts/cosmos1y3x7q772u8s25c5zve949fhanrhvmtnu484l8z

[amaury1093]

Will there need to be any migration?

There will be migration related to BaseAccount (whose PubKey field will use Any) in Genesis, and will be tackled in #6839 (comment), and I think that's it (correct me if I'm wrong @aaronc).

Will things just work "out-of-the-box" (i.e. addresses remain the same)?

Yeah, that's the idea behind the go-amino patch:

• bech32 addresses will remain the same (because we're keeping the same algorithm for the Address() method)
• marshalling the proto struct using Amino JSON (e.g. in legacy API) will use the MarshalAminoJSON override, so will marshal exactly the same output as before (see PubKey proto types #7147 (comment))
• say you're using the keyring (which still uses Amino binary). It unmarshals your backend keys into proto structs using the UnmarshalAmino override, so no migration there needed

that's all I can think of, but lmk if you can think of other stuff where it can get messy.

[amaury1093]

We have right now, in proto files:

• cosmos.crypto.multisig.LegacyAminoPubKey
• cosmos.crypto.secp256k1.{Pub,Priv}Key

Aaron also proposed:

I'm wondering though, why couldn't we just use the key type as a top-level namespace? At least for well-known key types, maybe not multisig. i.e. secp256k1.PubKey. What do people think?

Since these proto files are going as stable, we won't be able to change them without introducing proto-breaking changes, so it's worth taking some time to have a nomenclature we're happy about.

cc @zmanian @alexanderbez @alessio @jackzampolin @ethanfrey ?

Edit: Adding my opinion: I personally like having all our proto packages starting with cosmos.*

[alexanderbez]

Can you elaborate a bit on the proposal? What's wrong with the two proto files we have now?

[amaury1093]

Can you elaborate a bit on the proposal? What's wrong with the two proto files we have now?

Nothing wrong, just inviting people on some thoughts about a good nomenclature for pubkey proto packages, since they are not changeable after 0.40. Some ideas:

• cosmos.crypto.secp256k1.{Pub,Priv}Key
• cosmos.keys.secp256k1.{Pub,Priv}Key
• the shorter secp256k1.{Pub,Priv}Key

[aaronc]

There's nothing wrong with them. I was just thinking we could maybe make the package name even shorter. But maybe not worth it. I don't have a strong preference

[alexanderbez]

I like cosmos.crypto.secp256k1.{Pub,Priv}Key

[amaury1093]

aarg, #7147 (comment) is too big a refactor, and is entangled to the old multisig (import cycles). I added a comment there, and will be taken care of in #7284