Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

R4R Module/Genesis Generalization #4159

Merged
merged 120 commits into from
May 16, 2019
Merged

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Apr 19, 2019

closes #3006
REF affects #3976

Previously reviewed (now closed) PRs on this code: #4033 #4128 #4208
Followup PR: #4296
Followup issues: #4297 #4298 #4299


A good place to start reviewing is from the high level pattern structs defined and described in:

/*
This file contains application module patterns and associated "manager" functionality.
The module pattern has been broken down by:
- independent module functionality (AppModuleBasic)
- inter-dependent module functionality (AppModule)
inter-dependent module functionality is module functionality which somehow
depends on other modules, typically through the module keeper. Many of the
module keepers are dependent on each other, thus in order to access the full
set of module functionality we need to define all the keepers/params-store/keys
etc. This full set of advanced functionality is defined by the AppModule interface.
Independent module functions are separated to allow for the construction of the
basic application structures required early on in the application definition
and used to enable the definition of full module functionality later in the
process. This separation is necessary, however we still want to allow for a
high level pattern for modules to follow - for instance, such that we don't
have to manually register all of the codecs for all the modules. This basic
procedure as well as other basic patterns are handled through the use of
ModuleBasicManager.
*/

from here it's important to see how this implementation drastically simplifies cmd/gaia/app/ as well as gaiad (client simplification on the way see followup PR: #4296 - the followup shouldn't block this PR p.s.)

Initialization logic previously located in app/init as well as other genesis code scallywaggered into cmd/gaia/app/genesis.go has been refactored into two new modules which fit the high level module pattern and thus are initialized like modules:

  • x/genaccounts
  • x/genutil
    • all logic associated with creating/collecting/processing genesis transactions
    • server command functionality
    • testnet setup functionality

Also worth mentioning that nearly all of the existing testing has been preserved however due to some of the jimmyshimming of original test locations, they have now been moved out from gaia and into the appropriate modules test files (in x/genutil or x/genaccounts).

There is a lot of refactoring and improvements to code-organization in this PR - please feel free to ask all dumb questions as to why specific things have been moved - there is a justification for every little move in here.


  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]
  • 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)

@rigelrozanski rigelrozanski force-pushed the rigel/genesis-generalization3 branch from d67d665 to a45a6f3 Compare May 13, 2019 23:33
@cwgoes
Copy link
Contributor

cwgoes commented May 14, 2019

ACK modulo ensuring any API-breaking changes are included before a release is cut

@alessio alessio requested a review from alexanderbez May 14, 2019 17:18
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed this again. Generally happy with the approaches taken thus far. Although I still cannot settle with the fact that x/genutils and x/genaccounts are primary modules (@cwgoes voiced valid concerns here). I really do think this warrants further discussion. Namely, the following:

  • x/genutils should really be renamed to x/gentx
    • x/genutils has a hodgepodge of commands in x/genutils/client. I recommend only keeping the gentx related commands and moving the rest to commands under gaia/cmd/*.
  • Both thex/genutils (x/gentx) and x/genaccounts modules should really be sub-modules/packages under x/auth. Vesting accounts doesn't really change this because that will eventually be abstracted away (somehow).

I'll be more than happy to make a PR targeted against this PR with the changes I recommended (given that they're agreed upon).

Regardless of my objections here, I don't think this should be merged prior to @jaekwon taking at least a glimpse at this PR.

@rigelrozanski
Copy link
Contributor Author

okay so, I disagree with are moving some of the “util” commands back into gaia (namely, ValidateGenesis, and Init should be common importable code by other modules - however I think it’s fine is testnet live in gaia). So I suppose we could further separate out genutil into gentx and genutil by this logic + move testnet stuff back into gaia.

RE: genXxx being primary modules, we've made them into genesis-modules via the introduction of AppModuleGenesis which limit's their required input and hence I would not consider them "primary" modules. However, I think part of the hangup here is that your thinking along the lines that everything in x/ ought to be a full module... this is really not the case, consider mock, simulation, and params which are really not full modules either. Currently x/ is more like miscellaneous optional packages which are interchangeable and not required for the development of your own blockchain.

Also I do not believe that we should be using sub-modules. genutil and genaccounts should both be further abstracted to remove the auth dependancies and maintain ultimately independence from auth. Not that is matters but this mirrors Jae's perspective in the sense he's expressed we should not be using sub-modules, and it's fine to keep some interdependencies between modules.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 🎉

@rigelrozanski rigelrozanski force-pushed the rigel/genesis-generalization3 branch from b93f077 to 2bc901b Compare May 15, 2019 18:51
@rigelrozanski rigelrozanski force-pushed the rigel/genesis-generalization3 branch from 2ba6c00 to dc320a3 Compare May 15, 2019 19:30
@rigelrozanski rigelrozanski force-pushed the rigel/genesis-generalization3 branch from 550ebdd to e7f9c5a Compare May 15, 2019 20:44
@rigelrozanski rigelrozanski merged commit 3fe5869 into master May 16, 2019
@odeke-em odeke-em deleted the rigel/genesis-generalization3 branch December 10, 2020 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:genesis relating to chain genesis T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move generalizable genesis logic out of gaia
7 participants