Add MinCommissionRate param #9245
Conversation
github-actions
bot
added
C:CLI
C:Simulations
T:Docs
Codecov Report
@@ Coverage Diff @@
## master #9245 +/- ##
==========================================
- Coverage 60.30% 60.13% -0.17%
==========================================
Files 591 595 +4
Lines 36937 37217 +280
==========================================
+ Hits 22276 22382 +106
- Misses 12691 12853 +162
- Partials 1970 1982 +12
|
|
This looks like a really cool improvement @sunnya97 and happy to give my concept ACK. In terms of process, I'd like you to present something briefly at the next architecture call to just make sure there's general consensus and also visibility (adding a status label for that). And then we'll probably want a brief ADR or CIP in addition to the PR to document the change. Since this affects the hub I think the preference is for a CIP. |
aaronc
added
the
S:needs architecture review
|
Opened a CIP: cosmos/cips#3 |
| @@ -6427,6 +6427,7 @@ Params defines the parameters for the staking module. | |||
| | `historical_entries` | [uint32](#uint32) | | historical_entries is the number of historical entries to persist. | | |||
| | `bond_denom` | [string](#string) | | bond_denom defines the bondable coin denomination. | | |||
| | `power_reduction` | [string](#string) | | power_reduction is the amount of staking tokens required for 1 unit of consensus-engine power | | |||
| | `min_commission_rate` | [string](#string) | | | | |||
There was a problem hiding this comment.
Can we add a description here?
This is a great question. I think the correct behavior should be that their commission is effectively the minimum commission. Basically minimum commission overrides any lower values. I don't see such logic in this PR however. If we're in agreement on this, can you update this PR with that behavior @sunnya97 ? |
|
Yeah, I left that as an open question on the CIP. Is there a way to hook into a param change? I couldn't find one. We could add a hook that gets emitted on every param change, but then every receiver of that would have to filter by the param they care about. Another option @ValarDragon proposed is that similar to how every param has a |
|
Well the easiest option is to do nothing, i.e. current validator commissions that are below the governance-approved threshold are "grandfathered" in. If at any point in the future a validator wants to change their commission, they'll be forced at that point to be above the threshold. I don't see any other non-contentious way? |
But couldn't grandfathered be seen as contentious too - as a way to get around governance? Maybe some validators will keep grandfathered low commissions indefinitely. Implementation wise, I think it would be pretty simple for distribution to use the min commission rate if it is above the validator's current commission when allocating tokens. |
Yes, but less so. Certain validators may have legal or similar constraints in their current commission. To have the network change it for them to the threshold could be problematic for them. However, given that this must go through governance, it could give them enough time to figure their stuff out. |
Don't your gov hooks allow this? |
There's one hook that is I supposed that technically I could use that, but it would be really messy. Staking would need to first get the proposal, try to see if its a ParamChange proposal, and if it is, see if it's the MinCommissionRate change proposal to act upon. Also, there's no guarantee that governance proposals are the only way to change a param. With things like the groups module, control over certain params might be delegated to certain groups or even multisigs. The governance hooks are meant more for things that want to hook into the governance procedures. I think this should be done using proper param module hooks. |
Oh interesting. Are you suggesting we don't actually go change validator's commission rates in their struct, but rather in the distribution module, we use That could potentially work. But then when you query a validator, it will say it's original declared CommissionRate which might be misleading. I think the params hooks probably are safer. |
I think that's the right approach 👍 |
|
@fedekunze But what if the param gets changed by something other than a governance proposal? |
I think that approach is pretty untenable. The hook logic for every module would then have a routing mess, and it'd have tons of unnecessary call overhead. Every time a parameter changes, every module who wants to check for updates to one of their own params would have to filter it. Why go for more routing / filter overheads, when we already have a spot that allows the module to specify functions to be ran on proposal updates. (The current ValidateParameterUpdate) I've been meaning to write an issue for |
|
This issue is relevant to the Terra community as well. We are discussing setting this parameter in a governance vote. How can we assist to make this a reality? https://agora.terra.money/t/setting-minimum-validator-commissions/1490/8 |
|
I'm in favor of this. |
|
Cool, so let's include this in v0.44. @sunnya97 Are you still okay to work on this? |
| } | ||
|
|
||
| // MigrateStore performs in-place store migrations from v0.40 to v0.43. The | ||
| // migration includes: | ||
| // | ||
| // - Setting the Power Reduction param in the paramstore | ||
| // - Setting the MinCommissionRate param in the paramstore |
There was a problem hiding this comment.
migrations look good, but now they need to go to a 0.44 folder
There was a problem hiding this comment.
@AmauryM now, 0.45 folder?
|
@sunnya97 lmk if you want help with this PR. |
|
@alexanderbez would love some help. The main blocker is on how to update all existing commission rates every time there's a param change. |
Just checking here, are you still stuck on this? |
|
We're instead looking to do a more general and broad x/params refactor: #9913 |
any chance we can merge this then start the refactor? should be easy enough |
|
@sunnya97 can you get this updated? lets get it merged. |
|
We need to have a discussion if this PR is still needed. We merged the params ADR: https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-046-module-params.md Osmosis and other chains can just take this approach (pending governance/group changes to the SDK). |
|
I'll PR to sunny's PR |
|
Just to check, do the chains need to do a Stargate-style network upgrade if this PR gets included in the next stable release due to it breaking the current genesis file? |
this will be an in-place store migration |
So a normal upgrade is possible? Then do we need to export an updated genesis for those who want to setup state sync node? |
@Terra854 , in-pace migration is not compatible with genesis import-export. The former one will continue the chain, the latter one will start from scratch. |
|
I agree with @alexanderbez - now with module params it would be much better to rework this PR, rather than creating two incompatible versions. |
@Terra854 , yes , the "genesis" update is possible. But it must be coordinated - so all network must do either genesis update or in-place update. Otherwise you will have a consensus problem.
state sync work with both genesis update and in-place update. |
|
@marbar3778 Updated in #10422 |
Yeah, my guys at Crypto.org Chain really wants to get this PR merged asap as they are having a problem with everyone delegating towards 0% commission validators and a rogue actor taking advantage of it, jumping between 0% to get more delegations and a high value to soak up a lot of the rewards. |
|
Totally @Terra854 -- we just need the x/group changes to land, right @cmwaters @robert-zaremba ? |
|
closing in favour of #10422 |
Description
Added the ability to set a minimum commission rate that all validators cannot set their commission rate below.
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes