Update to Cobra CLI #64

rigelrozanski · 2017-04-02T04:36:13Z

Closes #62, Closes #61, Closes #46
In conjunction with Basecoin-Examples PR tendermint/basecoin-examples#8

Updated CLI, new "simplified" basecoin, and all associated documentation/tutorials

ethanfrey · 2017-04-03T08:49:55Z

Modified init to be more robust when an existing validator key is there but no genesis file.

Also, do we really want to print "%+v" for the error all the time. For debugging, the stack trace can be nice, but when using it and I type in an empty account, I just want the message, not the stack dump.

Maybe a separate flag "--verbose" / "--debug" so it will print this info, and just "%v" otherwise? You would just have to extract this idiom: cmn.Exit(fmt.Sprintf("%+v\n", err)) into a basecoin local helper, which checks the verbose flag itself.

rigelrozanski · 2017-04-03T19:37:25Z

hmm okay, yeah I agree that it would be nice to not have the stack dump when we're not debugging... Even easier just reference a string variable that can be either "v%" or "v+%" depending on if --debug is used. @ebuchman thoughts too?

ethanfrey · 2017-04-04T12:13:18Z

How about a new function in the cmd/commands directory:

func dieOnError(err error) {
  if err != nil {
    form := "%v"
    if viper.GetBool("debug") {
      form = "%+v"
    }
    cmn.Exit(fmt.Sprintf(form, err))
  }
}

Note the first check, so it does nothing if err is nil, and you can safely call it without checking..

err := foo()
dieOnError(err)

ethanfrey · 2017-04-04T12:14:14Z

You do use viper? Or not? Otherwise change the viper.GetBool to whatever makes sense to you. Or see how I register all the flags on viper in go-keys

rigelrozanski · 2017-04-04T21:58:01Z

Okay cool. Yeah viper is not used yet. Going to update tendermint and basecoin to use viper this week. @ebuchman and I have had a few discussions on using functions to check errors as opposed to checking them in place, the resolution is that we've decided to check errors in place to maintain portability... However I wonder if there is a way to do both... such as check the error in place first, but also if the debug flag is on, do a stack trace print and exit, an alternative util function might look something like this (which would be called after checking for the error in place)

func debug(err error) {
   if viper.GetBool("debug") && err != nil {
      cmn.Exit(fmt.Sprintf(%+v, err))
   }
}

ethanfrey · 2017-04-07T11:36:49Z

Let @ebuchman decide and please write the prefered mechanism in the style-guide

rigelrozanski · 2017-04-07T23:25:11Z

@ethanfrey Okay sounds good

ebuchman · 2017-04-13T23:38:16Z

--debug seems fine. The stack trace doesn't buy us much in most cases. Though in some sense it balances exiting in these functions. My personal preference is to return errors all the way to the top and exiting in one place only. Errors can include a stack trace when its useful

rigelrozanski · 2017-04-15T19:14:28Z

@ebuchman --debug has been added as well as returning errors all the way up

ebuchman

Great work!

ebuchman · 2017-04-17T22:12:53Z

cmd/commands/ibc.go

+	// register flags
+	registerFlags := []Flag2Register{
+		{&ibcChainIDFlag, "ibc_chain_id", "", "ChainID for the new blockchain"},
+		{&ibcGenesisFlag, "genesis", "", "Genesis file for the new blockchain"},


maybe for consistency all these flags should have ibc_. that could also avoid breaking changes later

actually leave this for now and we can discuss as a team after some QA testing. fine to have this on develop before a final decision

leaving for now

ebuchman · 2017-04-17T22:18:01Z

cmd/commands/init.go

+	}
+)
+
+// setupFile aborts on error... or should we return it??


looks like it always returns now

ebuchman · 2017-04-17T22:19:50Z

cmd/commands/init.go

+// returns 1 iff it set a file, otherwise 0 (so we can add them)
+func setupFile(path, data string, perm os.FileMode) (int, error) {
+	_, err := os.Stat(path)
+	if !os.IsNotExist(err) {


shouldnt this just be os.IsExist ?

causes a permission denied error if we use os.IsExist, this check is the same as in tendermint core, leaving (will add a comment)

ebuchman · 2017-04-17T22:20:36Z

cmd/commands/key.go

 	key := genKey()
 	keyJSON, err := json.MarshalIndent(key, "", "\t")
+	fmt.Println(&key)


ebuchman · 2017-04-17T22:46:57Z

cmd/commands/query.go

-	if err != nil {
-		return errors.New(cmn.Fmt("Proof (%v) is invalid hex: %v", c.String("proof"), err))
-	}
+	proofBytes, err := hex.DecodeString(StripHex(proofFlag))


check this error

whoops. done

ebuchman · 2017-04-17T23:33:19Z

cmd/commands/tx.go

+		{&amountFlag, "amount", "", "Coins to send in transaction of the format <amt><coin>,<amt2><coin2>,... (eg: 1btc,2gold,5silver},"},
+		{&gasFlag, "gas", 0, "The amount of gas for the transaction"},
+		{&feeFlag, "fee", "", "Coins for the transaction fee of the format <amt><coin>"},
+		{&seqFlag, "sequence", -1, "Sequence number for the account (-1 to autocalculate},"},


again, } should be )

ebuchman · 2017-04-17T23:35:47Z

cmd/commands/utils.go

@@ -21,16 +23,87 @@ import (
 	tmtypes "github.com/tendermint/tendermint/types"
 )

+//This variable can be overwritten by plugin applications
+// if they require a different working directory
+var DefaultHome = "basecoin"


ebuchman · 2017-04-17T23:36:00Z

cmd/commands/utils.go

 func BasecoinRoot(rootDir string) string {
 	if rootDir == "" {
 		rootDir = os.Getenv("BCHOME")
 	}
 	if rootDir == "" {
-		rootDir = os.Getenv("HOME") + "/.basecoin"
+		rootDir = os.Getenv("HOME") + "/." + DefaultHome


path.Join(os.Getenv("HOME"), DefaultHome)

ebuchman · 2017-04-17T23:41:03Z

cmd/commands/utils.go

+	var debug bool
+	RootCmd.PersistentFlags().BoolVar(&debug, "debug", false, "enables stack trace error messages")
+
+	//note that Execute() prints the error if encountered, so no need to reprint the error,


This is bad, because it also prints the help menu, which may be totally irrelevant.

solved with cmd.SilenceUsage but update the comment.

ebuchman · 2017-04-17T23:43:04Z

cmd/commands/utils.go

+	}
+}
+
+type Flag2Register struct {


neat. a comment would be helpful

ebuchman · 2017-04-18T03:58:44Z

cmd/commands/init.go

 // returns 1 iff it set a file, otherwise 0 (so we can add them)
 func setupFile(path, data string, perm os.FileMode) (int, error) {
 	_, err := os.Stat(path)
-	if !os.IsNotExist(err) {
+	if !os.IsNotExist(err) { //permission errors generated if use os.IsExist


Investigate

See this gist for a pretty informative description as to what is going on, looks like os.IsExist is not appropriate here https://gist.github.com/mastef/05f46d3ab2f5ed6a6787#file-isexist_vs_isnotexist-go-L35-L43

I think we can just check the err == nil. But fine.

ebuchman · 2017-04-18T03:59:15Z

cmd/commands/query.go

@@ -198,6 +198,9 @@ func verifyCmd(cmd *cobra.Command, args []string) error {
 	}

 	proofBytes, err := hex.DecodeString(StripHex(proofFlag))
+	if err != nil {
+		return errors.Errorf("Proof (%v) is invalid hex: %v\n", proofBytes, err)


should be proofFlag here, the bytes will be nil if there's an err

ebuchman · 2017-04-18T03:59:59Z

cmd/commands/utils.go

@@ -25,14 +26,14 @@ import (

 //This variable can be overwritten by plugin applications
 // if they require a different working directory
-var DefaultHome = "basecoin"
+var DefaultHome = ".basecoin"


actually we should probably just put the whole path here

This variable exists because it is changed by each basecoin plugin which may require a modified genesis (and the original space should not be overwritten). Only if we choose not to enforce use of the home directory for default basecoin-plugin working spaces should be we change this. If we choose to enforce using the home directory (which I had assumed we would) then it does not make sense to modify this to include the full path as it would mean that each plugin would be duplicating the code os.Getenv("Home").. Just considering

ebuchman · 2017-04-18T04:57:15Z

I'll handle my comments and the rebase tmrw.

rigelrozanski · 2017-04-18T06:52:24Z

Okay made minor fixes/left rebase and decision/code changes on your issues based on my comment - to you

rebaseFixes int removed ExitOnErr int int int int int added uint64 to RegisterFlags

int

int int int int

Merge sdk2 into develop

* Fix ISSUE #64 on cosmosdevs/stargate * Fix ISSUE #64 on cosmosdevs/stargate * Update x/upgrade/client/cli/tx.go Co-authored-by: Aleksandr Bezobchuk <[email protected]>

rigelrozanski mentioned this pull request Apr 2, 2017

Update to Cobra CLI tendermint/basecoin-examples#8

Merged

rigelrozanski force-pushed the cli_cobra branch from 4cdf57a to 318ce68 Compare April 10, 2017 02:26

rigelrozanski mentioned this pull request Apr 10, 2017

go-config -> viper, commands: Run -> RunE tendermint/tendermint#442

Merged

rigelrozanski force-pushed the cli_cobra branch from 318ce68 to 1b1e35a Compare April 13, 2017 22:14

rigelrozanski force-pushed the cli_cobra branch from c6f118e to 774d1f9 Compare April 15, 2017 19:13

ebuchman force-pushed the cli_cobra branch from 774d1f9 to 54e3c9a Compare April 17, 2017 21:55

ebuchman requested changes Apr 17, 2017

View reviewed changes

ebuchman reviewed Apr 18, 2017

View reviewed changes

rigelrozanski and others added 9 commits April 21, 2017 12:34

added cli cobra

fb3fd1b

rebaseFixes int removed ExitOnErr int int int int int added uint64 to RegisterFlags

cmd init cleanup

6eb5116

v0.4.0

18f1142

int

doc update

02b4fd5

BCHOME default dir change

b60845c

int int int int

Make initializing files more robust to some files existing

587461a

rebase updates/ rpc updates

5ebdd96

Debug Flag, Run -> RunE

c292d54

cmd.SilenceUsage

04f79f3

rigelrozanski added 3 commits April 21, 2017 12:34

tiny fixes

6da5f72

minor fix x2

55e4cf1

tendermint dep branch: cli_change -> viper

34fc383

ethanfrey force-pushed the cli_cobra branch from 271ddce to e35fb9b Compare April 21, 2017 16:43

glide update and rebase fixes

9d620e4

ebuchman force-pushed the cli_cobra branch from e35fb9b to 9d620e4 Compare April 21, 2017 16:56

ebuchman approved these changes Apr 21, 2017

View reviewed changes

ebuchman merged commit 488f3e1 into develop Apr 21, 2017

ethanfrey deleted the cli_cobra branch May 29, 2017 15:38

liamsi pushed a commit to liamsi/cosmos-sdk that referenced this pull request Jun 26, 2018

Merge pull request cosmos#64 from tendermint/sdk2

4542cc5

Merge sdk2 into develop

wimel added a commit to wimel/cosmos-sdk that referenced this pull request Oct 9, 2020

Fix ISSUE cosmos#64 on cosmosdevs/stargate

d1bbd81

wimel added a commit to wimel/cosmos-sdk that referenced this pull request Oct 9, 2020

Fix ISSUE cosmos#64 on cosmosdevs/stargate

93832bb

mergify bot pushed a commit that referenced this pull request Oct 9, 2020

Fix on gov module (#7493)

278eaa7

* Fix ISSUE #64 on cosmosdevs/stargate * Fix ISSUE #64 on cosmosdevs/stargate * Update x/upgrade/client/cli/tx.go Co-authored-by: Aleksandr Bezobchuk <[email protected]>

hleb-albau mentioned this pull request Jan 26, 2022

feat: add slash validator proposal #11032

Closed

19 tasks

Update to Cobra CLI #64

Update to Cobra CLI #64

Conversation

rigelrozanski commented Apr 2, 2017 • edited Loading

ethanfrey commented Apr 3, 2017

rigelrozanski commented Apr 3, 2017 • edited Loading

ethanfrey commented Apr 4, 2017

ethanfrey commented Apr 4, 2017

rigelrozanski commented Apr 4, 2017

ethanfrey commented Apr 7, 2017

rigelrozanski commented Apr 7, 2017

ebuchman commented Apr 13, 2017

rigelrozanski commented Apr 15, 2017

ebuchman left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski Apr 18, 2017 • 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

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

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

ebuchman commented Apr 18, 2017

rigelrozanski commented Apr 18, 2017

rigelrozanski commented Apr 2, 2017 •

edited

Loading

rigelrozanski commented Apr 3, 2017 •

edited

Loading

rigelrozanski Apr 18, 2017 •

edited

Loading