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: Fix LCD Validator Initialization #2452

Merged
merged 5 commits into from
Oct 9, 2018
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Oct 7, 2018

  • Update genesis app state initialization logic to allow for properly creating more than one validator during LCD tests.
  • Updated existing TestBonding to create two validators instead of one to provide an example.

Throughout this process, I've realized that the existing genesis logic is a bit convoluted, confusing and would warrant some rewriting.

closes: #2339


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • 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)

@codecov
Copy link

codecov bot commented Oct 8, 2018

Codecov Report

Merging #2452 into develop will increase coverage by 0.09%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2452      +/-   ##
===========================================
+ Coverage    59.69%   59.78%   +0.09%     
===========================================
  Files          136      136              
  Lines         8405     8405              
===========================================
+ Hits          5017     5025       +8     
+ Misses        3055     3048       -7     
+ Partials       333      332       -1

@alexanderbez alexanderbez changed the title WIP: Fix LCD Validator Initialization R4R: Fix LCD Validator Initialization Oct 8, 2018
@fedekunze
Copy link
Collaborator

@alexanderbez does it also work with more than 2 validators ?

cmd/gaia/app/test_utils.go Show resolved Hide resolved
cmd/gaia/app/test_utils.go Show resolved Hide resolved
cmd/gaia/app/test_utils.go Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

@fedekunze yes, this works with any # of validators so long as the proposing validator (in process val) has > 2/3 power which should always be the case for these simple tests.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK. cc: @cwgoes let's merge this to unblock the redelegation tests

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK

@cwgoes cwgoes merged commit e251088 into develop Oct 9, 2018
@cwgoes cwgoes deleted the bez/2339-fix-lcd-val-init branch October 9, 2018 18:10
@rigelrozanski
Copy link
Contributor

:( I was just reviewing this when it was merged: Generally disagree with the design approach used here through the creation of NewTestGaiaAppGenState - All genesis logic should be moved/merged with staking genesis logic that already exists (probably in x/stake/genesis.go) - by introducing this we're further fragmenting genesis staking logic which introduced downstream headaches whenever any little thing changes. - So yeah, let's reuse the existing genesis staking logic from x/stake/genesis.go within NewTestGaiaAppGenState instead of duplicating code. - We may need to updated the staking genesis logic/add new helper functions to accommodate for what's needed in this test, and that's fine, so long as it lives in staking

@rigelrozanski
Copy link
Contributor

I guess defragmentation can wait further to be addressed as a part of #2449

@alexanderbez
Copy link
Contributor Author

I don't see how what is implemented is further fragmenting what already exists. What is implemented is just a more flexible approach to what is already being done.

Granted, yes, the stake data initialization is genesis should live in the staking module.

@rigelrozanski
Copy link
Contributor

@alexanderbez yeah by fragment I just mean stuff ain't in the stake module that should be, the logic looked okay from what I saw, although there are other genesis procedures already existing in staking, and I'd liked if they were merged into a single process flow which is capable of genesis initialization for both LCD tests and Gaia

@alexanderbez
Copy link
Contributor Author

ACKed -- this made things confusing for me, so I definitely agree here.

@cosmos cosmos deleted a comment from alexanderbez Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InitializeTestLCD timeouts when nValidators > 1
4 participants