feat: Add genesis support for new modules and group genesis #270

blushi · 2021-02-18T13:58:01Z

closes: #214

codecov · 2021-02-18T14:03:00Z

Codecov Report

Merging #270 (2f24c89) into master (4032eb7) will decrease coverage by 0.40%.
The diff coverage is 59.85%.

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   62.66%   62.25%   -0.41%     
==========================================
  Files          46       47       +1     
  Lines        2954     2994      +40     
==========================================
+ Hits         1851     1864      +13     
- Misses        892      911      +19     
- Partials      211      219       +8

types/module/server/module.go

aaronc · 2021-03-03T18:46:29Z

testutil/fixture.go

+	// InitGenesisHandler is a function to get a module InitGenesisHandler.
+	InitGenesisHandler(moduleName string) module.InitGenesisHandler
+
+	// ExportGenesisHandler is a function to get a module ExportGenesisHandler.
+	ExportGenesisHandler(moduleName string) module.ExportGenesisHandler
+
+	// Codec is the app ProtoCodec.
+	Codec() *codec.ProtoCodec


Why do we need all this here?

To use them in tests, see in x/group/server/testsuite/ but happy to discuss any other solutions that would look better to you.

How about we simply have InitGenesis() and ExportGenesis() methods? Rather than being able to look up the handler by module. Also consider we want to use these same Fixtures for Network based tests in the future (#246).

The thing is that we can register multiple modules as part of FixtureFactory Setup() which is why I added them as look up methods.

I don't think that approach will work using a Network fixture. Would InitGenesis and ExportGenesis also work for the tests?

When you say InitGenesis() and ExportGenesis() methods, could you elaborate on the params and return values?
Do you see them as being similar to the manager InitGenesis() and ExportGenesis(), ie they would actually execute init or export genesis (instead of simply returning a function)?

I think Aaron has a good point - hiding the app details, and only have InitGenesis to initialize genesis for all modules would simplify the usage.

I've updated it so we have InitGenesis and ExportGenesis methods instead.

aaronc · 2021-03-10T21:59:04Z

I'm planning to review this soon. In the meantime can anyone else review?

blushi · 2021-03-11T08:10:39Z

I'm planning to review this soon. In the meantime can anyone else review?

@AmauryM said he'll have a look.

amaury1093

LGTM overall. Would like to avoid panics, and after that for me we can merge.

I also added a bunch of questions, mostly me getting up to speed with the groups module.

x/group/server/testsuite/genesis.go

proto/regen/group/v1alpha1/genesis.proto

x/group/server/genesis.go

amaury1093 · 2021-03-12T12:54:27Z

x/group/server/server.go

@@ -165,4 +165,5 @@ func RegisterServices(configurator servermodule.Configurator, accountKeeper Acco
 	impl := newServer(configurator.ModuleKey(), configurator.Router(), accountKeeper, configurator.Marshaler())
 	group.RegisterMsgServer(configurator.MsgServer(), impl)
 	group.RegisterQueryServer(configurator.QueryServer(), impl)
+	configurator.RegisterGenesisHandlers(impl.InitGenesis, impl.ExportGenesis)


I'm not a big fan of these custom Register* functions.

In the SDK, there's AppModule interface, and it's clear what the module developer needs to implement.

With this impl, we would have RegisterGenesisHandlers, RegisterInvariants, RegisterBeginBlocker etc... right? I feel it's easier to forget one register, whereas in AppModule the skipped ones were clearly marked (with an empty implementation).

One idea could be:

type ModuleConfig interface { RegisterMsgServer() RegisterQueryServer() InitGenesis() ExportGenesis() BeginBlocker() }

and then var _ ModuleConfig = impl{}

good point, I'll try to improve that, thanks!

Though we can't really add RegisterMsgServer and RegisterQueryServer as part of the interface, these are proto-generated standalone functions, serverImpl rather implements MsgServer and QueryServer interfaces, which are module specific. So I'd prefer to have separate interfaces for different concerns, and create a separate interface for genesis which would look like this (not really sure about the naming here):

type GenesisServer interface { InitGenesis() ... ExportGenesis() ... }

Or we could also have a single interface as you suggested such as:

type Server interface { InitGenesis() ExportGenesis() RegisterInvariants() ... }

And then we could also have one single entry method for the configurator to register genesis handlers, invariants, etc.

func (c *configurator) RegisterServer(s Server) { c.initGenesisHandler = s.InitGenesis c.exportGenesisHandler = s.ExportGenesis c.registerInvariantsHandler = s.RegisterInvariants ... }

But maybe that can be done as part of a separate PR, once we have more functions to register, wdyt?

But maybe that can be done as part of a separate PR, once we have more functions to register, wdyt?

Yes, good idea. #301

Do we need this things at all in a configurator? In comment's above I'm trying to find out if we could reuse module.Manager.

see my previous comment #270:

This server manager is used for the new modules (ecocredit, data, group) which are not registered with the usual sdk manager (see module wiring in app.go and experimental_appconfig.go). These new modules don't implement AppModule and don't have a direct access to the server implementation which is private (contrary to sdk modules with a reference to a keeper). So we pass relevant methods (init/export genesis in this case) to the configurator on RegisterServices so they can be passed in turn to the server manager on RegisterModules, then the server manager can use them to perform init/export genesis when needed.

This is needed in this context:

regen-ledger/types/module/server/manager.go

Lines 106 to 108 in 789f363

serverMod.RegisterServices(cfg)

mm.initGenesisHandlers[name] = cfg.initGenesisHandler

mm.exportGenesisHandlers[name] = cfg.exportGenesisHandler

x/group/server/genesis.go

amaury1093

All my questions were addressed, thanks @blushi

app/app.go

app/export.go

app/stable_appconfig.go

testutil/testdata/model.go

robert-zaremba · 2021-03-19T20:08:36Z

types/module/server/manager.go

+	router                *router
+	requiredServices      map[reflect.Type]bool
+	initGenesisHandlers   map[string]module.InitGenesisHandler
+	exportGenesisHandlers map[string]module.ExportGenesisHandler


why do we need to put this here? Can't types/module.Manager be used? It has InitGenesis method.

the manager InitGenesis and ExportGenesis methods use those handlers.

robert-zaremba · 2021-03-19T20:11:14Z

types/module/server/manager.go

+	return abci.ResponseInitChain{
+		Validators: validatorUpdates,
+	}
+}


This is almost a copy of types/module.Manager.InitGenesis - why we do that? Why we can't reuse the modules.Manager?

This server manager is used for the new modules (ecocredit, data, group) which are not registered with the usual sdk manager (see module wiring in app.go and experimental_appconfig.go). These new modules don't implement AppModule and don't have a direct access to the server implementation which is private (contrary to sdk modules with a reference to a keeper). So we pass relevant methods (init/export genesis in this case) to the configurator on RegisterServices so they can be passed in turn to the server manager on RegisterModules, then the server manager can use them to perform init/export genesis when needed.

Could you add a doc string explaining that (to the Manager.Init/Export Genesis

Also, this mechanism looks tricky. Instead of calling interface, we pass a manager and ask the module to register itself. I don't think that extending the Registration pattern is a good approach. Not going to block on that. Let me know wht do you think. If we find that it's good to rethink it then maybe we can continue in a new issue?
@aaronc - do you have any comments on that?

Maybe we should try something like I'm describing here which is a hybrid between the ADR 033 approach and the existing approach: #301 (comment).

Let's follow up on that as part of this issue #301

Yes, let's roll this PR and think about better approach in the issue.

robert-zaremba · 2021-03-19T21:20:39Z

x/group/server/server.go

@@ -165,4 +165,5 @@ func RegisterServices(configurator servermodule.Configurator, accountKeeper Acco
 	impl := newServer(configurator.ModuleKey(), configurator.Router(), accountKeeper, configurator.Marshaler())
 	group.RegisterMsgServer(configurator.MsgServer(), impl)
 	group.RegisterQueryServer(configurator.QueryServer(), impl)
+	configurator.RegisterGenesisHandlers(impl.InitGenesis, impl.ExportGenesis)


Do we need this things at all in a configurator? In comment's above I'm trying to find out if we could reuse module.Manager.

x/group/server/testsuite/genesis.go

…gen-ledger into marie/214-group-genesis

robert-zaremba

Let's think what to do with types/module.Manager vs initGenesisHandlers and exportGenesisHandlers
Not going to block on it, but let's see if this is a right approach we want to use towards SDKv1 before merging. (we can expand on this in an issue).

app/stable_appconfig.go

robert-zaremba · 2021-03-25T14:03:45Z

orm/genesis.go

 }

-// ImportTableData initializes a table and attached indexers from the given json encoded `[]Model`s.
+// ImportTableData initializes a table and attaches indexers from the given data interface{}.


I think copying indexes (first approach) is not bad. It won't take much more memory since we are only copying pointers.

If you like we can do it (or consider 2nd approach or even something else) later in a new issue?

robert-zaremba · 2021-03-25T14:09:40Z

types/module/server/manager.go

+	return abci.ResponseInitChain{
+		Validators: validatorUpdates,
+	}
+}


Could you add a doc string explaining that (to the Manager.Init/Export Genesis

robert-zaremba · 2021-03-25T14:13:46Z

types/module/server/manager.go

+	return abci.ResponseInitChain{
+		Validators: validatorUpdates,
+	}
+}


Also, this mechanism looks tricky. Instead of calling interface, we pass a manager and ask the module to register itself. I don't think that extending the Registration pattern is a good approach. Not going to block on that. Let me know wht do you think. If we find that it's good to rethink it then maybe we can continue in a new issue?
@aaronc - do you have any comments on that?

robert-zaremba

Please recheck if we have documentation for attributes updated.

blushi added 2 commits February 18, 2021 14:56

Add basic support for genesis with new modules wip

1186a40

cleanup

e9545d5

blushi added 9 commits February 18, 2021 16:53

Add genesis proto definition

62a608e

Update manager

9d8c24f

Wire up in/export genesis at app level

f5235e3

Add init/export group genesis

2a57c87

Fix seqs

51d4e8f

Merge branch 'master' into marie/214-group-genesis

16b28c4

Fix typo

fdc91ae

Use testdata with auto uint64 table in orm test

28331ee

Lint

0b3edc3

blushi marked this pull request as ready for review March 3, 2021 13:57

blushi requested review from aaronc, likhita-809, clevinson, aleem1314, amaury1093, anilcse, atheeshp and robert-zaremba March 3, 2021 13:57

aaronc reviewed Mar 3, 2021

View reviewed changes

types/module/server/module.go Outdated Show resolved Hide resolved

aaronc reviewed Mar 3, 2021

View reviewed changes

Rename cfg RegisterGenesisHandlers

0e2168b

blushi mentioned this pull request Mar 10, 2021

feat: add x/group simulations #251

Merged

Merge branch 'master' into marie/214-group-genesis

d8e71e3

amaury1093 reviewed Mar 12, 2021

View reviewed changes

Only keep relevant code in NotPanics

cbd6d5b

Clean up

ba57e0c

clevinson assigned robert-zaremba Mar 17, 2021

Return error as part of init/export genesis handlers

5a07f0e

amaury1093 mentioned this pull request Mar 18, 2021

Rethink configurator registration #301

Closed

amaury1093 approved these changes Mar 18, 2021

View reviewed changes

Merge branch 'master' into marie/214-group-genesis

235e8f6

robert-zaremba reviewed Mar 19, 2021

View reviewed changes

blushi added 10 commits March 24, 2021 12:31

Add doc about sequence

e53314e

Merge branch 'marie/214-group-genesis' of github.com:regen-network/re…

672938a

…gen-ledger into marie/214-group-genesis

Address some review comments

59c54ea

Lint

8ba8fc9

Improve error msg and doc

6f13740

Group account renaming

36d401f

Use require var in genesis tests

c0c7aae

Lint

858bc24

Use primary key instead of natural key

789f363

Use Init/ExportGenesis in Fixture

2b36281

blushi changed the title ~~Add genesis support for new modules and group genesis~~ feat: Add genesis support for new modules and group genesis Mar 25, 2021

robert-zaremba reviewed Mar 25, 2021

View reviewed changes

Add docs to setCustomModules

821d7c2

blushi mentioned this pull request Mar 25, 2021

Rethink ORM ImportTableData function #308

Closed

robert-zaremba mentioned this pull request Mar 25, 2021

Avoid using reflection in orm.ImportTableData #309

Closed

robert-zaremba approved these changes Mar 25, 2021

View reviewed changes

blushi added 3 commits March 25, 2021 16:56

Use address for group account address on proposal

1551d00

Merge branch 'master' into marie/214-group-genesis

a3a3666

Merge branch 'master' into marie/214-group-genesis

2f24c89

blushi merged commit a38ebdb into master Mar 30, 2021

blushi deleted the marie/214-group-genesis branch March 30, 2021 08:39

	serverMod.RegisterServices(cfg)
	mm.initGenesisHandlers[name] = cfg.initGenesisHandler
	mm.exportGenesisHandlers[name] = cfg.exportGenesisHandler

feat: Add genesis support for new modules and group genesis #270

feat: Add genesis support for new modules and group genesis #270

Conversation

blushi commented Feb 18, 2021 • edited by zube bot Loading

codecov bot commented Feb 18, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Mar 10, 2021

blushi commented Mar 11, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blushi Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

blushi Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blushi Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

amaury1093 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

robert-zaremba 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

robert-zaremba left a comment

Choose a reason for hiding this comment

blushi commented Feb 18, 2021 •

edited by zube bot

Loading

codecov bot commented Feb 18, 2021 •

edited

Loading

blushi Mar 18, 2021 •

edited

Loading

blushi Mar 18, 2021 •

edited

Loading

blushi Mar 25, 2021 •

edited

Loading