-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auto Sequencing, Error Cleanup, Bash Tests Cleanup #177
Auto Sequencing, Error Cleanup, Bash Tests Cleanup #177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job. I'll merge soon, take a look at the comments if you wish.
errUnknownModule = fmt.Errorf("Unknown module") | ||
errExpired = fmt.Errorf("Tx expired") | ||
errUnknownKey = fmt.Errorf("Unknown key") | ||
errDecoding = fmt.Errorf("Error decoding input") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, very correct with moving the errors
} | ||
|
||
func doAccountQuery(cmd *cobra.Command, args []string) error { | ||
func accountQueryCmd(cmd *cobra.Command, args []string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i forgot the naming scheme
@@ -80,3 +81,11 @@ func ErrNoOutputs() errors.TMError { | |||
func IsNoOutputsErr(err error) bool { | |||
return errors.IsSameError(errNoOutputs, err) | |||
} | |||
|
|||
func ErrUnknownKey(mod string) errors.TMError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also do: return errors.WithMessage(mod, errUnknownKey, unknownRequest)
just using the basecoin.errors package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh awesome I was mostly copying and pasting code from the other files, changing now. cleaner as now need to import pkg/errors
modules/nonce/commands/query.go
Outdated
@@ -6,9 +6,10 @@ import ( | |||
"github.com/pkg/errors" | |||
"github.com/spf13/cobra" | |||
|
|||
lc "github.com/tendermint/light-client" | |||
"github.com/tendermint/basecoin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a re-ordering of the imports... light-client and other tendermint together, then basecoin stuff...
I know they are all a bit mixed up after the merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good eye, done
modules/nonce/commands/query.go
Outdated
if len(args) == 0 { | ||
return errors.New("Missing required argument [address]") | ||
} | ||
addr := strings.Join(args, ",") | ||
act, err := parseActors(addr) | ||
|
||
var signers []basecoin.Actor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to define signers here when you do a :=
below. I think it is clear without the var
, but if you use it, change the :=
to =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, thanks, relic
@@ -67,13 +69,28 @@ func parseActors(key string) (signers []basecoin.Actor, err error) { | |||
return | |||
} | |||
|
|||
func readSequence() (uint32, error) { | |||
// read the sequence from the flag or query for it if flag is -1 | |||
func readSequence(signers []basecoin.Actor) (seq uint32, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks very good
@@ -53,7 +53,8 @@ test02SendTxWithFee() { | |||
SENDER=$(getAddr $RICH) | |||
RECV=$(getAddr $POOR) | |||
|
|||
TX=$(echo qwertyuiop | ${CLIENT_EXE} tx send --amount=90mycoin --fee=10mycoin --sequence=2 --to=$RECV --name=$RICH) | |||
# Test to see if the auto-sequencing works, the sequence here should be calculated to be 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice.
@@ -4,103 +4,103 @@ CLIENT_EXE=basecli | |||
SERVER_EXE=basecoin | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the change you made here on all the lines? tabs to spaces?
let's get things straight with our editors to not have this issue in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, please use 4 spaces as tabs on bash files you were using two. More in the coding guide: https://github.com/tendermint/coding/blob/master/Coding_Standard.md#non-golang-code
I have this automatically built in to format when I save a .sh
in vim
(cherry picked from commit 66442bf) Co-authored-by: Roman <[email protected]>
No description provided.