Rewrite CLI: nknc and nknd

[omani]

Proposed changes in this pull request

Switch from urfave/cli to spf13/cobra. Same functionality, just different cli library, resulting in less loc.

Additionally, minor changes in code to fix a few warnings by compiler.

Type (put an x where ever applicable)

• Bug fix: Link to the issue
• Feature (Non-breaking change)
• Feature (Breaking change)
• Documentation Improvement

Extra information

cobra will have a new release around June 2022 with new features like MarkFlagsRequiredTogether() and MarkFlagsMutuallyExclusive() which will reduce the loc even further, since we won't have to check for set flags manually anymore.

Im planning to implement them once cobra releases v1.5.0.

[yilunzhang]

break should be changed to continue since you removed select

[yilunzhang]

We follow the convention to match package name with directory name. Since the directory name is commands, the package name should also be commands. Same for 'root.go

[yilunzhang]

I think it's better to submit the changes for nknc in the same PR so we can remove the dependency of urfave/cli.

[omani]

pushed nknc cli. fixed review.

[omani]

squashed.

[yilunzhang]

With the new structure, all commands (e.g. nknc wallet, nknc asset, etc) are defined in the same package, so all of them share private variables (e.g. fee for all commands are defined in asset.go). It's better to move those shared variable (e.g. fee, nonce) to a file that is not specific to a command, like root.go, this way removing or change one command won't affect the other ones.

[omani]

With the new structure, all commands (e.g. nknc wallet, nknc asset, etc) are defined in the same package, so all of them share private variables (e.g. fee for all commands are defined in asset.go). It's better to move those shared variable (e.g. fee, nonce) to a file that is not specific to a command, like root.go, this way removing or change one command won't affect the other ones.

yeah I already moved all shared vars to root.go. and kept all local (file scoped) vars to the appropriate files. to distinguish them from global vars. or in other words: if any var is being used in more than one file, it is declared in root.go. any other var is in the file itself.

so, as I understand it, you want me to make all vars global?

[yilunzhang]

yeah I already moved all shared vars to root.go. and kept all local (file scoped) vars to the appropriate files. to distinguish them from global vars. or in other words: if any var is being used in more than one file, it is declared in root.go. any other var is in the file itself.

This is exactly what I'm thinking as well, but there are a few variables that are used in more than one files but not defined in root.go. Specifically, fee and nonce are defined in asset.go but used in multiple files. I'm not sure if there are more variables like this but we can all check that.

[omani]

This is exactly what I'm thinking as well, but there are a few variables that are used in more than one files but not defined in root.go. Specifically, fee and nonce are defined in asset.go but used in multiple files. I'm not sure if there are more variables like this but we can all check that.

ok. I will check everything and commit soon.

[omani]

This is exactly what I'm thinking as well, but there are a few variables that are used in more than one files but not defined in root.go. Specifically, fee and nonce are defined in asset.go but used in multiple files. I'm not sure if there are more variables like this but we can all check that.

done

[yilunzhang]

Looks good to me

[omani]

rebased

[gdmmx]

LGTM

[yilunzhang]

Upon further review it seems that print version feature is missing. Previously nknc --version and nknd --version will print out nknc/nknd version (set in Makefile https://github.com/nknorg/nkn/blob/master/Makefile#L13), but I can't find it in the new code. Is it something missing here?

[omani]

Upon further review it seems that print version feature is missing. Previously nknc --version and nknd --version will print out nknc/nknd version (set in Makefile https://github.com/nknorg/nkn/blob/master/Makefile#L13), but I can't find it in the new code. Is it something missing here?

$ go run main.go --version
nknc version 1.0

$ go run main.go --version
nknd version 1.0

is what I get.

[yilunzhang]

Yep that's what I meant by missing because it's always 1.0... The expected behavior is to print version set in Makefile, to be more specific, it should be git tag and commit hash. You should pass version when creating cobra.Command (to config.Version) like spf13/cobra#943 and also change the part in Makefile L14 from
github.com/nknorg/nkn/v2/cmd/nknc/common.Version=$(VERSION)
to
github.com/nknorg/nkn/v2/config.Version=$(VERSION)

[omani]

NKND_BUILD_PARAM is already set to config.Version. can't we just get rid of NKNC_BUILD_PARAM in Makefile and (optionally rename the var to something better) and use only that var for both nknd and nknc? currently it only exists for assigning VERSION and doesnt do more.

[yilunzhang]

For now they are the same, but as you can see they might contain different build parameters in the future, so it's better to keep both NKND_BUILD_PARAM and NKNC_BUILD_PARAM. If you want to reduce redundancy I would suggest something like this:

NKNC_BUILD_PARAM=$NKND_BUILD_PARAM

[omani]

pls review the change. I kept the version flag for rootCmd only.

[yilunzhang]

Looks good to me!