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

feat: add minimum commission rate to x/staking #27

Merged
merged 9 commits into from
Apr 5, 2022

Conversation

youngjoon-lee
Copy link

@youngjoon-lee youngjoon-lee commented Mar 29, 2022

Important

This PR is based on the v0.42-panacea branch which points to the v0.42.11 tag. Although the current panacea-core v2.0.2 uses cosmos-sdk v0.42.9, I thought it's okay to use cosmos-sdk v0.42.11 because there's no serious breaking changes. Also, if we decided to implement this feature based on v0.42.9, the branch strategy may become complicated.

Changes

  • Added a new min_commission_rate genesis param of the x/staking module.
  • Modified the logic CreateValidator and UpdateValidator to check if the desired commission rate is >= min_commission_rate.
  • Added a special function for updating validator commissions without checking max_commission_change_rate.
    • Our chain upgrade strategy is updating the commission rate of all validators whose commission rate is smaller than min_commission_rate. In normal mode, this logic can cause an error: commission change rate cannot be more than the max rate. That's why we need a special function which doesn't ignore the max_commission_change_rate param.

Tips for reviewers

You don't need to review changes which are displayed as Some generated files are not rendered because those file are generated by make proto-gen. Unfortunately, Cosmos guys didn't execute make proto-gen after modifying proto files. That's why so many unexpected .pb.go files have been regenerated in this PR.

References

What's next?

Please jump to medibloc/panacea-core#291.

@youngjoon-lee youngjoon-lee force-pushed the v0.42-panacea-min-commission-rate branch 2 times, most recently from d325e85 to fc2177f Compare March 30, 2022 07:15
@youngjoon-lee youngjoon-lee force-pushed the v0.42-panacea-min-commission-rate branch from fc2177f to f9ba875 Compare March 30, 2022 07:18
@youngjoon-lee youngjoon-lee force-pushed the v0.42-panacea-min-commission-rate branch from 455c861 to 7e81e1b Compare March 30, 2022 08:06
@youngjoon-lee youngjoon-lee force-pushed the v0.42-panacea-min-commission-rate branch from a335170 to 9241201 Compare March 30, 2022 08:48
@youngjoon-lee youngjoon-lee marked this pull request as ready for review March 30, 2022 11:11
@youngjoon-lee youngjoon-lee marked this pull request as draft March 30, 2022 11:11
@youngjoon-lee youngjoon-lee marked this pull request as ready for review March 30, 2022 11:35
@youngjoon-lee youngjoon-lee self-assigned this Mar 30, 2022
@youngjoon-lee youngjoon-lee requested a review from inchori March 30, 2022 11:37
@youngjoon-lee youngjoon-lee marked this pull request as draft March 31, 2022 00:19
@youngjoon-lee youngjoon-lee changed the title feat: add minimum commission rate to x/staking feat: WIP: add minimum commission rate to x/staking Mar 31, 2022
@youngjoon-lee youngjoon-lee changed the title feat: WIP: add minimum commission rate to x/staking feat: add minimum commission rate to x/staking Mar 31, 2022
@youngjoon-lee youngjoon-lee marked this pull request as ready for review March 31, 2022 02:31
DOCKER := $(shell which docker)
DOCKER_BUF := $(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace bufbuild/buf
DOCKER_BUF := $(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace bufbuild/buf:0.56.0
Copy link
Author

Choose a reason for hiding this comment

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

I specified the version explicitly because the buf > 0.56.0 has so many CLI breaking changes. It's safe to use 0.56.0 since this Makefile was written when the latest version of buf was 0.56.0.

Copy link

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gyuguen
Copy link

gyuguen commented Mar 31, 2022

Ah, I have one question.
It seems that this min commission is not supported by cosmos-sdk.
Should this change be applied later when upgrading the cosmos-sdk version?

@youngjoon-lee
Copy link
Author

youngjoon-lee commented Mar 31, 2022

@gyuguen

It seems that this min commission is not supported by cosmos-sdk.

Yeah. That's why I forked cosmos/cosmos-sdk as medibloc/cosmos-sdk.

Should this change be applied later when upgrading the cosmos-sdk version?

After merging this PR, we will release the v0.42.11-panacea tag from this medibloc/cosmos-sdk repository. Then, we will change the cosmos-sdk dependency of panacea-core from cosmos/cosmos-sdk v0.42.9 to medibloc/cosmos-sdk v0.42.11-panacea. That have been already done in the PR medibloc/panacea-core#291.
If you're worried about the data market protocol that is being developed in the master branch of panacea-core, it would be all good if we use medibloc/cosmos-sdk v0.42.11-panacea in the master branch as well.
In fact, we have to use the medibloc/cosmos-sdk instead of cosmos/cosmos-sdk forever as Osmosis does.

@youngjoon-lee youngjoon-lee merged commit 4680505 into v0.42-panacea Apr 5, 2022
@youngjoon-lee youngjoon-lee deleted the v0.42-panacea-min-commission-rate branch April 5, 2022 02:57
@youngjoon-lee
Copy link
Author

I merged this PR even though I got only one approval, in order to submit the software upgrade gov proposal soon.
But, please revisit this PR if you have any question or suggestion.

@youngjoon-lee
Copy link
Author

As Osmosis does in their osmosis-labs/cosmos-sdk forked repo, it would be okay to not update the CHANGELOG.md because of potential merge-conflicts with the upstream repo. Instead, I will write the release note in detail at GitHub Release pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants