Migrate Tendermint PubKey types to the SDK

[aaronc]

In support of #5694 and possibly #6928.

Copy the following PubKey types from Tendermint into the SDK and update references in code:

• PubKeySecp256k1
• PubKeyEd25519
• PubKeySr25519

[tac0turtle]

yes, that works. Since ed25519 and sr25519 are imported libraries then you can build a wrapper around the library and not tendermint. This will further decouple the sdk from tendermint.

[alexanderbez]

Yes, this is what I've been trying to advocate for in the beginning haha

[tac0turtle]

I know @ebuchman had a few words to this approach. Should we doulbe check what they were?

[aaronc]

I'll update the issue description to just mention copying. Would be good to hear from @ebuchman too if we can reach him!

[ebuchman]

I don't think I have strong opinions here other than concerns around keeping things in sync between repos.

[tac0turtle]

Before the copying begins is there anything that you would like to change about the interface or will the sdk still adhere to tendermints key interface?

[aaronc]

Before the copying begins is there anything that you would like to change about the interface or will the sdk still adhere to tendermints key interface?

The PubKey interface is undocumented and the methods aren't intuitive.

Bytes() for instance returns amino marshaled bytes. I would advocate to just remove that method.

VerifyBytes() should probably be called VerifySignature().

Address() is okay I think, but keys should follow #5694 except in legacy cases.

I would probably advocate for doing this with a different PubKey interface in the SDK, but maybe not right away.

[tac0turtle]

Bytes() for instance returns amino marshaled bytes. I would advocate to just remove that method.

This has been fixed now it's just the byte representation, which could be removed. Will see about making this change.

VerifyBytes() should probably be called VerifySignature().

Good point I'll see about fixing this before 0.34

[aaronc]

Bytes() for instance returns amino marshaled bytes. I would advocate to just remove that method.

This has been fixed now it's just the byte representation, which could be removed. Will see about making this change.

Yeah, I would recommend to just remove it because it's underspecified. That works for regular pubkeys, but what about multisigs? That will need to be some encoded version.

[sahith-narahari]

With regards to @marbar3778 comment #7047 (comment). I also feel it's better to migrate Secp256k1 first and make changes accordingly.
@aaronc will this work or do you intend modify other keys too in your future PRs

[aaronc]

Yes, the intention is to modify all the PubKeys making them 1) unsized and proto compatible and 2) updating address format for non-legacy keys.

So my preference is to have all of them copied in.