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

Add a column for which params are governance modifiable #2966

Closed
rootulp opened this issue Dec 27, 2023 · 5 comments
Closed

Add a column for which params are governance modifiable #2966

rootulp opened this issue Dec 27, 2023 · 5 comments
Assignees
Labels
good first issue Good for newcomers specs directly relevant to the specs

Comments

@rootulp
Copy link
Collaborator

rootulp commented Dec 27, 2023

Context

## Parameters
There are three parameters that can be modified via governance to modify gas
usage.
| Parameter | Default Value | Description |
|-----------------|---------------|-----------------------------------------|
| consensus/max_gas | -1 | The maximum gas allowed in a block. Default of -1 means this value is not capped. |
| auth/tx_size_cost_per_byte | 10 | Gas used per each byte used by the transaction. |
| auth/sig_verify_cost_secp256k1 | 1000 | Gas used per verifying a secp256k1 signature |
| blob/gas_per_blob_byte | 8 | Gas used per byte used by blob. Note that this value is applied to all encoding overhead, meaning things like the padding of the remaining share and namespace. See PFB gas estimation section for more details. |

Problem

The statement before the table claims 3 params are governance modifiable but it's not immediately clear which 3 out of the 4 in the table are governance modifiable.

Proposal

Add a column to the table that states if a param is governance modifiable

@rootulp rootulp added good first issue Good for newcomers specs directly relevant to the specs labels Dec 27, 2023
@SanjayNithin2002
Copy link

Can you tell which parameter is not governance modifiable?

@rootulp
Copy link
Collaborator Author

rootulp commented Dec 27, 2023

I can't. We do have https://celestiaorg.github.io/celestia-app/specs/params.html but it may not be 100% correct because #2916

@fahimahmedx
Copy link
Contributor

Made a PR for this: #2974

evan-forbes pushed a commit that referenced this issue Jan 2, 2024
…#2974)

## Overview
Addresses #2966 by using the governance modifiable params tests in PR
#2973.
All four of the params are modifiable by governance, which matches the
specs in
[params.md](https://github.com/celestiaorg/celestia-app/blob/main/specs/src/specs/params.md).

## Checklist
- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords
@fahimahmedx
Copy link
Contributor

PR is merged, this can be closed now! :)

@rootulp
Copy link
Collaborator Author

rootulp commented Jan 3, 2024

Thanks @fahimahmedx !

@rootulp rootulp closed this as completed Jan 3, 2024
rootulp pushed a commit to rootulp/celestia-app that referenced this issue Jan 11, 2024
add auth, blob, blobstream, and consensus governance param tests

add governance params tests for distribution

add tests for modifying gov params

add tests for modifying ibc params

add tests for modifying slashing params

add tests for modifying staking params

ci: remove version_bump job (celestiaorg#2962)

Closes celestiaorg#2884

- Remove the version_bump and branch_name jobs
- Extract the goreleaser jobs to a new file

Can still create a release. See
https://github.com/rootulp/celestia-app/actions/runs/7324051789/job/19947265102

docs: add column for governance modifiable params in resource_pricing (celestiaorg#2974)

Addresses celestiaorg#2966 by using the governance modifiable params tests in PR
All four of the params are modifiable by governance, which matches the
specs in
[params.md](https://github.com/celestiaorg/celestia-app/blob/main/specs/src/specs/params.md).

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords

chore: remove unnecessary conversions (celestiaorg#2975)

Use linter to detect unnecessary type conversions
https://github.com/mdempsky/unconvert

ci: enable govulncheck (celestiaorg#2969)

Closes celestiaorg#2968
Supersedes celestiaorg#2744

- CI workflow is inspired by
https://github.com/celestiaorg/celestia-core/blob/main/.github/workflows/govulncheck.yml
- Bumped to Go 1.21.5 to resolve some vulnerabilities identified by
govulncheck

improve naming of governance params test suite

fix govHandler and remove test for an unmodifiable param.

remove unneeded minter setup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers specs directly relevant to the specs
Projects
None yet
Development

No branches or pull requests

3 participants