Skip to content

Commit

Permalink
perf: reduce user's password prompts when calling keyring List functi…
Browse files Browse the repository at this point in the history
…on (cosmos#13207)

* Reduce user password prompts by taking advantage of the already existing MigrateAll function

* Print message when no records were found on the keyring

* Update changelog

* Fix migration test

* Add keys sort
  • Loading branch information
raynaudoe authored Sep 9, 2022
1 parent 5912f75 commit 4882f93
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 46 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* (cli) [#13207](https://github.com/cosmos/cosmos-sdk/pull/13207) Reduce user's password prompts when calling keyring `List()` function
* (x/authz) [#12648](https://github.com/cosmos/cosmos-sdk/pull/12648) Add an allow list, an optional list of addresses allowed to receive bank assets via authz MsgSend grant.
* (sdk.Coins) [#12627](https://github.com/cosmos/cosmos-sdk/pull/12627) Make a Denoms method on sdk.Coins.
* (testutil) [#12973](https://github.com/cosmos/cosmos-sdk/pull/12973) Add generic `testutil.RandSliceElem` function which selects a random element from the list.
Expand Down
5 changes: 5 additions & 0 deletions client/keys/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ func runListCmd(cmd *cobra.Command, _ []string) error {
return err
}

if len(records) == 0 {
cmd.Println("No records were found in keyring")
return nil
}

if ok, _ := cmd.Flags().GetBool(flagListNames); !ok {
return printKeyringRecords(cmd.OutOrStdout(), records, clientCtx.OutputFormat)
}
Expand Down
2 changes: 1 addition & 1 deletion client/keys/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func runMigrateCmd(cmd *cobra.Command, _ []string) error {
return err
}

if err = clientCtx.Keyring.MigrateAll(); err != nil {
if _, err = clientCtx.Keyring.MigrateAll(); err != nil {
return err
}

Expand Down
54 changes: 11 additions & 43 deletions crypto/keyring/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ type Importer interface {

// Migrator is implemented by key stores and enables migration of keys from amino to proto
type Migrator interface {
MigrateAll() error
MigrateAll() ([]*Record, error)
}

// Exporter is implemented by key stores that support export of public and private keys.
Expand Down Expand Up @@ -517,43 +517,7 @@ func wrapKeyNotFound(err error, msg string) error {
}

func (ks keystore) List() ([]*Record, error) {
if err := ks.MigrateAll(); err != nil {
return nil, err
}

keys, err := ks.db.Keys()
if err != nil {
return nil, err
}

var res []*Record //nolint:prealloc
sort.Strings(keys)
for _, key := range keys {
// Recall that each key is twice in the keyring:
// - once with the `.info` suffix, which holds the key info
// - another time with the `.address` suffix, which only holds a reference to its associated `.info` key
if !strings.HasSuffix(key, infoSuffix) {
continue
}

item, err := ks.db.Get(key)
if err != nil {
return nil, err
}

if len(item.Data) == 0 {
return nil, sdkerrors.ErrKeyNotFound.Wrap(key)
}

k, err := ks.protoUnmarshalRecord(item.Data)
if err != nil {
return nil, err
}

res = append(res, k)
}

return res, nil
return ks.MigrateAll()
}

func (ks keystore) NewMnemonic(uid string, language Language, hdPath, bip39Passphrase string, algo SignatureAlgo) (*Record, string, error) {
Expand Down Expand Up @@ -895,30 +859,34 @@ func (ks keystore) writeMultisigKey(name string, pk types.PubKey) (*Record, erro
return k, ks.writeRecord(k)
}

func (ks keystore) MigrateAll() error {
func (ks keystore) MigrateAll() ([]*Record, error) {
keys, err := ks.db.Keys()
if err != nil {
return err
return nil, err
}

if len(keys) == 0 {
return nil
return nil, nil
}

sort.Strings(keys)
var recs []*Record
for _, key := range keys {
// The keyring items only with `.info` consists the key info.
if !strings.HasSuffix(key, infoSuffix) {
continue
}

_, err := ks.migrate(key)
rec, err := ks.migrate(key)
if err != nil {
fmt.Printf("migrate err for key %s: %q\n", key, err)
continue
}

recs = append(recs, rec)
}

return nil
return recs, nil
}

// migrate converts keyring.Item from amino to proto serialization format.
Expand Down
4 changes: 2 additions & 2 deletions crypto/keyring/migration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,12 @@ func (s *MigrationTestSuite) TestMigrateAllLegacyMultiOffline() {

s.Require().NoError(s.ks.SetItem(item))

err = s.kb.MigrateAll()
_, err = s.kb.MigrateAll()
s.Require().NoError(err)
}

func (s *MigrationTestSuite) TestMigrateAllNoItem() {
err := s.kb.MigrateAll()
_, err := s.kb.MigrateAll()
s.Require().NoError(err)
}

Expand Down

0 comments on commit 4882f93

Please sign in to comment.