Misc CLI fixes #812

cwgoes · 2018-04-06T16:09:38Z

codecov · 2018-04-06T16:13:14Z

Codecov Report

Merging #812 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #812   +/-   ##
========================================
  Coverage    65.18%   65.18%           
========================================
  Files           67       67           
  Lines         3539     3539           
========================================
  Hits          2307     2307           
  Misses        1045     1045           
  Partials       187      187

cwgoes · 2018-04-09T14:07:07Z

Ref #807

cwgoes · 2018-04-09T21:01:48Z

Ready for review.

rigelrozanski

Good stuff, few comments herein also we should include tests for NextSequence and AutoSequence

rigelrozanski · 2018-04-10T01:29:30Z

client/context/viper.go

+			if err == nil {
+				var doc tmtypes.GenesisDoc
+				err = json.Unmarshal(bz, &doc)
+				if err == nil {


Bad practice to nest if statements like this. I understand that we don't return an error in this function so you are nesting the operations but this suggests to me that this nest should be a seperate function which does return an error.

Good point, updated.

rigelrozanski · 2018-04-10T01:30:50Z

client/context/viper.go

+
+// Automatically set sequence number
+func AutoSequence(ctx core.CoreContext) (core.CoreContext, error) {
+	if !viper.IsSet(client.FlagSequence) {


same thing with the nesting here, instead you could check the opposite and just return if it's true:

if viper.IsSet(client.FlagSequence) { return ctx nil }

rigelrozanski · 2018-04-10T01:38:00Z

client/context/viper.go

+}
+
+// Automatically set sequence number
+func AutoSequence(ctx core.CoreContext) (core.CoreContext, error) {


This function name is slightly misleading because it's not always auto-sequencing it's only sequencing when there isn't a sequence number provided - but I get that you're trying to avoid dup code in all the command functions.. Maybe a more appropriate name would be SetSequence or EnsureSequence

Changed to EnsureSequence.

rigelrozanski · 2018-04-10T01:39:06Z

client/core/context.go

@@ -48,3 +52,13 @@ func (c CoreContext) WithClient(client rpcclient.Client) CoreContext {
 	c.Client = client
 	return c
 }
+
+func (c CoreContext) WithDecoder(decoder sdk.AccountDecoder) CoreContext {


Please add comment headers - what's dis stuff doing anyways

Makes it easier to change context parameters one-at-a-time (the IBC module uses these functions quite a bit, for example).

Comment headers added.

rigelrozanski · 2018-04-10T01:39:10Z

client/core/context.go

+	return c
+}
+
+func (c CoreContext) WithAccountStore(accountStore string) CoreContext {


Please add comment headers

Header added.

rigelrozanski · 2018-04-10T01:40:15Z

client/core/core.go

@@ -137,6 +140,24 @@ func (ctx CoreContext) SignBuildBroadcast(name string, msg sdk.Msg, cdc *wire.Co
 	return ctx.BroadcastTx(txBytes)
 }

+func (c CoreContext) NextSequence(address []byte) (int64, error) {


Function header comments please

Header added.

uh.. not quite hehehe - I just pushed with a comment though

Oops, too many windows open apparently. Thanks!

…810)

cwgoes · 2018-04-10T09:55:50Z

Also in favor of tests, but not sure how best to implement - we'll need an app with accounts to test sequence retrieval, doesn't make sense to import Basecoin in the context package. Maybe modules wanting to use sequence retrieval should test it with their own AccountDecoders?

All other comments addressed.

Addressed, except tests (see comment).

rigelrozanski · 2018-04-10T21:38:30Z

cool - let's finalize the sequence testing as a part of this issue when we address #816

melekes · 2018-04-11T08:42:39Z

client/context/viper.go

+		return "", err
+	}
+	genesisFile := cfg.GenesisFile()
+	bz, err := ioutil.ReadFile(genesisFile)


please use https://godoc.org/github.com/tendermint/tendermint/types#GenesisDocFromFile

melekes · 2018-04-11T08:43:15Z

client/core/context.go

 func (c CoreContext) WithChainID(chainID string) CoreContext {
 	c.ChainID = chainID
 	return c
 }

+// WithHeight - eturn a copy of the context with an updated height


Address PR comments on #812

cwgoes requested a review from ebuchman as a code owner April 6, 2018 16:09

cwgoes force-pushed the cwgoes/misc-cli-fixes branch 2 times, most recently from ae87ed2 to a13a4d4 Compare April 9, 2018 14:07

cwgoes force-pushed the cwgoes/misc-cli-fixes branch 3 times, most recently from 6155d45 to 4d4a7ee Compare April 9, 2018 19:06

cwgoes changed the title ~~WIP: Misc CLI fixes~~ Misc CLI fixes Apr 9, 2018

rigelrozanski previously requested changes Apr 10, 2018

View reviewed changes

cwgoes added 5 commits April 10, 2018 11:16

Automatically determine chainID, throw error if not provided (closes #…

0be655b

…810)

Default to next sequence number (closes #807)

ec98545

Helper function

bd626ba

Add AutoSequence to all transaction-sending commands

7383c99

Address PR comments

c7b680a

cwgoes force-pushed the cwgoes/misc-cli-fixes branch from f2ae070 to c7b680a Compare April 10, 2018 09:17

rigelrozanski mentioned this pull request Apr 10, 2018

Move out modules unit tests from basecoin #816

Closed

next sequence comment, CHANGELOG updates

c741be1

rigelrozanski approved these changes Apr 10, 2018

View reviewed changes

melekes reviewed Apr 11, 2018

View reviewed changes

rigelrozanski merged commit 5212ac0 into develop Apr 11, 2018

rigelrozanski deleted the cwgoes/misc-cli-fixes branch April 11, 2018 15:42

rigelrozanski restored the cwgoes/misc-cli-fixes branch April 11, 2018 15:43

cwgoes mentioned this pull request Apr 11, 2018

Address PR comments on #812 #847

Merged

rigelrozanski added a commit that referenced this pull request Apr 11, 2018

Merge pull request #847 from cosmos/cwgoes/misc-cli-fixes

8aaf1b6

Address PR comments on #812

cwgoes mentioned this pull request Apr 12, 2018

basecli txs: automatically figure out sequence number #807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc CLI fixes #812

Misc CLI fixes #812

cwgoes commented Apr 6, 2018 •

edited

Loading

codecov bot commented Apr 6, 2018 •

edited

Loading

cwgoes commented Apr 9, 2018

cwgoes commented Apr 9, 2018

rigelrozanski left a comment •

edited

Loading

rigelrozanski Apr 10, 2018

cwgoes Apr 10, 2018

rigelrozanski Apr 10, 2018

cwgoes Apr 10, 2018

rigelrozanski Apr 10, 2018

cwgoes Apr 10, 2018

rigelrozanski Apr 10, 2018

cwgoes Apr 10, 2018

rigelrozanski Apr 10, 2018

cwgoes Apr 10, 2018

rigelrozanski Apr 10, 2018

cwgoes Apr 10, 2018

rigelrozanski Apr 10, 2018

cwgoes Apr 10, 2018

cwgoes commented Apr 10, 2018

rigelrozanski commented Apr 10, 2018

melekes Apr 11, 2018

melekes Apr 11, 2018

Misc CLI fixes #812

Misc CLI fixes #812

Conversation

cwgoes commented Apr 6, 2018 • edited Loading

codecov bot commented Apr 6, 2018 • edited Loading

Codecov Report

cwgoes commented Apr 9, 2018

cwgoes commented Apr 9, 2018

rigelrozanski left a comment • 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

cwgoes commented Apr 10, 2018

rigelrozanski commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Apr 6, 2018 •

edited

Loading

codecov bot commented Apr 6, 2018 •

edited

Loading

rigelrozanski left a comment •

edited

Loading