From 2cea8f61d3b71990980be383d9d2ab681bd50f49 Mon Sep 17 00:00:00 2001 From: levisyin Date: Tue, 12 Dec 2023 13:41:48 +0800 Subject: [PATCH 1/5] feat: check multisig key duplicate --- .gitignore | 1 + client/keys/add.go | 10 ++++-- client/keys/add_test.go | 71 ++++++++++++++++++++++++++++++++++++++++ client/keys/show.go | 12 +++++-- client/keys/show_test.go | 10 ++++++ 5 files changed, 99 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 43e8b06a15a4..0fc9f902957a 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,7 @@ dist tools-stamp buf-stamp artifacts +simapp/simd/simd # Go go.work diff --git a/client/keys/add.go b/client/keys/add.go index 527209c918ea..9e452e4ce601 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -164,8 +164,14 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf return err } - for i, keyname := range multisigKeys { - k, err := kb.Key(keyname) + keyFilter := make(map[string]struct{}) + for i, keyName := range multisigKeys { + if _, ok := keyFilter[keyName]; ok { + return fmt.Errorf("duplicate multisig keys: %s", keyName) + } else { + keyFilter[keyName] = struct{}{} + } + k, err := kb.Key(keyName) if err != nil { return err } diff --git a/client/keys/add_test.go b/client/keys/add_test.go index 84b81833bbb7..aab0b3b14a61 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -127,6 +127,77 @@ func Test_runAddCmdBasic(t *testing.T) { require.Error(t, cmd.ExecuteContext(ctx)) } +func Test_runAddCmdMultisig(t *testing.T) { + cmd := AddKeyCommand() + cmd.Flags().AddFlagSet(Commands().PersistentFlags()) + + mockIn := testutil.ApplyMockIODiscardOutErr(cmd) + kbHome := t.TempDir() + + cdc := moduletestutil.MakeTestEncodingConfig().Codec + kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn, cdc) + require.NoError(t, err) + + clientCtx := client.Context{}. + WithKeyringDir(kbHome). + WithInput(mockIn). + WithCodec(cdc). + WithAddressCodec(addresscodec.NewBech32Codec("cosmos")). + WithValidatorAddressCodec(addresscodec.NewBech32Codec("cosmosvaloper")). + WithConsensusAddressCodec(addresscodec.NewBech32Codec("cosmosvalcons")) + + ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx) + + t.Cleanup(func() { + _ = kb.Delete("keyname1") + _ = kb.Delete("keyname2") + _ = kb.Delete("multisigname") + }) + + cmd.SetArgs([]string{ + "keyname1", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + }) + require.NoError(t, cmd.ExecuteContext(ctx)) + + cmd.SetArgs([]string{ + "keyname2", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + }) + require.NoError(t, cmd.ExecuteContext(ctx)) + + cmd.SetArgs([]string{ + "multisigname", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"), + fmt.Sprintf("--%s=%s", flagMultiSigThreshold, "2"), + }) + require.NoError(t, cmd.ExecuteContext(ctx)) + + cmd.SetArgs([]string{ + "multisigname", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname1"), + fmt.Sprintf("--%s=%s", flagMultiSigThreshold, "2"), + }) + mockIn.Reset("y\n") + require.Error(t, cmd.ExecuteContext(ctx)) + mockIn.Reset("y\n") + require.EqualError(t, cmd.ExecuteContext(ctx), "duplicate multisig keys: keyname1") +} + func Test_runAddCmdDryRun(t *testing.T) { pubkey1 := `{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AtObiFVE4s+9+RX5SP8TN9r2mxpoaT4eGj9CJfK7VRzN"}` pubkey2 := `{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"A/se1vkqgdQ7VJQCM4mxN+L+ciGhnnJ4XYsQCRBMrdRi"}` diff --git a/client/keys/show.go b/client/keys/show.go index 582e6ec9c1a5..1fb645b52770 100644 --- a/client/keys/show.go +++ b/client/keys/show.go @@ -71,10 +71,16 @@ func runShowCmd(cmd *cobra.Command, args []string) (err error) { } } else { pks := make([]cryptotypes.PubKey, len(args)) - for i, keyref := range args { - k, err := fetchKey(clientCtx.Keyring, keyref, clientCtx.AddressCodec) + keyFilter := make(map[string]struct{}) + for i, keyRef := range args { + if _, ok := keyFilter[keyRef]; ok { + return fmt.Errorf("duplicate keys: %s", keyRef) + } else { + keyFilter[keyRef] = struct{}{} + } + k, err := fetchKey(clientCtx.Keyring, keyRef, clientCtx.AddressCodec) if err != nil { - return fmt.Errorf("%s is not a valid name or address: %w", keyref, err) + return fmt.Errorf("%s is not a valid name or address: %w", keyRef, err) } key, err := k.GetPubKey() if err != nil { diff --git a/client/keys/show_test.go b/client/keys/show_test.go index 676385525974..48a651b7aa5d 100644 --- a/client/keys/show_test.go +++ b/client/keys/show_test.go @@ -134,6 +134,16 @@ func Test_runShowCmd(t *testing.T) { }) require.EqualError(t, cmd.ExecuteContext(ctx), "threshold must be a positive integer") + // Now try multisig key duplicate + cmd.SetArgs([]string{ + fakeKeyName1, fakeKeyName1, + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", FlagBechPrefix, sdk.PrefixAccount), + fmt.Sprintf("--%s=2", flagMultiSigThreshold), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + }) + require.EqualError(t, cmd.ExecuteContext(ctx), "duplicate keys: "+fakeKeyName1) + cmd.SetArgs([]string{ fakeKeyName1, fakeKeyName2, fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), From 409b23e264c217bdc9a33f6d6cef3b071649b3e9 Mon Sep 17 00:00:00 2001 From: levisyin Date: Tue, 12 Dec 2023 13:52:22 +0800 Subject: [PATCH 2/5] add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9577206d408..371ee331ff72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (client/keys) [#18703](https://github.com/cosmos/cosmos-sdk/pull/18703) Improve ` keys add` and ` keys show` by checking wether if there exist duplicate keys in multisig case. * (client/keys) [#18687](https://github.com/cosmos/cosmos-sdk/pull/18687) Improve ` keys mnemonic` by displaying mnemonic discreetly on an alternate screen and adding `--indiscreet` option to disable it. * (client/keys) [#18663](https://github.com/cosmos/cosmos-sdk/pull/18663) Improve ` keys add` by displaying mnemonic discreetly on an alternate screen and adding `--indiscreet` option to disable it. * (types) [#18440](https://github.com/cosmos/cosmos-sdk/pull/18440) Add `AmountOfNoValidation` to `sdk.DecCoins`. From 794201269afa3e55c1b30362bb1cb2afda2bd828 Mon Sep 17 00:00:00 2001 From: levisyin Date: Tue, 12 Dec 2023 18:31:07 +0800 Subject: [PATCH 3/5] show warning message instead of return error --- client/keys/show.go | 2 +- client/keys/show_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/keys/show.go b/client/keys/show.go index 1fb645b52770..65ae9eff9061 100644 --- a/client/keys/show.go +++ b/client/keys/show.go @@ -74,7 +74,7 @@ func runShowCmd(cmd *cobra.Command, args []string) (err error) { keyFilter := make(map[string]struct{}) for i, keyRef := range args { if _, ok := keyFilter[keyRef]; ok { - return fmt.Errorf("duplicate keys: %s", keyRef) + cmd.Printf("WARNING: duplicate keys found: %s.\n", keyRef) } else { keyFilter[keyRef] = struct{}{} } diff --git a/client/keys/show_test.go b/client/keys/show_test.go index 48a651b7aa5d..2aafd19f912b 100644 --- a/client/keys/show_test.go +++ b/client/keys/show_test.go @@ -134,7 +134,7 @@ func Test_runShowCmd(t *testing.T) { }) require.EqualError(t, cmd.ExecuteContext(ctx), "threshold must be a positive integer") - // Now try multisig key duplicate + // Now try multisig key duplicate(we just show warning message instead of return error) cmd.SetArgs([]string{ fakeKeyName1, fakeKeyName1, fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), @@ -142,7 +142,7 @@ func Test_runShowCmd(t *testing.T) { fmt.Sprintf("--%s=2", flagMultiSigThreshold), fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), }) - require.EqualError(t, cmd.ExecuteContext(ctx), "duplicate keys: "+fakeKeyName1) + require.NoError(t, cmd.ExecuteContext(ctx)) cmd.SetArgs([]string{ fakeKeyName1, fakeKeyName2, From 86efdae65f7d02c8155ff9097cb700c9a7a0ed5a Mon Sep 17 00:00:00 2001 From: levisyin <150114626+levisyin@users.noreply.github.com> Date: Tue, 12 Dec 2023 18:32:05 +0800 Subject: [PATCH 4/5] Update CHANGELOG.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c68aa369377b..8091ba620694 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements -* (client/keys) [#18703](https://github.com/cosmos/cosmos-sdk/pull/18703) Improve ` keys add` and ` keys show` by checking wether if there exist duplicate keys in multisig case. +* (client/keys) [#18703](https://github.com/cosmos/cosmos-sdk/pull/18703) Improve ` keys add` and ` keys show` by checking whether there are duplicate keys in the multisig case. * (x/bank) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. * (x/distribution) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `CalculateDelegationRewards` and `DelegationTotalRewards` methods no longer panics on any sanity checks and instead returns appropriate errors. * (x/slashing) [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `JailUntil` and `Tombstone` methods no longer panics if the signing info does not exist for the validator but instead returns error. From 9000881f99ab60d3c98b7248d233a5f6bab56899 Mon Sep 17 00:00:00 2001 From: levisyin Date: Wed, 13 Dec 2023 09:16:53 +0800 Subject: [PATCH 5/5] update for suggestion change --- client/keys/add.go | 8 ++++---- client/keys/add_test.go | 2 +- client/keys/show.go | 11 +++++++---- client/keys/show_test.go | 4 +++- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/client/keys/add.go b/client/keys/add.go index 9e452e4ce601..a30a2d865a20 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -164,13 +164,13 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf return err } - keyFilter := make(map[string]struct{}) + seenKeys := make(map[string]struct{}) for i, keyName := range multisigKeys { - if _, ok := keyFilter[keyName]; ok { + if _, ok := seenKeys[keyName]; ok { return fmt.Errorf("duplicate multisig keys: %s", keyName) - } else { - keyFilter[keyName] = struct{}{} } + seenKeys[keyName] = struct{}{} + k, err := kb.Key(keyName) if err != nil { return err diff --git a/client/keys/add_test.go b/client/keys/add_test.go index aab0b3b14a61..53ce3af2798e 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -127,7 +127,7 @@ func Test_runAddCmdBasic(t *testing.T) { require.Error(t, cmd.ExecuteContext(ctx)) } -func Test_runAddCmdMultisig(t *testing.T) { +func Test_runAddCmdMultisigDupKeys(t *testing.T) { cmd := AddKeyCommand() cmd.Flags().AddFlagSet(Commands().PersistentFlags()) diff --git a/client/keys/show.go b/client/keys/show.go index 65ae9eff9061..afda1b5fd6ff 100644 --- a/client/keys/show.go +++ b/client/keys/show.go @@ -71,17 +71,20 @@ func runShowCmd(cmd *cobra.Command, args []string) (err error) { } } else { pks := make([]cryptotypes.PubKey, len(args)) - keyFilter := make(map[string]struct{}) + seenKeys := make(map[string]struct{}) for i, keyRef := range args { - if _, ok := keyFilter[keyRef]; ok { - cmd.Printf("WARNING: duplicate keys found: %s.\n", keyRef) + if _, ok := seenKeys[keyRef]; ok { + // we just show warning message instead of return error in case someone relies on this behavior. + cmd.PrintErrf("WARNING: duplicate keys found: %s.\n\n", keyRef) } else { - keyFilter[keyRef] = struct{}{} + seenKeys[keyRef] = struct{}{} } + k, err := fetchKey(clientCtx.Keyring, keyRef, clientCtx.AddressCodec) if err != nil { return fmt.Errorf("%s is not a valid name or address: %w", keyRef, err) } + key, err := k.GetPubKey() if err != nil { return err diff --git a/client/keys/show_test.go b/client/keys/show_test.go index 2aafd19f912b..5d7f9c033263 100644 --- a/client/keys/show_test.go +++ b/client/keys/show_test.go @@ -134,7 +134,8 @@ func Test_runShowCmd(t *testing.T) { }) require.EqualError(t, cmd.ExecuteContext(ctx), "threshold must be a positive integer") - // Now try multisig key duplicate(we just show warning message instead of return error) + // Now try multisig key duplicate + _, mockOut := testutil.ApplyMockIO(cmd) cmd.SetArgs([]string{ fakeKeyName1, fakeKeyName1, fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), @@ -143,6 +144,7 @@ func Test_runShowCmd(t *testing.T) { fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), }) require.NoError(t, cmd.ExecuteContext(ctx)) + require.Contains(t, mockOut.String(), fmt.Sprintf("WARNING: duplicate keys found: %s", fakeKeyName1)) cmd.SetArgs([]string{ fakeKeyName1, fakeKeyName2,