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

Implement InitGenesis for staking #772

Merged
merged 3 commits into from
Apr 3, 2018
Merged

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Apr 2, 2018

Ref #737
Awaiting rebase of rigel/tick-tests onto develop
Still need to write initial genesis info on basecoind init / democoind init

@cwgoes cwgoes requested a review from ebuchman as a code owner April 2, 2018 13:50
@cwgoes cwgoes requested review from rigelrozanski and removed request for ebuchman April 2, 2018 13:51
@codecov
Copy link

codecov bot commented Apr 2, 2018

Codecov Report

Merging #772 into develop will decrease coverage by 0.09%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           develop     #772     +/-   ##
==========================================
- Coverage    63.28%   63.19%   -0.1%     
==========================================
  Files           62       62             
  Lines         3315     3323      +8     
==========================================
+ Hits          2098     2100      +2     
- Misses        1069     1072      +3     
- Partials       148      151      +3

@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 2, 2018

Also ref #724

Awaiting consensus on InitGenesis spec

@rigelrozanski
Copy link
Contributor

just rebased rigel/tick-tests to develop

@cwgoes cwgoes force-pushed the cwgoes/staking-initgenesis branch from 382aa7c to 8fad09a Compare April 3, 2018 10:58
@cwgoes
Copy link
Contributor Author

cwgoes commented Apr 3, 2018

Rebased. Awaiting finalization of InitGenesis spec.

@rigelrozanski rigelrozanski changed the base branch from rigel/tick-tests to develop April 3, 2018 17:53
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

lookin' good let's just refine a bit more before merge

@@ -343,8 +356,7 @@ func (k Keeper) GetParams(ctx sdk.Context) (params Params) {
store := ctx.KVStore(k.storeKey)
b := store.Get(ParamKey)
if b == nil {
k.params = defaultParams()
return k.params
panic(errors.New("Stored params should not have been nil"))
Copy link
Contributor

Choose a reason for hiding this comment

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

do not need to use errors.New here (https://play.golang.org/p/skq8dkMt8fd)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@@ -374,7 +386,7 @@ func (k Keeper) GetPool(ctx sdk.Context) (gs Pool) {
store := ctx.KVStore(k.storeKey)
b := store.Get(PoolKey)
if b == nil {
return initialPool()
panic(errors.New("Stored pool should not have been nil"))
Copy link
Contributor

Choose a reason for hiding this comment

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

or here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -28,6 +30,17 @@ func NewKeeper(ctx sdk.Context, cdc *wire.Codec, key sdk.StoreKey, ck bank.CoinK
return keeper
}

// InitGenesis - store genesis parameters
func (k Keeper) InitGenesis(ctx sdk.Context, data json.RawMessage) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test function for InitGenesis (which can take a json.RawMessage defined in test taken from a hardcoded string)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good start to have

	encoded, err := json.Marshal(GenesisState{initialPool(), defaultParams()})
	if err != nil {
		panic(err)
	}
	if err = keeper.InitGenesis(ctx, encoded); err != nil {
		panic(err)
	}

But I'd like to see the test with the raw string information (as in the genesis.json file) being turned into the json raw message... this will give us an idea as to what are genesis file will actually look like here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, added test with raw string.

}
if err = keeper.InitGenesis(ctx, encoded); err != nil {
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I notice here that the InitGenesis functionality is almost "tested" here... let's move this to a separate explicit test for InitGenesis (aka TestInitGenesis) in keeper_test.go and replace lines 152-158 with:

k.setPool(ctx, state.Pool)
k.setParams(ctx, state.Params)

The function createTestInput is not really supposed to a test itself just a helper for other actual tests hence it's not in a _test.go file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, moved to TestInitGenesis.

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Looks good, did a minor cleanup on TestInitGenesis

@rigelrozanski rigelrozanski merged commit 9fc9db0 into develop Apr 3, 2018
@rigelrozanski rigelrozanski deleted the cwgoes/staking-initgenesis branch April 3, 2018 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants