Per-module genesis initialization #714

cwgoes · 2018-03-27T18:06:12Z

Work in progress, more tests to come.

rigelrozanski · 2018-03-27T18:08:17Z

types/initgenesis.go

+
+/* Run only once on chain initialization, should write genesis state to store
+   or throw an error if some required information was not provided, in which case
+   the application will panic. */


// comment
// like
// this

codecov · 2018-03-27T18:11:10Z

Codecov Report

Merging #714 into develop will decrease coverage by 0.23%.
The diff coverage is 54.16%.

@@            Coverage Diff             @@
##           develop    #714      +/-   ##
==========================================
- Coverage    64.04%   63.8%   -0.24%     
==========================================
  Files           43      43              
  Lines         2022    2039      +17     
==========================================
+ Hits          1295    1301       +6     
- Misses         615     623       +8     
- Partials       112     115       +3

rigelrozanski · 2018-03-27T18:13:45Z

client/lcd/lcd_test.go

 			{
 				Name:    "tester",
 				Address: pubKey.Address(),
 				Coins:   coins,
 			},
 		},
+		"cool": map[string]string{
+			"trend": "ice-cold",
+		},


lcd_test should have nothing to do cool module

Agree but must be included for this test to pass until that test refactoring happens.

rigelrozanski · 2018-03-27T18:22:36Z

baseapp/baseapp.go

+			}
+		}
+		return nil
+	})


This is confusing... we should make a bit simpler - let's breakout this function variable definition to another line before line 244

rigelrozanski · 2018-03-27T18:22:54Z

baseapp/router.go

+	return nil
+}
+
+func (rtr *router) ForEach(f func(string, sdk.Handler, sdk.InitGenesis) error) error {


Need a comment, enable go-metalinter

we should make func(string, sdk.Handler, sdk.InitGenesis) error a type

Maybe a different name than ForEach - or better explanation

Instead of ForEach let's create something called InitModules which performs the loop over all the modules.... if this is possible

rigelrozanski · 2018-03-27T18:23:16Z

baseapp/router.go

 			return route.h
 		}
 	}
 	return nil
 }
+
+func (rtr *router) RouteGenesis(path string) (i sdk.InitGenesis) {


high level comment

rigelrozanski · 2018-03-27T18:25:02Z

examples/basecoin/app/app_test.go

 			types.NewGenesisAccount(acc),
 		},
+		"cool": map[string]string{
+			"trend": "ice-cold",
+		},


is there another way than using a map here? I know it's safe

maybe fine - what are our options

Other option - pass bytestring directly - means we'd have to use printf for variable parameters, though, seems ugly.

rigelrozanski · 2018-03-27T18:27:05Z

baseapp/baseapp_test.go

@@ -180,7 +180,7 @@ func TestInitChainer(t *testing.T) {

 	// set initChainer and try again - should see the value
 	app.SetInitChainer(initChainer)
-	app.InitChain(abci.RequestInitChain{})
+	app.InitChain(abci.RequestInitChain{AppStateBytes: []byte("{}")})


add comment explaining this change

rigelrozanski · 2018-03-27T18:28:34Z

baseapp/baseapp.go

+		// TODO Return something intelligent
+		panic(err)
+	}
+	err = app.Router().ForEach(func(r string, _ sdk.Handler, i sdk.InitGenesis) error {


should reference a type which is that func(r string, _ sdk.Handler, i sdk.InitGenesis) error

rigelrozanski · 2018-03-27T18:29:28Z

baseapp/baseapp.go

+			encoded, exists := (*genesisState)[r]
+			if !exists {
+				// TODO should this be a Cosmos SDK standard error?
+				return errors.New(fmt.Sprintf("Expected module genesis information for module %s but it was not present", r))


can use fmt.Errorf instead here

rigelrozanski · 2018-03-27T18:29:37Z

baseapp/baseapp.go

+		if i != nil {
+			encoded, exists := (*genesisState)[r]
+			if !exists {
+				// TODO should this be a Cosmos SDK standard error?


yes... let's do it!

rigelrozanski

hecka yeah

cwgoes · 2018-03-27T19:49:13Z

@rigelrozanski hecka refactored

Still need to rebase onto keeper-modules though.

cwgoes · 2018-03-27T21:22:56Z

Rebased onto #690

rigelrozanski · 2018-03-28T00:48:38Z

aight yo - merged in - rebase can commence!

cwgoes · 2018-03-28T09:31:20Z

Rerebased

All the test changes can be reverted when democoin is separated (#695 (comment))

rigelrozanski

great work!

ebuchman · 2018-03-31T14:48:56Z

baseapp/router.go

 	"regexp"

 	sdk "github.com/cosmos/cosmos-sdk/types"
 )

 // Router provides handlers for each transaction type.
 type Router interface {
-	AddRoute(r string, h sdk.Handler) (rtr Router)
+	AddRoute(r string, h sdk.Handler, i sdk.InitGenesis) (rtr Router)


I'm not sure about this.

What makes InitGenesis different from BeginBlock and EndBlock that it gets this special treatment ?

See also my thoughts in #559 (which I don't think Jae agrees with, but anyways).

In the absence of doing this, modules should export methods that can be called by the InitChain function that is stitched together by the application and passed to SetInitChain

The problem mostly lies routing the correct genesis option to the correct module. I think we want to enforce a basic structure as to how genesis information is passed from the genesis block directly to the module to handle. Right now we simply pass a json.rawmessage contained by a key with the module with the matching name, the module can parse / handle the raw message in arbitrary ways. I don't really see the advantage of customizable global application init genesis method... right now the only reason this is necessary is you are extending the standard account. Anyways let's talk more about this

What makes InitGenesis different from BeginBlock and EndBlock that it gets this special treatment ?

we need to route genesis information based on the key provided in genesis. BeginBlock/EndBlock do not need to route custom information - I also assume that BeginBlock/EndBlock handlers may have some interdependence whereas InitGenesis will not

further thoughts? - maybe we should break this out into a discussion issue

@rigelrozanski Discussion issue: #759

cwgoes requested a review from ebuchman as a code owner March 27, 2018 18:06

rigelrozanski reviewed Mar 27, 2018

View reviewed changes

rigelrozanski suggested changes Mar 27, 2018

View reviewed changes

cwgoes mentioned this pull request Mar 27, 2018

Generic per-module genesis store initialization #712

Closed

cwgoes force-pushed the cwgoes/per-module-genesis branch from 2f1407e to 5dc13d0 Compare March 27, 2018 21:21

cwgoes mentioned this pull request Mar 27, 2018

SDK Module Developer Perspective Questions #694

Closed

cwgoes force-pushed the cwgoes/per-module-genesis branch from 5dc13d0 to cd650b3 Compare March 28, 2018 09:22

cwgoes added 3 commits March 28, 2018 11:24

Initial per-module genesis initialization

5b64206

Address Rigel review - also run gofmt

578392d

Rebase onto #690 (tiny fix)

90304fc

cwgoes force-pushed the cwgoes/per-module-genesis branch from cd650b3 to 90304fc Compare March 28, 2018 09:25

rigelrozanski approved these changes Mar 28, 2018

View reviewed changes

rigelrozanski changed the title ~~WIP: Per-module genesis initialization~~ Per-module genesis initialization Mar 28, 2018

rigelrozanski merged commit 144ea05 into develop Mar 28, 2018

rigelrozanski deleted the cwgoes/per-module-genesis branch March 28, 2018 16:29

ebuchman reviewed Mar 31, 2018

View reviewed changes

ebuchman mentioned this pull request Mar 31, 2018

Per-Module ABCI msgs #759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per-module genesis initialization #714

Per-module genesis initialization #714

cwgoes commented Mar 27, 2018

rigelrozanski Mar 27, 2018

codecov bot commented Mar 27, 2018 •

edited

Loading

rigelrozanski Mar 27, 2018 •

edited

Loading

rigelrozanski Mar 27, 2018

cwgoes Mar 28, 2018

rigelrozanski Mar 27, 2018 •

edited

Loading

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

cwgoes Mar 28, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski Mar 27, 2018

rigelrozanski left a comment

cwgoes commented Mar 27, 2018 •

edited

Loading

cwgoes commented Mar 27, 2018

rigelrozanski commented Mar 28, 2018

cwgoes commented Mar 28, 2018

rigelrozanski left a comment

ebuchman Mar 31, 2018

rigelrozanski Apr 1, 2018

rigelrozanski Apr 2, 2018

cwgoes Apr 2, 2018

Per-module genesis initialization #714

Per-module genesis initialization #714

Conversation

cwgoes commented Mar 27, 2018

Choose a reason for hiding this comment

codecov bot commented Mar 27, 2018 • edited Loading

Codecov Report

rigelrozanski Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski Mar 27, 2018 • edited Loading

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

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

cwgoes commented Mar 27, 2018 • edited Loading

cwgoes commented Mar 27, 2018

rigelrozanski commented Mar 28, 2018

cwgoes commented Mar 28, 2018

rigelrozanski 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

codecov bot commented Mar 27, 2018 •

edited

Loading

rigelrozanski Mar 27, 2018 •

edited

Loading

rigelrozanski Mar 27, 2018 •

edited

Loading

cwgoes commented Mar 27, 2018 •

edited

Loading