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

[Feature] minimum commission rate upgrade #47

Closed
wants to merge 12 commits into from

Conversation

yun-yeo
Copy link
Contributor

@yun-yeo yun-yeo commented May 31, 2022

Summary of changes

This PR is still under consensus and need governance proposal to be reflected on chain.

What're Changed

  • Add upgrade handler to make validators commission to bigger than minimum rate (= initially 5%)
  • Implement ante handler to check whether MsgCreateValidator and MsgEditValidator's commission rate is bigger than minimum rate (= initially 5%)
  • New parameter is introduced ante.MinimumCommissionEnforced.
  • New ante store value is introduced MinimumCommission.
  • New gRPC endpoint /terra/ante/v2/params introduced.
  • New gRPC endpoint /terra/ante/v2/minimum_commission introduced.
  • New cmd terrad query ante params introduced.
  • New cmd terrad query ante minimum-commission introduced.
  • New proposal /terra.ante.v2.MinimumCommissionUpdateProposal added
    • This proposal will check and update validator commissions, whose commission rate little than the given proposal value.
  • New proposal creation cmd terrad tx gov submit-proposal minimum-commission-update
{
  ...
  "content": {
    "@type": "/terra.ante.v2.MinimumCommissionUpdateProposal",
    "title": "minimum commission update",
    "description": "minimum commission update desc",
    "minimum_commission": "0.030000000000000000"
  },
  ...
}

Default ante.Params

// Ante params default values
const (
	// DefaultMinimumCommissionEnforced minimum commission Enforced flag
	DefaultMinimumCommissionEnforced bool = true
)

Default ante.GenesisState

// Ante params default values
var (
	// Default minimum commission rate
	DefaultMinimumCommission sdk.Dec = sdk.NewDecWithPrec(10, 2) // 10%
)

…mum rate and implement ante handler to check MsgCreateValidator and MsgEditValidator commission rate
@yun-yeo yun-yeo added the enhancement New feature or request label May 31, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2022

Codecov Report

Merging #47 (c66baa0) into main (5a3cd66) will decrease coverage by 2.86%.
The diff coverage is 61.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   73.70%   70.84%   -2.87%     
==========================================
  Files           7       17      +10     
  Lines         658      830     +172     
==========================================
+ Hits          485      588     +103     
- Misses        148      209      +61     
- Partials       25       33       +8     
Impacted Files Coverage Δ
app/ante/proposal_handler.go 0.00% <0.00%> (ø)
app/ante/types/codec.go 0.00% <0.00%> (ø)
app/upgrade.go 0.00% <0.00%> (ø)
app/ante/types/proposal.go 14.28% <14.28%> (ø)
app/ante/types/params.go 41.17% <41.17%> (ø)
app/ante/types/genesis.go 75.00% <75.00%> (ø)
app/app.go 88.40% <75.75%> (-1.29%) ⬇️
app/ante/min_commission.go 89.47% <89.47%> (ø)
app/ante/ante.go 71.42% <100.00%> (+1.05%) ⬆️
app/ante/keeper/grpc_querier.go 100.00% <100.00%> (ø)
... and 2 more

Copy link

@dylanschultzie dylanschultzie left a comment

Choose a reason for hiding this comment

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

Nice job, looks like you covered all the cases:

  1. if val_commission < minimum, upgrade it
  2. don't allow val to create at under minimum
  3. don't allow val to edit to below minimum
  4. DO allow to edit above minimum
  5. DO allow to create above minimum

The only case I see missing is creating with the minimum commission.

@AutoStake-com
Copy link

Nice job, looks like you covered all the cases:

  1. if val_commission < minimum, upgrade it
  2. don't allow val to create at under minimum
  3. don't allow val to edit to below minimum
  4. DO allow to edit above minimum
  5. DO allow to create above minimum

The only case I see missing is creating with the minimum commission.

Just seen this PR; nice!
Think these switch case statements will handle new validator creation messages
https://github.com/terra-money/core/pull/47/files#diff-e3bd85f729e3ebf3e9b2bb00eba3d579a0e53d80cc8a4750151279c9041ee81cR39

@yun-yeo yun-yeo self-assigned this Jun 20, 2022
@yun-yeo yun-yeo requested review from JSHan94 and Vritra4 June 20, 2022 06:12
@yun-yeo yun-yeo marked this pull request as ready for review June 20, 2022 06:12

if msg.CommissionRate.LT(minimumCommission) {
return sdkerrors.Wrap(sdkerrors.ErrUnauthorized,
fmt.Sprintf("commission can't be lower than %s%%", minimumCommission.String()),
Copy link

@JSHan94 JSHan94 Jun 20, 2022

Choose a reason for hiding this comment

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

fmt.Sprintf("commission can't be lower than %s%%", minimumCommission.String()),

it shows 0.1%, % should be removed

switch msg := m.(type) {
case *stakingtypes.MsgCreateValidator:
// prevent new validators joining the set with
// commission set below 5%
Copy link

Choose a reason for hiding this comment

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

10%, not 5%

@yun-yeo yun-yeo closed this Jun 30, 2022
@gregnuj gregnuj deleted the feature/minimum-commission branch March 20, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants