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

Validator Creation Updates #3803

Merged
merged 8 commits into from
Mar 5, 2019
Merged

Validator Creation Updates #3803

merged 8 commits into from
Mar 5, 2019

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Mar 4, 2019

Update validator creation flow:

  • Remove NewMsgCreateValidatorOnBehalfOf and corresponding business logic
  • Ensure the validator address equals the delegator address during
    MsgCreateValidator#ValidateBasic

closes: #3789


  • 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 Mar 4, 2019

Codecov Report

Merging #3803 into develop will decrease coverage by 0.09%.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##           develop   #3803     +/-   ##
=========================================
- Coverage    60.99%   60.9%   -0.1%     
=========================================
  Files          191     191             
  Lines        14177   14172      -5     
=========================================
- Hits          8647    8631     -16     
- Misses        4976    4985      +9     
- Partials       554     556      +2

@alexanderbez alexanderbez added C:x/staking T: State Machine Breaking State machine breaking changes (impacts consensus). labels Mar 4, 2019
@alexanderbez alexanderbez marked this pull request as ready for review March 4, 2019 18:40
@jackzampolin jackzampolin added this to the v0.33.0 (Launch) milestone Mar 4, 2019
@jackzampolin
Copy link
Member

What about Ensure you cannot edit a validator if self-delegation is below minimum and testing? It looks like you have updated existing tests in this PR.

@alexanderbez
Copy link
Contributor Author

@jackzampolin let me check if there is a test that covers the case if a delegation is below the min self delegation 👍 (if not, I'll add one).

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.

Two tiny nits - otherwise ACK - but I don't think this closes #3789, it doesn't prevent editing a validator below minimum self-delegation (although I don't think we need to do that, but it's listed in the original issue).

docs/gaia/validators/validator-setup.md Outdated Show resolved Hide resolved
docs/gaia/validators/validator-setup.md Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

@jackzampolin there already is a test case in TestMsgCreateValidator for what you suggested. @cwgoes I intentionally left out the edit validator part. That needs more discussion.

@cwgoes
Copy link
Contributor

cwgoes commented Mar 5, 2019

Thanks @alexanderbez; my approval stands.

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

This gets a 👍 LGTM from me!

@cwgoes cwgoes merged commit 51c03aa into develop Mar 5, 2019
@cwgoes cwgoes deleted the bez/3789-val-creation-updates branch March 5, 2019 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/staking T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validator Creation Updates
3 participants