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 Add security contact to Validator description #4865

Merged
merged 10 commits into from
Aug 12, 2019

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Aug 7, 2019

Closes #4814

  • 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 a relevant changelog entry: clog add [section] [-t <tag>] [-m <msg>]

  • Re-reviewed 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 changed the title R4R Add security contact to Validator description WIP Add security contact to Validator description Aug 7, 2019
@rigelrozanski rigelrozanski changed the title WIP Add security contact to Validator description R4R Add security contact to Validator description Aug 8, 2019
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.

Pending swagger.yaml update. Also imo we should be more specific on which type of contact (email-phone number, etc). I have a preference for the first. Thoughts ?

x/staking/types/validator.go Outdated Show resolved Hide resolved
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.

LGTM -- one minor nitpick. Also, don't we have to modify any of the REST docs/types?

x/staking/types/validator.go Outdated Show resolved Hide resolved
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #4865 into master will decrease coverage by 0.02%.
The diff coverage is 56.66%.

@@            Coverage Diff             @@
##           master    #4865      +/-   ##
==========================================
- Coverage   54.05%   54.03%   -0.03%     
==========================================
  Files         269      269              
  Lines       17116    17130      +14     
==========================================
+ Hits         9252     9256       +4     
- Misses       7178     7186       +8     
- Partials      686      688       +2

@fedekunze
Copy link
Collaborator

do we also need to add the genesis migration file?

@rigelrozanski
Copy link
Contributor Author

@fedekunze not sure, but I don't think we need to specify between email and phone... I doubt anyone will put their phone.

x/staking/legacy/v0_37/migrate.go Outdated Show resolved Hide resolved
@rigelrozanski
Copy link
Contributor Author

Olright I added some migration details to the staking package however I haven't included a migration command upgrade to the genutil CLI command yet - that should occur in a separate PR, I was going to do it here but I realized I think there may be an abstraction that we could implement to avoid dup. code in genutil/legacy/XXX/. (new issue here: #4888)

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 ⚡️

@alexanderbez alexanderbez merged commit 771f8a0 into master Aug 12, 2019
@alexanderbez alexanderbez deleted the rigel/4814-validators-email branch August 12, 2019 13:10
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
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.

Security Contact Email to Validator Descrition
3 participants