From d10331a2b583ea7f1c3a99d8640e03ac1d70eafa Mon Sep 17 00:00:00 2001 From: Ezequiel Raynaudo Date: Fri, 9 Sep 2022 08:38:10 -0300 Subject: [PATCH 1/3] perf: reduce user's password prompts when calling keyring List function (#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 (cherry picked from commit 4882f933b1a108d578343573595fb06d7fd319c2) # Conflicts: # CHANGELOG.md --- CHANGELOG.md | 8 +++++ client/keys/list.go | 5 +++ client/keys/migrate.go | 2 +- crypto/keyring/keyring.go | 54 +++++++------------------------- crypto/keyring/migration_test.go | 4 +-- 5 files changed, 27 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0bd8958e37e..06be5d8e2d3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,15 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features +<<<<<<< HEAD * (cli) [#13304](https://github.com/cosmos/cosmos-sdk/pull/13304) Add `tx gov draft-proposal` command for generating proposal JSONs. +======= +* (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. +* (client) [#12936](https://github.com/cosmos/cosmos-sdk/pull/12936) Add capability to preprocess transactions before broadcasting from a higher level chain. +>>>>>>> 4882f933b (perf: reduce user's password prompts when calling keyring List function (#13207)) * (x/authz) [#13047](https://github.com/cosmos/cosmos-sdk/pull/13047) Add a GetAuthorization function to the keeper. * (cli) [#12742](https://github.com/cosmos/cosmos-sdk/pull/12742) Add the `prune` CLI cmd to manually prune app store history versions based on the pruning options. diff --git a/client/keys/list.go b/client/keys/list.go index 73c2e91c22af..7d47c8896b26 100644 --- a/client/keys/list.go +++ b/client/keys/list.go @@ -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) } diff --git a/client/keys/migrate.go b/client/keys/migrate.go index f76206f6ed9f..0a9fc78e2143 100644 --- a/client/keys/migrate.go +++ b/client/keys/migrate.go @@ -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 } diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index 370e0111cec4..7553617d2c55 100644 --- a/crypto/keyring/keyring.go +++ b/crypto/keyring/keyring.go @@ -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. @@ -492,43 +492,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) { @@ -870,30 +834,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. diff --git a/crypto/keyring/migration_test.go b/crypto/keyring/migration_test.go index 2435671e4e47..066cd9ceb227 100644 --- a/crypto/keyring/migration_test.go +++ b/crypto/keyring/migration_test.go @@ -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) } From b69ce720c3ab1406fe9567d93a58e6930b6d7178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= <31522760+fedekunze@users.noreply.github.com> Date: Fri, 23 Sep 2022 11:41:16 +0200 Subject: [PATCH 2/3] fix conflicts --- CHANGELOG.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06be5d8e2d3b..8f34a2e4fdcd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,15 +43,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features -<<<<<<< HEAD * (cli) [#13304](https://github.com/cosmos/cosmos-sdk/pull/13304) Add `tx gov draft-proposal` command for generating proposal JSONs. -======= * (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. -* (client) [#12936](https://github.com/cosmos/cosmos-sdk/pull/12936) Add capability to preprocess transactions before broadcasting from a higher level chain. ->>>>>>> 4882f933b (perf: reduce user's password prompts when calling keyring List function (#13207)) * (x/authz) [#13047](https://github.com/cosmos/cosmos-sdk/pull/13047) Add a GetAuthorization function to the keeper. * (cli) [#12742](https://github.com/cosmos/cosmos-sdk/pull/12742) Add the `prune` CLI cmd to manually prune app store history versions based on the pruning options. From e7bb05ba04a27e1382c8a281c084a700c66c90a9 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Fri, 23 Sep 2022 15:37:50 +0200 Subject: [PATCH 3/3] suggestions Co-authored-by: Aleksandr Bezobchuk --- CHANGELOG.md | 2 +- crypto/keyring/keyring.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f34a2e4fdcd..97cbab72088e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features * (cli) [#13304](https://github.com/cosmos/cosmos-sdk/pull/13304) Add `tx gov draft-proposal` command for generating proposal JSONs. -* (cli) [#13207](https://github.com/cosmos/cosmos-sdk/pull/13207) Reduce user's password prompts when calling keyring `List()` function +* (cli) [#13207](https://github.com/cosmos/cosmos-sdk/pull/13207) Reduce user's password prompts when calling keyring `List()` function. * (x/authz) [#13047](https://github.com/cosmos/cosmos-sdk/pull/13047) Add a GetAuthorization function to the keeper. * (cli) [#12742](https://github.com/cosmos/cosmos-sdk/pull/12742) Add the `prune` CLI cmd to manually prune app store history versions based on the pruning options. diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index 7553617d2c55..d34bfe3c0369 100644 --- a/crypto/keyring/keyring.go +++ b/crypto/keyring/keyring.go @@ -845,6 +845,7 @@ func (ks keystore) MigrateAll() ([]*Record, error) { } sort.Strings(keys) + var recs []*Record for _, key := range keys { // The keyring items only with `.info` consists the key info.