[R4R] Update testnet to use canonical genesis time

[alexanderbez]

• General cleanup of init structure
• Support creating genesis file with a given canonical time

closes: #2691

---

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

• Wrote tests

• Updated relevant documentation (docs/)

• Added entries in PENDING.md with issue #

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (develop@2a3d1f1). Click here to learn what that means.
The diff coverage is 15.65%.

@@            Coverage Diff             @@
##             develop    #2692   +/-   ##
==========================================
  Coverage           ?   57.75%           
==========================================
  Files              ?      154           
  Lines              ?     9554           
  Branches           ?        0           
==========================================
  Hits               ?     5518           
  Misses             ?     3675           
  Partials           ?      361

[alessio]

Nihil obstat from me 👍

[cwgoes]

Looks like some of these changes might conflict with #2675 - also, localnet tests on CI still fail.

[alexanderbez]

Mhmmm it did work. My list commit broke it. Fixing it now. w.r.t #2675, that is still a WIP. Would rather get this in first and then we can update that. Thoughts?

[alexanderbez]

Ughhh silly variable names. We really need to adopt the flag* variable name convention.

[alexanderbez]

@cwgoes fixed.

@alessio I reverted the viper changes. This is because I found even more embedded viper usage in the auxiliary functions. It would be more changes than it's worth in this PR. I agree we should remove viper pollution, but since this is all in the context of the init command, I think it's fine...for now.

[cwgoes]

Tested ACK

[rigelrozanski]

i no like this style - adds an extra line, and an extra indent

[rigelrozanski]

additionally these variables should probably be moved to right before they are used and combined with those code blocks (aka genTxs and jsonRawTx definitions should be moved further in this function)

[alexanderbez]

Imho this code is much easier to read due to the indentation. I don't think it should block the PR.

[rigelrozanski]

this is a weird way to split up the func header, I'd suggest something along the lines of:

func initGenFiles(cdc *codec.Codec, chainID string,
     accs []app.GenesisAccount, genFiles []string) error {

[alexanderbez]

Ughhh I find that so much less elegant and harder to read imho. I'll be happy to change it if it's our standard though.

[alexanderbez]

What's prettier:

a.

foo := []string{"foo",
	"bar", "cat"}

b.

foo := []string{
	"foo", "bar", "cat",
}

[rigelrozanski]

k - yeah so-far the style I've suggested is used everywhere, so I think you should change it - we could open up a stylistic issue to discuss further (but post-launch change obv.).

I think functions are unique from struct definition, I don't find it adequate to draw the comparison (but ftr obv. b.). Part of what's nice about keeping func wrapping as I've suggested is that it's unique from how structs are defined - it becomes a visual cue as you're skimming through the code to notice that this a function not a struct

[if you create an issue should link it to this comment]

[alexanderbez]

Ok, I'm vetoed here haha. I'll change it. Will open up an issue later to discuss this more openly.

[rigelrozanski]

Same, feel weird about this style, suggest:

func collectGenFiles(cdc *codec.Codec, config *cfg.Config, chainID string,
	monikers, nodeIDs []string, valPubKeys []crypto.PubKey,) error {

[rigelrozanski]

same, let's not use viper this way, these should be inputs to the function and viper should be used only in the cobra commands

[rigelrozanski]

k just realized you're only re-organizing here, and it was already this way - so probably okay to just leave for this PR - however it would still be great if you wanted to address this

[rigelrozanski]

func InitializeNodeValidatorFiles(config *cfg.Config) (
     nodeID string, valPubKey crypto.PubKey, err error) {

[rigelrozanski]

func WriteGenesisFile(genFile, chainID string, validators []types.GenesisValidator, 
      appState json.RawMessage) error {

[rigelrozanski]

mostly looks good - left single line comments above

[jaekwon]

There's no need to have 2 methods... lets just require time on WriteGenesisFile.

[jaekwon]

genAppStateFromConfig was always a misnomer... it also writes the genesis file. Then we write it again below.
Lets fix this by making genAppStateFromConfig stateless and not write the genesis file.

[jaekwon]

It's weird that we're using a "canonical" state here, and yet... we call genAppStateFromConfig 4 times.... and it's also weird that we "initGenFiles" which writes mostly files, then load them again...

I think we should be creating a single genesis doc once without writing, and calling WriteGenesisFile(genFile, chainID, nil, appState, genTime) 4 times instead. Basically, most of this logic doesn't belong in for-loop.

"collectGenFiles" is also a misnomer. While it's collecting gentxs, this function does not "collect" gen files.

Basically, we should not have 2 steps of "init" and "collect"... we should init the gendoc once and "write" 4 times.

[alexanderbez]

Yes @jaekwon. The original logic was not great to begin with. It initially was one giant function. I simply broke the steps out to auxiliary functions. I believe that was the right approach. However, I agree that we must clean this up drastically and I'll take care of that.

[jaekwon]

As mentioned below, there are several things weird here... (partially due to the weirdness of the original logic). We shouldn't be init'ing 4 times and then "collecting" (misnomer) 4 times.