-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
R4R: Add validate-genesis command #3399
Conversation
cmd/gaia/init/validate_genesis.go
Outdated
RunE: func(cmd *cobra.Command, args []string) (err error) { | ||
var genesis string | ||
var genDoc types.GenesisDoc | ||
var genstate app.GenesisState |
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.
These vars should be defined right before their first usage (aka genesis
on line 31 etc.)
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.
👍
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.
one minor comment to be addressed, otherwise LGTM
Codecov Report
@@ Coverage Diff @@
## develop #3399 +/- ##
==========================================
- Coverage 59.38% 59.19% -0.2%
==========================================
Files 131 132 +1
Lines 9862 9884 +22
==========================================
- Hits 5857 5851 -6
- Misses 3632 3660 +28
Partials 373 373 |
} else { | ||
genesis = args[0] | ||
} | ||
|
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'd add a diagnostic info here just to inform the user which location genesis.json
is being read from:
fmt.Fprintf(os.Stderr, "validating %s", genesis)
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.
Great add! Thank you!
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.
Minor comment, not a blocker though. Good to go for me.
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'm sorry, forgot one thing: please add a cli test
Thank you for keeping me honest @alessio will do! |
@jackzampolin any updates on this? Also, |
@alexanderbez I did update it Satuday, need to adjust test to not fail. |
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.
ACK - pending tests pass
Fixes: #3398
PENDING.md
with issue #Files changed
in the github PR explorerFor Admin Use: