[Feature Request]: Keys Management

[Bidon15]

Summary

Celestia Node can CRUD keys

Impact on users

It will be beneficial to produce a feature that allows Celestia Node to do CRUD on keys as this can allow signing Txs.

Currently, we have a merged ADR #369 that is proposing a state interaction for Celestia Node and a respective PR #411

Implementation ideas

As we are already using RPC approach lately, we can use it for this feature

Urgency

Medium

[liamsi]

The cosmos-sdk uses this for key storage: https://github.com/cosmos/cosmos-sdk/blob/master/crypto/keyring/doc.go
And this is also what lens uses under the hood I think.

[renaynay]

Yes @liamsi. I suggest if we continue w/ using lens, we should be consistent and use SDK's keyring.

[liamsi]

I think independent of lens we should be consistent and use SDK's keyring.

[renaynay]

We already do with #420

[Wondertan]

While reviewing #420 I went deeper into SDK's keyring. My observations are:

• They have some custom migration logic for amino and stuff that we don't need
• They introduce a keyring interface that is overloaded, not ergonomic and can be simplified
  • They treat Ledger as something that needs additional methods rather than an implementation detail
  • SupportedAlgorithms() (SigningAlgoList, SigningAlgoList) two lists instead of one?
  • Exporter interface whose responsibility is only ASCII armoring shouldn't really be an interface.
    • There can be only GetPubKey and GetPrivKey methods with armoring as separate utility functions and not an interface.
    • These getters will also remove the need for an Unsafe interface.
    • Importers could also use generic setters/writers with armoring treated elsewhere. Basically, armoring is a (de)serialization format that should not be a part of the interface.
  • Methods for migration
  • Multisig special methods can be implementation details
  • Why wallets are indexed by bother uid and address? Why only address or uid is not enough?
    • There might be a good reason for this, but it seems like a legacy
• They use github.com/99designs/keyring which looks sane.
  • The SDK's Keyring interface does not look sane and long term we should write our own generic wallet management which may wrap 99designs/keyring
  • We should look deeper into extending 99designs/keyring backend implementations with Ledger as a backend and possibly other hardware wallets. If its Keyring interface allows us to do so then we can remove our own Keystore interface, If not we should look for other generic wallets and/or improve on our Keystore

cc @liamsi @renaynay

[Wondertan]

I believe that all the Celestia wallets(like Kepler) should be super/light clients by default, so celestia-node(which implements those) has to contain first-class wallet support.

[renaynay]

Another requirement: when constructing keyring, allow users to pass custom userInput.

Ref: #420

[Wondertan]

@renaynay, this shouldn't be a requirement, but optional feature that tightens security for users. As otherwise, before sending any the user would need to write the password. But it can be undesirable:

• User has OS-level data encryption and he enters a similar purpose password every time he enters his PC
• User can have a desirable level of security just by entering the password to unlock his PR through a login screen. This does not involve encryption of the store private keys, but it is ok for the majority of users, IMO

Personally, I would like to have this feature, but I would not call this a requirement.

[renaynay]

"Allow users to specify a custom key name we should also allow custom path to the keyring, not necessary under store/keys, so that users can access keyrings elsewhere"

[tzdybal]

Any estimates on delivery of this feature?

[renaynay]

Allow users to indicate whether they want the mnemonic for generated key to be logged.

[renaynay]

Any estimates on delivery of this feature?

@tzdybal we already have a basic key utility that is taken from cosmos-sdk. It can be accessed via celestia <node_type> keys --help

[Bidon15]

Grooming 16/11/2022:

This issue is touching org wide decision making and we need help to understand how to resolve this org wise
cc: @MSevey

[MSevey]

is this a blocker for incentivized testnet or mainnet?

[Wondertan]

Not a blocker. We can live with what CosmosSDK provides for now and for mainnet

[MSevey]

ok, removing my assignment then