Initialize Genesis State #416

rigelrozanski · 2018-02-03T01:11:50Z

closes #339
This PR includes some cleanup to make go-lint compliant

codecov · 2018-02-05T01:46:47Z

Codecov Report

Merging #416 into develop will increase coverage by 0.05%.
The diff coverage is 58.2%.

@@             Coverage Diff             @@
##           develop     #416      +/-   ##
===========================================
+ Coverage    52.13%   52.18%   +0.05%     
===========================================
  Files           34       35       +1     
  Lines         1619     1671      +52     
===========================================
+ Hits           844      872      +28     
- Misses         721      742      +21     
- Partials        54       57       +3

ebuchman · 2018-02-08T18:58:26Z

baseapp/baseapp.go

@@ -32,33 +32,37 @@ type BaseApp struct {
 	// Main (uncached) state
 	cms sdk.CommitMultiStore

-	// Unmarshal []byte into sdk.Tx
+	// unmarshal []byte into sdk.Tx


Let's try to keep comments well formed (capitalized and with periods). https://github.com/tendermint/coding/blob/master/go/coding_standard.md#various

Did go-lint really complain about this ?

no, there are not sentences, as in the spec, only sentences should start capitalized. Everything else should start lowercase and end without a period... We should further clarify this in the spec

ebuchman · 2018-02-08T19:19:06Z

baseapp/baseapp.go

@@ -174,7 +183,7 @@ func (app *BaseApp) initFromStore(mainKey sdk.StoreKey) error {

 //----------------------------------------

-// Implements ABCI.
+// Implements ABCI


Why get rid of periods? It's also in the coding conventions

See my previous comment (not a sentence)

ebuchman

Thanks Rige. I'll try to wrap this up tonight

ebuchman · 2018-02-13T02:01:26Z

baseapp/genesis.go

+// TODO this is dup code from tendermint-core to avoid dep issues
+// should probably remove from both SDK / Tendermint and move to tmlibs
+//  ^^^^^^^^^^^^^ This is my preference to avoid DEP hell
+// or sync up versioning and just reference tendermint


lol our deps are in a brutal state :(

we need to make sure it never happens again. obscene.

ebuchman · 2018-02-13T02:04:08Z

examples/basecoin/app/app.go

+	}
+	app.BaseApp.BeginBlock(abci.RequestBeginBlock{
+		Hash:                nil,
+		Header:              header,


why do we need this ?!

to set up the multistore cache wrap for deliver store used in NewContext, I just made this code considerably smaller

ebuchman · 2018-02-13T02:05:25Z

examples/basecoin/app/init_baseapp.go

@@ -26,3 +33,68 @@ func (app *BasecoinApp) initBaseAppTxDecoder() {
 		return tx, nil
 	})
 }
+
+// We use GenesisAccount instead of types.AppAccount for cleaner json input of PubKey


is this because we don't have go-wire JSON yet ?

no, json representation of the new PubKey is a byte array... I'd obviously rather not use an input struct, if you have any other ideas lmn

Oh, we don't even need PubKeys for accounts in the genesis file!

ebuchman · 2018-02-13T02:07:29Z

examples/basecoin/app/init_baseapp.go

+			return nil
+		}
+
+		var gaccs []*GenesisAccount


we should more clearly state this expectation somewhere - probably define an AppGenesis type that is a []*GenesisAccount ... or probably better to use a struct containing a []*GenesisAccount.

ie. we want to make clear somewhere outside this function what the json in the genesis should look like

good point, an alternative design should actually be used for custom genesis types which defines its own unmarshalling. aka AppAccount should actually exist as a custom module within basecoin which defines genesis unmarshalling... right now everything is just kind of unmarshalled here, I'm going to integrate this change in with the basecoin refactor

ebuchman · 2018-02-13T02:07:51Z

examples/basecoin/cmd/basecoind/main.go


 func main() {
-	fmt.Println("TODO: move examples/basecoin/main.go here and refactor")
+	// TODO CREATE CLI


ebuchman · 2018-02-13T02:09:37Z

types/errors.go

-	CodeUnknownRequest      CodeType = 6
-	CodeUnrecognizedAddress CodeType = 7
-	CodeInvalidSequence     CodeType = 8
+	CodeGenesisParse        CodeType = 3


weird seeing this in here. InitChain doesn't even return a code - do we need this ?

SetInitStater can return error when initializing genesis - what error code would you use otherwise? CodeInternal?

* Alter identifier requirements * Rebuild PDF

* Fix merge conflict * Run go mod tidy

rigelrozanski requested a review from jaekwon as a code owner February 3, 2018 01:11

rigelrozanski force-pushed the feature/initgen branch from 9db8b2f to 6c2ef50 Compare February 4, 2018 20:56

rigelrozanski changed the base branch from develop to feature/remove_tmc_dep February 4, 2018 20:57

rigelrozanski changed the base branch from feature/remove_tmc_dep to develop February 7, 2018 13:33

rigelrozanski added 5 commits February 8, 2018 10:36

init genesis WIP, also making golint compliant

40fd458

working

41ae60c

more comments cleanup

da538a8

genesis wip compiles

7643dea

wip genesis parsing

6eaafa4

rigelrozanski force-pushed the feature/initgen branch from f72a3fb to 6eaafa4 Compare February 8, 2018 10:59

ebuchman reviewed Feb 8, 2018

View reviewed changes

rigelrozanski and others added 7 commits February 8, 2018 21:16

working

849139e

working

17acf9e

genesis exisiting tests pass

7206c43

working genesis tests

2b9633d

...

fd1684a

Fix init state bug

b09653c

initgen tests complete

0bab936

rigelrozanski changed the title ~~WIP: Initialize Genesis State~~ Initialize Genesis State Feb 12, 2018

remove genesis of checkTx

ed66256

cosmos deleted a comment from rigelrozanski Feb 13, 2018

ebuchman reviewed Feb 13, 2018

View reviewed changes

cleanup

658d763

ebuchman approved these changes Feb 13, 2018

View reviewed changes

fixes from review

d9ebe34

ebuchman merged commit 69c316f into develop Feb 13, 2018

ebuchman deleted the feature/initgen branch February 13, 2018 12:38

ParthDesai pushed a commit to ChorusOne/cosmos-sdk that referenced this pull request Apr 19, 2021

Merge PR cosmos#416: Alter identifier requirements

dc4ecb8

* Alter identifier requirements * Rebuild PDF

patiee pushed a commit to patiee/cosmos-sdk that referenced this pull request Mar 23, 2023

feat: Backport: (feat: Add grants by grantee authz query) (cosmos#416)

0907b75

* Fix merge conflict * Run go mod tidy

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize Genesis State #416

Initialize Genesis State #416

rigelrozanski commented Feb 3, 2018

codecov bot commented Feb 5, 2018 •

edited

Loading

ebuchman Feb 8, 2018

ebuchman Feb 8, 2018

rigelrozanski Feb 8, 2018

ebuchman Feb 8, 2018

rigelrozanski Feb 8, 2018

ebuchman left a comment

ebuchman Feb 13, 2018

ebuchman Feb 13, 2018

rigelrozanski Feb 13, 2018

ebuchman Feb 13, 2018

rigelrozanski Feb 13, 2018

ebuchman Feb 13, 2018

ebuchman Feb 13, 2018

rigelrozanski Feb 13, 2018

ebuchman Feb 13, 2018

ebuchman Feb 13, 2018

rigelrozanski Feb 13, 2018

Initialize Genesis State #416

Initialize Genesis State #416

Conversation

rigelrozanski commented Feb 3, 2018

codecov bot commented Feb 5, 2018 • edited Loading

Codecov Report

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

ebuchman left a 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

codecov bot commented Feb 5, 2018 •

edited

Loading