chore: improve keyring handling in add keys command #493
Conversation
Why do we want to skip saving ? |
RafilxTenfen
left a comment
There was a problem hiding this comment.
What if the key name is the same, but the key changes like imagine running the following commands:
$~ eotsd init --home ./test-eotsd/ --force
$~ eotsd keys add xx2 --home ./test-eotsd/ --keyring-backend test
- address: bbn1fsskpwxqttrtwps25l6j3q2uh9x8d0uac29sgs
name: xx2
pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"A79CrHeRyWl2GfziPvMWJpFDA2TxmUtWSy1xlu8/3Wtz"}'
pubkey_hex: bf42ac7791c9697619fce23ef3162691430364f1994b564b2d7196ef3fdd6b73
type: local
**Important** write this mnemonic phrase in a safe place.
It is the only way to recover your account if you ever forget your password.
salon smoke usual dynamic input stamp smooth basic leader various elbow harbor fire chronic pony crane photo tip arena sphere monster emerge kitchen this
$~ eotsd keys add xx2 --home ./test-eotsd/ --keyring-backend file
Enter keyring passphrase (attempt 1/3):
Re-enter keyring passphrase:
- address: bbn10f5whvp2qh7cl534nmq42026tmfm9y3qschjf8
name: xx2
pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AglN5v5xv9/H5LeOnKJveY13+TPmteCjMzPhq6tkLXYa"}'
pubkey_hex: 094de6fe71bfdfc7e4b78e9ca26f798d77f933e6b5e0a33333e1abab642d761a
type: local
**Important** write this mnemonic phrase in a safe place.
It is the only way to recover your account if you ever forget your password.
lesson nut menu vivid soap host weasel jewel actor remain crystal coil advance pepper tape worth seat couch enforce impose ignore car allow usedlike the key name xx2 was created twice with keyring backend test and file but the key is different, is this expected ?
Valid question, but we ultimately care about |
There was a problem hiding this comment.
it looks good to me, I can create another key with the same name
I am not sure whether it is expected that I can create a new key with the same name without checking if the eots pk corresponding to that name matches the old one or maybe propmt ask the user when there is an ErrDuplicateEOTSKeyName
So I am approving, but might be good to ask for @KonradStaniec to take a look
| flagMnemonicSrc = "source" | ||
| flagDBPath = "db-path" | ||
| flagBackupDir = "backup-dir" | ||
| flagMigrate = "migrate" |
| // ignore the err, keyring will handle it | ||
| if errors.Is(err, store.ErrDuplicateEOTSKeyName) { | ||
| return eotsPk, nil | ||
| } |
There was a problem hiding this comment.
We probably need to check if the key is the same as the key with the same key name. If not, we probably should reject
| if err := eotsManager.SaveEOTSKeyName(eotsPk.MustToBTCPK(), keyName); err != nil { | ||
| // ignore the err, keyring will handle it | ||
| if errors.Is(err, store.ErrDuplicateEOTSKeyName) { | ||
| return eotsPk, nil | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("failed to save key name mapping: %w", err) | ||
| } |
There was a problem hiding this comment.
Seems duplicate with Line 154. Let's wrap it
Agree that we probably need to check if the key is the same with the key with the same key name and reject if they are not the same key. The goal is to not overwrite the keyname -> pk mapping |
| return eotsPk, nil | ||
| } | ||
|
|
||
| if errors.Is(err, store.ErrDuplicateEOTSKeyRecord) { |
There was a problem hiding this comment.
Either we just warn the user here. Or we delete the key by calling clientCtx.Keyring.Delete(....)
There was a problem hiding this comment.
IMO we should just error out like you are doing
There was a problem hiding this comment.
sg, I'll fix unit tests and merge. Thanks for reviews everyone!
- ignores key already exists err, as this is already handled by keyring - propagates `client.Context` so that we don't need to reinitialize keyring over and over again but share the ctx
client.Contextso that we don't need to reinitialize keyring over and over again but share the ctx