search

cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.23k stars 3.6k forks source link

Migrate Tendermint PubKey types to the SDK #7008

Closed aaronc closed 4 years ago

aaronc commented 4 years ago

In support of #5694 and possibly #6928.

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

tac0turtle commented 4 years ago

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 commented 4 years ago

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

tac0turtle commented 4 years ago

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

aaronc commented 4 years ago

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

ebuchman commented 4 years ago

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

tac0turtle commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

With regards to @marbar3778 comment https://github.com/cosmos/cosmos-sdk/pull/7047#discussion_r470918264. 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 commented 4 years ago

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.