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.

Sign up for GitHub

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

Jump to bottom

Parameters that are stated as not governance modifiable, are actually governance modifiable. #2972

Closed
fahimahmedx opened this issue Dec 31, 2023 · 3 comments · Fixed by #3025
Closed

Parameters that are stated as not governance modifiable, are actually governance modifiable. #2972

fahimahmedx opened this issue Dec 31, 2023 · 3 comments · Fixed by #3025
Assignees
@rootulp
Labels
specs directly relevant to the specs

Comments

@fahimahmedx
Copy link
Contributor

fahimahmedx commented Dec 31, 2023 •
edited by rootulp
Loading

Problem

Two governance params are actually modifiable when the specs claim they aren't governance modifiable:

  • consensus.Version.AppVersion
  • staking.MaxValidators

Proposal

Update the specs
@fahimahmedx fahimahmedx added the bug Something isn't working label Dec 31, 2023
@rootulp
Copy link
Collaborator

rootulp commented Jan 3, 2024

IMO we should update the specs to state these params are governance modifiable. We'll also need to update https://github.com/celestiaorg/CIPs/blob/main/cips/cip-13.md

We can only make them non governance modifiable via a hard-fork which requires a CIP.

@rootulp
Copy link
Collaborator

rootulp commented Jan 11, 2024

The original issue isn't accurate because these aren't governance modifiable:

  • bank.SendEnabled
  • staking.BondDenom
  • staking.UnbondingTime
  • conensus.validator.PubKeyTypes

Modification is prevented via the paramfilter.

@rootulp
Copy link
Collaborator

rootulp commented Jan 11, 2024

Since only two params are modifiable

  • consensus.Version.AppVersion
  • staking.MaxValidators

Let's repurpose this issue to fix the specs for those two.

@rootulp rootulp self-assigned this Jan 11, 2024
@rootulp rootulp added specs directly relevant to the specs and removed bug Something isn't working labels Jan 11, 2024
This was referenced Jan 11, 2024
Merged
Merged
Merged
rootulp added a commit that referenced this issue Jan 22, 2024
@rootulp
docs: mark two params as gov modifiable (#3025)
1fd4b34 
Closes #2972
rootulp added a commit that referenced this issue Jan 29, 2024
@rootulp @fahimahmedx
test: increase test coverage for governance params (#3007)
88b58cf 
Closes #2916 with
heavy inspiration from PRs by @fahimahmedx
Opens #2972

---------

Co-authored-by: fahimahmedx <[email protected]>
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
@rootulp
docs: mark two params as gov modifiable (#3025)
d48002c 
Closes celestiaorg/celestia-app#2972
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
@rootulp @fahimahmedx
test: increase test coverage for governance params (#3007)
d4a2eec 
Closes celestiaorg/celestia-app#2916 with
heavy inspiration from PRs by @fahimahmedx
Opens celestiaorg/celestia-app#2972

---------

Co-authored-by: fahimahmedx <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rootulp rootulp

Labels
specs directly relevant to the specs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

docs: mark two params as gov modifiable rootulp/celestia-app
2 participants
@rootulp @fahimahmedx