Simplify #51
Simplify #51
Conversation
README.md
Outdated
|
|
||
| ## Learn more | ||
| ## Guide | ||
|
|
||
| 1. Getting started with the [Basecoin tool](/docs/guide/basecoin-basics.md) |
|
Very nice. I especially like the merging of genesis files, consolidation to one chain id, and the special format for accounts, to make that more clear, as that is the most common usage of the basecoin config. I wonder if we should make BCHOME required? Without the default to This whole refactor really makes basecoin much more of a stand-alone app, and great intro. Now, I just need to get that light-client and a UI working nicely with it... |
|
Thanks frey. I think we should keep the default rather than forcing users to set it just to get started. Seems even Go adopted a default GOPATH :) |
| return nil | ||
| } | ||
|
|
||
| const privValJSON = `{ |
There was a problem hiding this comment.
Curious as to why are all these json constants hard coded in? aka what is the purpose of not having them in json files that copy? It looks like we're creating new default json files. may make sense to copy these files as to not need to change source code if one wanted to change init parameters. just an idea/not certain on the drawbacks
There was a problem hiding this comment.
It's just simpler than needing to use a file. We should probably have no defaults and generate new keys/genesis on init but then we can't have a deterministic tutorial.
There was a problem hiding this comment.
Yeah, definitely need to be able to easily use this for the tutorial.
| }, | ||
| } | ||
|
|
||
| func cmdUnsafeResetAll(c *cli.Context) error { |
There was a problem hiding this comment.
This looks like copy-paste from tendermint-core. Should reference common code if possible
There was a problem hiding this comment.
once all the cobra/cli stuff is merged we can go back through these and replace the ones that are just tendermint
| key1File := path.Join(rootDir, "key.json") | ||
| key2File := path.Join(rootDir, "key2.json") | ||
|
|
||
| if err := ioutil.WriteFile(genesisFile, []byte(genesisJSON), 0644); err != nil { |
There was a problem hiding this comment.
For readability, during refactor to cobra, error returns are not-necessarily needed can just exit. As so, we can condense code further using pkg/errors (for tracing) and a function variable, will implement during refactor
exitOnErr := func(err error){
if err != nil {
cmn.Exit(fmt.Sprintf("%+v\n", err))
}
}
exitOnErr(ioutil.WriteFile(genesisFile, []byte(genesisJSON), 0644))
... There was a problem hiding this comment.
This is one of those things that looks like a good idea at first and is discovered to have been a mistake later. If we ever want this functionality to be useable from a library, its a pain to have to go pull out the exits - best to just return errors.
There was a problem hiding this comment.
hmm.. alright let's talk about this more later, I have had no trouble using traceable errors while debugging checking out failed tests etc. + I find it a whole lot easier to navigate the code.
| }, | ||
| } | ||
|
|
||
| func cmdInit(c *cli.Context) error { |
There was a problem hiding this comment.
Is there a functional difference (besides the directory) between basecoin init and tendermint init noticed code is totally different, here if just looks like we're writing in some files, don't fully understand everything happening in tendermint init. If there is no other functional difference we should find a way to rectify the two functions to be able to call some core common code if possible. The refactor to cobra should make this easier.
There was a problem hiding this comment.
tendermint creates a new key and makes a genesis from that. here we're using hard-coded. when the tendermint cli refactor is merged we can revisit and maybe have a basecoin init --default for sake of deterministic tutorials.
There was a problem hiding this comment.
I like the idea of having a deterministic option strictly for tutorial usage... maybe a little bit of a discussion in the tutorial explaining this
cmd/commands/start.go
Outdated
| err := basecoinApp.LoadGenesis(genesisFile) | ||
| if err != nil { | ||
| return errors.New(cmn.Fmt("%+v", err)) | ||
| fmt.Println("CHAIN ID", basecoinApp.GetState().GetChainID()) |
There was a problem hiding this comment.
do we need to print CHAIN ID twice? If it is empty, the client will get
"CHAIN ID"
"CHAIN ID test"
if not empty
"CHAIN ID test"
"CHAIN ID test"
There was a problem hiding this comment.
No we don't this is old debugging - thanks.
Simplify starting basecoin:
--in-proc, add--without-tendermintBCHOMEand removedata/(https://github.com/tendermint/basecoin/issues/49)initandunsafe_reset_allcommands (https://github.com/tendermint/basecoin/issues/48)app_optionsand has well structured fields, plus a newplugin_optionsfor the amorphous array of key/value pairs for SetOptionFix up docs and guides