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: Validator Commission Model #2365

Merged
merged 32 commits into from
Sep 24, 2018
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Sep 20, 2018

Implement validator commission model:

  • Validator structure updated to include Commission as a first class citizen type
  • Create and edit validator messages handlers updated accordingly.
  • Update create-validator and edit-validator CLI commands.
  • Updated staking tx docs

closes: #1672
ref: #2236


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

Commission sdk.Dec `json:"commission"` // the commission rate charged to delegators
CommissionMax sdk.Dec `json:"commission_max"` // maximum commission rate which validator can ever charge
CommissionMaxChangeRate sdk.Dec `json:"commission_max_change_rate"` // maximum daily increase of the validator commission
CommissionChangeToday sdk.Dec `json:"commission_change_today"` // total commission rate change today (reset each day)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this variable is necessary. You can just limit a validator to one change every 24 hours, and when they do change validate its within the max change rate. You don't need to store the last delta change.

Copy link
Contributor Author

@alexanderbez alexanderbez Sep 20, 2018

Choose a reason for hiding this comment

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

I was informed by @rigelrozanski that we would allow multiple changes a day as long as those changes are within the constraints of CommissionMaxChangeRate. Also, CommissionChangeToday is not new.

Are you suggesting we change and simplify the spec? This would things much easier to track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll go with this approach for now that you're suggesting @ValarDragon -- I'll amend if @rigelrozanski suggests otherwise.

Copy link
Contributor

@ValarDragon ValarDragon Sep 20, 2018

Choose a reason for hiding this comment

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

Awesome! Lets make a follow-up issue tagged postlaunch then

I have the PoV atm that lets table implementing convenience changes like that to postlaunch. (It is a good idea, but unnecessary atm)

Copy link
Contributor

@rigelrozanski rigelrozanski Sep 21, 2018

Choose a reason for hiding this comment

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

Great suggestion - One change per day sounds fine if it will simplify the codebase :) - however we will of course still need to keep track of when the last change was made to check against - good improvement idea val!

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

Merging #2365 into develop will increase coverage by 0.02%.
The diff coverage is 75%.

@@             Coverage Diff             @@
##           develop    #2365      +/-   ##
===========================================
+ Coverage    64.77%   64.79%   +0.02%     
===========================================
  Files          137      137              
  Lines         8469     8494      +25     
===========================================
+ Hits          5486     5504      +18     
- Misses        2620     2625       +5     
- Partials       363      365       +2

@alexanderbez alexanderbez changed the title WIP: Validator Commission Model R4R: Validator Commission Model Sep 21, 2018
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.

Good work! - few suggestions herein

x/stake/keeper/validator.go Outdated Show resolved Hide resolved
x/stake/keeper/validator_test.go Outdated Show resolved Hide resolved
x/stake/types/commission.go Outdated Show resolved Hide resolved
x/stake/types/validator_test.go Outdated Show resolved Hide resolved
x/stake/client/cli/utils.go Outdated Show resolved Hide resolved
x/gov/tally_test.go Outdated Show resolved Hide resolved
x/stake/client/cli/flags.go Show resolved Hide resolved
x/stake/client/cli/utils.go Outdated Show resolved Hide resolved
x/stake/client/cli/utils.go Outdated Show resolved Hide resolved
x/stake/keeper/validator.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

@ValarDragon and @rigelrozanski address your comments 👍

@rigelrozanski
Copy link
Contributor

dopeness

@rigelrozanski rigelrozanski merged commit 9dafa32 into develop Sep 24, 2018
@rigelrozanski rigelrozanski deleted the bez/1672-val-commission-model branch September 24, 2018 22:24
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.

Implement Validator Commission Model
3 participants