R4R: Additional gentx verfication #2971

jackzampolin · 2018-11-30T23:50:21Z

This PR checks that when running gaiad gentx that the --from account:

Exists in genesis
Contains enough funds of type stakeTypes.DefaultBondDenom to stake them

Should it additionally check that the denomination of the funds is the correct type?
Are there any additional checks we should add here?

cc @alexanderbez @cwgoes @alessio

Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
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)

…tx command

jackzampolin · 2018-11-30T23:51:38Z

CLI Tests failing are the result of this change. Still more work there and we should likely add tests for this functionality as well.

codecov · 2018-12-03T00:26:44Z

Codecov Report

Merging #2971 into develop will decrease coverage by 0.19%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           develop    #2971     +/-   ##
==========================================
- Coverage    55.72%   55.53%   -0.2%     
==========================================
  Files          120      120             
  Lines         8468     8494     +26     
==========================================
- Hits          4719     4717      -2     
- Misses        3427     3455     +28     
  Partials       322      322

't check error on key delete

jackzampolin · 2018-12-03T00:51:53Z

Ok so I have these tests passing locally with no problem, but the new functionality I added here (validation that the account you are gentxing exists in the genesis accounts array) is failing here. I've added some additional debugging to print the genesis before the failure and it shows that the account is in fact in the genesis file.

cmd/gaia/init/gentx.go

cwgoes · 2018-12-03T11:25:27Z

The CLI tests also fail for me locally @jackzampolin, with the same error as on CI. I wonder if you have a different genesis.json which is not being automatically initialized correctly in the tests?

alexanderbez

Left some minor feedback. Most importantly I think @cwgoes comments should be addressed and then it could be ready for merge.

cmd/gaia/cli_test/cli_test.go

cmd/gaia/init/gentx.go

Makefile

Co-Authored-By: jackzampolin <[email protected]>

cmd/gaia/init/gentx.go

alexanderbez

A few more tidbits ☕️

cmd/gaia/init/gentx.go

tests/util.go

alexanderbez

CI passes and changes look reasonable. Thanks @jackzampolin. This should warrant another review or two though.

rigelrozanski · 2018-12-03T23:06:05Z

cmd/gaia/init/gentx.go

+			bondDenom := genesisState.StakeData.Params.BondDenom
+
+			// Check if the account is in genesis
+			for _, acc := range genesisState.Accounts {


The contents of this for loop should probably be broken out into a seperate function, or this whole containing function restructured to reduce indentation

rigelrozanski · 2018-12-03T23:07:07Z

cmd/gaia/init/gentx.go

+				return err
+			}
+
+			inGenesis := false


use of inGenesis is confusing - I don't intuitively know what this parameter is doing

rigelrozanski · 2018-12-03T23:08:52Z

cmd/gaia/init/gentx.go

+			}
+
+			if !inGenesis {
+				return fmt.Errorf(


We can eliminate use of inGenesis by just moving this error into the above loop (which should also probably get moved to it's own funcion per my previous comment)

jackzampolin · 2018-12-04T02:30:15Z

Ok I've addressed all the PR comments and this should be good to go!

alessio

All previous comments were addressed, nothing to add from me. Furthermore, integration tests execution went down to 1m30s!!

* Add check that account is in genesis and contains enough funds to gentx command * Fix CLI tests and make them parallel * Update makefile to take advantage of parallel tests * Add path seperator back in * Don 't check error on key delete * Add debuggin printout for debugging remote test failures * Update cmd/gaia/init/gentx.go Co-Authored-By: jackzampolin <[email protected]> * Update cmd/gaia/init/gentx.go Co-Authored-By: jackzampolin <[email protected]> * Change to bondDenom from the stake section in genesis * Add PENDING.md * Push changes * Fix CI failure * Address PR comments * Fix broken gov tests * Address PR comments * Address PR comments

Add check that account is in genesis and contains enough funds to gen…

1a0bfd0

…tx command

jackzampolin requested review from cwgoes, ebuchman and rigelrozanski as code owners November 30, 2018 23:50

jackzampolin added 2 commits December 2, 2018 16:21

Fix CLI tests and make them parallel

fc54c14

Update makefile to take advantage of parallel tests

b351b55

jackzampolin added 3 commits December 2, 2018 16:30

Merge branch 'develop' into jack/check-gentx

c6c207c

Add path seperator back in

98322e0

Don

cc81f2d

't check error on key delete

jackzampolin changed the title ~~WIP: Additional gentx verfication~~ R4R: Additional gentx verfication Dec 3, 2018

Add debuggin printout for debugging remote test failures

e48bcaf

cwgoes mentioned this pull request Dec 3, 2018

Game of Stakes release blockers #2981

Closed

4 tasks

cwgoes suggested changes Dec 3, 2018

View reviewed changes

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

Merge branch 'develop' into jack/check-gentx

bd9bc6d

alexanderbez suggested changes Dec 3, 2018

View reviewed changes

cmd/gaia/cli_test/cli_test.go Show resolved Hide resolved

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

alessio reviewed Dec 3, 2018

View reviewed changes

Makefile Show resolved Hide resolved

alexanderbez and others added 6 commits December 3, 2018 08:07

Update cmd/gaia/init/gentx.go

ea73d0e

Co-Authored-By: jackzampolin <[email protected]>

Update cmd/gaia/init/gentx.go

a48a3a5

Co-Authored-By: jackzampolin <[email protected]>

Change to bondDenom from the stake section in genesis

058a687

Add PENDING.md

377724c

Push changes

bcc325a

Fix CI failure

96e3726

alexanderbez reviewed Dec 3, 2018

View reviewed changes

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

jackzampolin added 2 commits December 3, 2018 10:46

Address PR comments

4aea762

Fix broken gov tests

5e2bb24

alexanderbez suggested changes Dec 3, 2018

View reviewed changes

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved

tests/util.go Show resolved Hide resolved

Address PR comments

d0e97cd

alexanderbez approved these changes Dec 3, 2018

View reviewed changes

Merge branch 'develop' into jack/check-gentx

96ea990

rigelrozanski reviewed Dec 3, 2018

View reviewed changes

Address PR comments

661ff07

alessio approved these changes Dec 4, 2018

View reviewed changes

cwgoes approved these changes Dec 4, 2018

View reviewed changes

cwgoes merged commit d8fbae6 into develop Dec 4, 2018

cwgoes deleted the jack/check-gentx branch December 4, 2018 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Additional gentx verfication #2971

R4R: Additional gentx verfication #2971

jackzampolin commented Nov 30, 2018 •

edited

Loading

jackzampolin commented Nov 30, 2018

codecov bot commented Dec 3, 2018 •

edited

Loading

jackzampolin commented Dec 3, 2018

cwgoes commented Dec 3, 2018

alexanderbez left a comment

alexanderbez left a comment

alexanderbez left a comment

rigelrozanski Dec 3, 2018

rigelrozanski Dec 3, 2018

rigelrozanski Dec 3, 2018

jackzampolin commented Dec 4, 2018

alessio left a comment

R4R: Additional gentx verfication #2971

R4R: Additional gentx verfication #2971

Conversation

jackzampolin commented Nov 30, 2018 • edited Loading

jackzampolin commented Nov 30, 2018

codecov bot commented Dec 3, 2018 • edited Loading

Codecov Report

jackzampolin commented Dec 3, 2018

cwgoes commented Dec 3, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

rigelrozanski Dec 3, 2018

Choose a reason for hiding this comment

rigelrozanski Dec 3, 2018

Choose a reason for hiding this comment

rigelrozanski Dec 3, 2018

Choose a reason for hiding this comment

jackzampolin commented Dec 4, 2018

alessio left a comment

Choose a reason for hiding this comment

jackzampolin commented Nov 30, 2018 •

edited

Loading

codecov bot commented Dec 3, 2018 •

edited

Loading