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

Sum validator operator's all voting power #5107

Merged
merged 13 commits into from
Oct 18, 2019
Merged

Conversation

Ruihuan
Copy link
Contributor

@Ruihuan Ruihuan commented Sep 26, 2019

If an address is a validator operator and it's delegated to another validator. When the address vote for a proposal, it just sums the voting power about its own validator. Here should sums its voting power under other validators that delegated to.

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.

I believe the changes to the tally logic are correct. I added a comment and I would also request to add a changelog entry. @sunnya97 does this make sense?

x/gov/keeper/tally.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d7a08ce). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #5107   +/-   ##
=========================================
  Coverage          ?   53.35%           
=========================================
  Files             ?      290           
  Lines             ?    17696           
  Branches          ?        0           
=========================================
  Hits              ?     9441           
  Misses            ?     7518           
  Partials          ?      737

@alexanderbez
Copy link
Contributor

alexanderbez commented Oct 12, 2019

bump @Ruihuan on a changelog entry :)

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 🎉

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.

After reviewing with @rigelrozanski, I believe we need to handle a special case for self-delegation -- i..e we don't want double accounting.

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.

Thanks! there is one issue which needs to be addressed per my review comments.

x/gov/keeper/tally.go Show resolved Hide resolved
x/gov/keeper/tally.go 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.

ACK, but we should probably have a test case that checks the self-delegation case.

x/gov/keeper/tally.go Outdated Show resolved Hide resolved
@sunnya97
Copy link
Member

Good catch on the logic bug. Thanks!

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
Copy link
Contributor

Mind resolving changelog conflicts @Ruihuan and then we'll get this merged :)

@alexanderbez alexanderbez dismissed sunnya97’s stale review October 18, 2019 16:30

Sunny approved given the changes requested were made which the were.

@alexanderbez alexanderbez merged commit 0ea8c32 into cosmos:master Oct 18, 2019
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.

4 participants