proposal: move keys to cosmos-sdk

[tac0turtle]

Summary

Currently, all the keys that are used in the cosmos-sdk are located in Tendermint. This can create issues sometimes if changes need to be made to the keys for the cosmos-sdk it gets blocked on a Tendermint release (major most likely).

Problem Definition

Cosmos-sdk keys (secp, sr25519, multisig) are defined in Tendermint.

Things with the current proto migration are blocked or can not be fully implemented until tendermint implements all keys in proto and does a release with this change (major release).

Proposal

move secp, sr25519 and multi-sig to cosmos-sdk

1. Create a keys directory within crypto that will house default keys that the cosmos-sdk supports.
2. Move the supported keys from tendermint to the cosmos-sdk.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

In favor of this, but this will we need to clearly document what is breaking and backward-incompatible. AFAIK, addresses are NOT based on the result of Bytes, but rather the raw byte slices. So I think keys should be OK. Anything stored and transferred over the wire will break, but that's also OK.

[alessio]

We are talking about the github.com/tendermint/tendermint/crypto package, aren't we?
In line of principle I'm in favor of this (there are definitely too many crypto and keys packages around), do you have ideas on how to go about this? Are you proposing to merge tendermint and cosmos-sdk crypto together?

[alexanderbez]

No, they'd be separately maintained packages. The SDK would drop its dependency on Tendermint's crypto package. @ebuchman do you have any reservations on this?

[tac0turtle]

started a preliminary pr #5832, if this gets accepted ill finish the work there

[alexanderbez]

Why is that PR so large? That is not what I had in mind at all. We should not be duplicating code to that extent. I would have a separate repo with core types and business logic.

[tac0turtle]

If we don't want to move it over because its a lot of code then i'd prefer the cosmos-sdk to define their own oneof for proto types.

I opened it quickly so I am fine to close it as well

[aaronc]

In general I'm in favor of this since it seems like it would simplify Cosmos SDK maintenance cycles. Trying to understand what the proposed approach is. Are you suggesting a third repo outside of both cosmos-sdk and tendermint @alexanderbez ?

[alexanderbez]

I believe the approach is to just redefine the high-level types and interfaces (e.g. Pubkey and SECP256K1) and re-use the libraries and auxiliary logic in Tendermint (to keep code DRY).

[alexanderbez]

Do you have any interest or bandwidth to do this @aaronc? It should be pretty trivial.

[fedekunze]

I will tackle this

[ebuchman]

I guess this means these key types will be duplicated for now? Tendermint mostly just uses ed25519, but in theory it can/should be able to use other keys. I worry about divergence - if this is necessary for now, so be it, but ideally we can get release cycles back on track and undo this down the road to avoid divergence in types, unless there is a good reason after all (besides release cycles) for these types to be duplicated.

[tac0turtle]

Since addresses will break when the proto migration is completed, the question on how to best handle addresses arises.

One option is to define custom marshallers that the SDK can use for addresses.

• this can be defined in Tendermint or the SDK.

The path that is being taken for keys is here: #5997.
(not moving keys into the sdk but defining prototypes for keys in the cosmos-sdk)

cc @zmanian @alexanderbez @ebuchman @aaronc

[aaronc]

Instead of breaking addresses can we just "grandfather" the amino encoding for multisigs?

Also, let's keep #5694 in mind regarding addresses going forward.

[fedekunze]

Instead of breaking addresses can we just "grandfather" the amino encoding for multisigs?

@aaronc what does "grandfather" mean in this context?

[aaronc]

Well just that we keep the current multisig around with addresses formed by amino serialization of legacy compatibility.

Then we should add a second multisig that is proto-encoded and follows the new address conventions (#5694).

[tac0turtle]

I think we should move the multisgs to the SDK. I dont believe anyone other than the sdk uses them and they are not fully implemented as stated here tendermint/tendermint#2163). This would allow the sdk to customize the multisigs without waiting for tendermint to do a release. Are there any objections?

[tac0turtle]

closing this as the work has been completed. Multisig was moved to sdk but other keytypes are being imported from tendermint which is a better design. We can revisit if anything else needs to change in a different issue