feat: add flag for the private key selection #43

Kynea0b · 2020-12-17T04:52:21Z

Description

FlagPrivKeyType is added to the flags.go.

Motivation and context

This flag is required when issuing a transaction with the privatekey type set to composite. This flag is used by setting it in the command with link.

How has this been tested?

Please set and execute as follows in the link project.

Describe the go.mod file of link as follows.:

replace (
	github.com/cosmos/cosmos-sdk => github.com/line/cosmos-sdk [tagname]
)

And this command:

make install

#!/usr/bin/env bash
set -ex

# initialize
rm -rf ~/.linkd ~/.linkcli

# Configure your CLI to eliminate need for chain-id flag
linkcli config chain-id link
linkcli config output json
linkcli config indent true
linkcli config trust-node true
linkcli config keyring-backend test

# Initialize configuration files and genesis file
# moniker is the name of your node
linkd init solo --chain-id link --priv_key_type composite


linkcli keys add jack
linkcli keys add alice
linkcli keys add bob
linkcli keys add rinah
linkcli keys add sam
linkcli keys add evelyn


# Add both accounts, with coins to the genesis file
linkd add-genesis-account $(linkcli keys show jack -a) 1000link,100000000stake
linkd add-genesis-account $(linkcli keys show alice -a) 1000link,100000000stake
linkd add-genesis-account $(linkcli keys show bob -a) 1000link,100000000stake
linkd add-genesis-account $(linkcli keys show rinah -a) 1000link,100000000stake
linkd add-genesis-account $(linkcli keys show sam -a) 1000link,100000000stake
linkd add-genesis-account $(linkcli keys show evelyn -a) 1000link,100000000stake

linkd --keyring-backend=test gentx --name jack --priv_key_type composite

linkd collect-gentxs --priv_key_type composite

linkd validate-genesis

Checklist:

I followed the contributing guidelines.

Ref:
Finschia/ostracon#125
https://github.com/line/link/issues/1111
https://github.com/line/link/pull/1133

egonspace

LGTM

torao · 2021-01-18T09:05:27Z

x/genutil/client/cli/collect.go

@@ -64,6 +64,7 @@ func CollectGenTxsCmd(ctx *server.Context, cdc *codec.Codec,
 cmd.Flags().String(flagGenTxDir, "",
 "override default \"gentx\" directory from which collect and execute "+
 "genesis transactions; default [--home]/config/gentx/")
+ cmd.Flags().String(flags.FlagPrivKeyType, flags.DefaultPrivKeyType, "Select private key type")


The descriptions of other flags seem to begin with a lower case.

I feel this explanation is too simple. What does the "private key type" here mean to the CLI operator?

Please write the default behaviour, and the available choices.

Thank you. I will fix the above.

By specifying the priv_key_type=composite, set priv_key.type in the file(.linkd/config/priv_validator.json) to tendermint/PrivKeyComposite when initializing or creating the validator. By default, priv_key.type is set to tendermint/PrivKeyEd25519. So, I willl write this explanation.

I fixed. PTAL.

I think it's still lack of explanation. I think the description should show what arguments we could input. Here is a sample description from code.

"Select keyring's backend (os|file|test)"

With current description, I couldn't know what arguments I could input.

Thank you both. I added a description of the default value ed25519 and what arguments we could input.

torao · 2021-01-18T09:10:14Z

x/genutil/client/cli/gentx.go

@@ -190,6 +190,7 @@ func GenTxCmd(ctx *server.Context, cdc *codec.Codec, mbm module.BasicManager, sm
 cmd.Flags().String(flags.FlagKeyringBackend, flags.DefaultKeyringBackend, "Select keyring's backend (os|file|test)")
 viper.BindPFlag(flags.FlagKeyringBackend, cmd.Flags().Lookup(flags.FlagKeyringBackend))

+ cmd.Flags().String(flags.FlagPrivKeyType, flags.DefaultPrivKeyType, "Select private key type")


ditto. (The item above it seems to begin with an upper case..?)

Looking at the other flag descriptions, some start with a uppercase letter and some start with a lowercase letter. It doesn't seem to be unified. This time I will make it lowercase.

I fixed. PTAL.

torao · 2021-01-18T09:10:27Z

x/genutil/client/cli/init.go

@@ -121,6 +121,7 @@ func InitCmd(ctx *server.Context, cdc *codec.Codec, mbm module.BasicManager,
 cmd.Flags().String(cli.HomeFlag, defaultNodeHome, "node's home directory")
 cmd.Flags().BoolP(flagOverwrite, "o", false, "overwrite the genesis.json file")
 cmd.Flags().String(flags.FlagChainID, "", "genesis file chain-id, if left blank will be randomly created")
+ cmd.Flags().String(flags.FlagPrivKeyType, flags.DefaultPrivKeyType, "Select private key type")


I fixed. PTAL.

jinsan-line · 2021-01-20T05:46:13Z

I found that same code occurs 3 times in this PR. Is it really needed? Could we follow DRY principle?

https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

client/flags/flags.go

jinsan-line · 2021-01-20T05:49:21Z

client/flags/flags.go

@@ -62,13 +62,15 @@ const (
 FlagKeyringBackend = "keyring-backend"
 FlagPage = "page"
 FlagLimit = "limit"
+ FlagPrivKeyType = "priv_key_type"


I think priv-key-type is better in terms of following local convention. Please note that all other flags use - as a delimiter but not _.

Thank you. I think so too, but since it is bound to the key name of the tendermint config file and is delimited by underscores _ in tendermint, I had no choice but to do this.
Have a look at this:
Finschia/ostracon#125

W/ Finschia/ostracon#125, I don't have confidence but it looks like tendermint already uses priv-key-type in the PR code even though the PR description shows priv_key_type.

https://github.com/line/tendermint/blob/0f9fe215f7d467cc25bdff022875c226bdeb246d/cmd/tendermint/commands/init.go#L29-L32

@egonspace Please could you double check it?

After that, it seems that it was changed to an underscore_ here. Please check it. @egonspace
Finschia/ostracon@b6420bc

I've got it. But, IMHO, I think we need to keep consistency even though revising tendermint as well if needed. I hope it's revised in the future.

wetcod · 2021-01-20T05:58:40Z

I can not find where to use the flag. Will it be added to the next PR?

Kynea0b · 2021-01-20T06:09:24Z

I can not find where to use the flag. Will it be added to the next PR?

Yes, it will be added to the next PR line/link. The command defined here is used in link.

wetcod · 2021-01-20T06:10:31Z

Yes, it will be added to the next PR line/link. The command defined here is used in link.

Oh, I see. Thank for your answer.

Kynea0b · 2021-01-20T06:17:08Z

I found that same code occurs 3 times in this PR. Is it really needed? Could we follow DRY principle?

@jinsan-line

The same settings are required for each of the init, gentx, and collect-gentx commands.
Not all of the linkd subcommands, but some of them, so I need to add the same flags in this way.

kukugi

LGTM 👍

Kynea0b requested review from tnasu, torao and egonspace December 17, 2020 05:13

egonspace previously approved these changes Dec 17, 2020

View reviewed changes

Kynea0b removed request for torao and tnasu December 22, 2020 05:42

Kynea0b dismissed egonspace’s stale review via 38e99e7 January 12, 2021 08:01

Kynea0b requested review from egonspace, torao and tnasu January 18, 2021 05:48

egonspace requested review from wetcod, jinsan-line and kukugi January 18, 2021 08:56

torao reviewed Jan 18, 2021

View reviewed changes

Kynea0b requested review from torao, tnasu, wetcod, jinsan-line and kukugi and removed request for egonspace, tnasu, wetcod, jinsan-line and kukugi January 18, 2021 09:18

jinsan-line reviewed Jan 20, 2021

View reviewed changes

client/flags/flags.go Show resolved Hide resolved

jinsan-line reviewed Jan 20, 2021

View reviewed changes

wetcod previously approved these changes Jan 20, 2021

View reviewed changes

kukugi previously approved these changes Jan 20, 2021

View reviewed changes

Kynea0b dismissed stale reviews from kukugi and wetcod via 46c3fe0 January 20, 2021 06:47

jinsan-line approved these changes Jan 20, 2021

View reviewed changes

tnasu approved these changes Jan 20, 2021

View reviewed changes

kukugi approved these changes Jan 20, 2021

View reviewed changes

Kynea0b added 2 commits January 20, 2021 18:08

feat: add flag for the private key selection

4b7aaa2

test: add an init test with composite to init_test

a592915

Kynea0b merged commit a4ef89b into Finschia:v0.38.5-0.1.0-linking_link_with_line_consensus Jan 20, 2021

Kynea0b self-assigned this Jan 21, 2021

This was referenced Feb 9, 2021

feat: add flag for the private key selection #73

Merged

test: add test whether pubkey is generated with composite type or not #75

Merged

egonspace pushed a commit that referenced this pull request Mar 28, 2021

ci: report perf results to PR comments (#43)

fb28833

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add flag for the private key selection #43

feat: add flag for the private key selection #43

Kynea0b commented Dec 17, 2020 •

edited

Loading

egonspace left a comment

torao Jan 18, 2021

Kynea0b Jan 18, 2021 •

edited

Loading

Kynea0b Jan 19, 2021 •

edited

Loading

Kynea0b Jan 20, 2021

jinsan-line Jan 20, 2021

Kynea0b Jan 20, 2021 •

edited

Loading

torao Jan 18, 2021

Kynea0b Jan 19, 2021

Kynea0b Jan 20, 2021

torao Jan 18, 2021

Kynea0b Jan 20, 2021

jinsan-line commented Jan 20, 2021

jinsan-line Jan 20, 2021

Kynea0b Jan 20, 2021

jinsan-line Jan 20, 2021 •

edited

Loading

jinsan-line Jan 20, 2021

Kynea0b Jan 20, 2021 •

edited

Loading

jinsan-line Jan 20, 2021 •

edited

Loading

wetcod commented Jan 20, 2021

Kynea0b commented Jan 20, 2021

wetcod commented Jan 20, 2021 •

edited

Loading

Kynea0b commented Jan 20, 2021

kukugi left a comment

feat: add flag for the private key selection #43

feat: add flag for the private key selection #43

Conversation

Kynea0b commented Dec 17, 2020 • edited Loading

Description

Motivation and context

How has this been tested?

Checklist:

egonspace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kynea0b Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Kynea0b Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kynea0b Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinsan-line commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinsan-line Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kynea0b Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

jinsan-line Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

wetcod commented Jan 20, 2021

Kynea0b commented Jan 20, 2021

wetcod commented Jan 20, 2021 • edited Loading

Kynea0b commented Jan 20, 2021

kukugi left a comment

Choose a reason for hiding this comment

Kynea0b commented Dec 17, 2020 •

edited

Loading

Kynea0b Jan 18, 2021 •

edited

Loading

Kynea0b Jan 19, 2021 •

edited

Loading

Kynea0b Jan 20, 2021 •

edited

Loading

jinsan-line Jan 20, 2021 •

edited

Loading

Kynea0b Jan 20, 2021 •

edited

Loading

jinsan-line Jan 20, 2021 •

edited

Loading

wetcod commented Jan 20, 2021 •

edited

Loading