Evaluate refactoring or possible rewrite of Keyring

[ainhoa-a]

A short review and evaluation of a possible rewrite of Keyring is proposed.

[tac0turtle]

would love to have a chat about this. there was this work https://github.com/cosmos/keystone that stopped last year. The concept is really good and could be extendable to other forms of singing without supporting it in the sdk

[JulianToledano]

@tac0turtle that looks like a really good approach.
The main goal of this refactor should be the possibility of use different signing algos as is something the community is requesting.

We should focus on:

• Generate a common interface for implementing different algos
• Makes those algos pluginable so there is no need to rebuild the sdk for custom algos

It should be as simple as possible to be easy to use and tweak

[tac0turtle]

oh okay. Keystone is a different layer. You are talking about Algos and im talking about signing methods. We do already support injecting different algos, but maybe it could use a refresh

[JulianToledano]

oh my bad there! but if I’m not wrong the injection of the algos is not dynamic, right? someone should fulfil the SignatureAlgo interface and add it to SupportedAlgos. That would require to do before compiling.

[tac0turtle]

yes build time addition, runtime injection isn't really needed here

[JulianToledano]

👋 , @tac0turtle
We took a look at keystone and the idea is really great, although the code is not currently working. The idea would be to start working and maintain it so in the near future we can replace the current keyring and remove the dependency with 99designs/keyring which is not really active right now. One of the main goals will be to define an interface so new backends are easy to develop and integrate.

However there are small improvements that can be easily achieved.

1. Use cosmossdk.io/errors instead of github.com/cosmos/cosmos-sdk/types/errors and github.com/pkg/errors
2. Define errors as variables instead. There are some errors that are created at the return level.
3. Some functions as ImportPubKey(uid string, armor string) error can be change to ImportPubKey(uid, armor string) error to be more compliant with the code style.

[tac0turtle]

this sounds good to me, would be amazing to have keystone working and allowing other to extend backend types

[ainhoa-a]

Hi @tac0turtle thanks for the feedback! we'll get started with this.