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

Param Change Proposal #4206

Merged
merged 108 commits into from Apr 30, 2019

Conversation

@alexanderbez
Copy link
Contributor

commented Apr 26, 2019

This PR addresses the implementation of parameter change proposals. It's major features and changes include:

  • Minor updates to the x/param module
    • Cleanup and moving types to types/ package
    • New ParameterChangeProposal and ParamChange types
  • Updates to the x/gov module
    • Cleanup and moving types to types/ package
    • Disable SoftwareUpgrade proposals
    • Implement Content and Handler interfaces
    • Implement and use internal Content router in the keeper
    • Update EndBlocker to execute Content handler
  • Update docs
  • Update client functionality

This is state machine breaking. The proposal's Content JSON field has been renamed and SoftwareUpgrade are temporarily disabled.

A follow up PR should be made to add param change proposals to simulation.


replaces/supersedes: #3880
closes: #3565

credit: @mossid


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

alexanderbez added some commits Apr 16, 2019

@jackzampolin
Copy link
Contributor

left a comment

Thanks for walking me through this @alexanderbez! Looks great! 👍 We also need to make an issue to ensure we don't forget to migrate state to work with these changes.

@mossid
Copy link
Contributor

left a comment

Looks great, left some comments!

Show resolved Hide resolved docs/spec/governance/02_state.md Outdated
Show resolved Hide resolved x/gov/client/utils/utils.go
Show resolved Hide resolved x/gov/types/msgs.go
@@ -0,0 +1,99 @@
package params_test

This comment has been minimized.

Copy link
@mossid

mossid Apr 29, 2019

Contributor

++

This comment has been minimized.

Copy link
@rigelrozanski

rigelrozanski Apr 30, 2019

Member

? I'm confused by this package name

This comment has been minimized.

Copy link
@alexanderbez

alexanderbez Apr 30, 2019

Author Contributor

It's under the params package. Go allows you to name your test file packages with _test to have its contents be private and separate it from the package that is being tested -- very useful. Here we're testing the handler.

{
"subspace": "staking",
"key": "MaxValidators",
"value": "105"

This comment has been minimized.

Copy link
@mossid

mossid Apr 29, 2019

Contributor

I wonder if there is a convenient way to use a json value directly in the value field and later stringify. Currently the parameter store is using json encoding only so it won't be a problem. So in this case the field value will be 105 not "105". Double encoded json value is annoying sometimes too. For example string encoded integer "100" has to be written as "\"100\"". It can be addressed in another PR.

This comment has been minimized.

Copy link
@mossid

mossid Apr 29, 2019

Contributor

https://golang.org/pkg/encoding/json/#RawMessage seems like an option - we can leave the target struct's value field to json.RawMessage and use it directly as the value string in parameter change.

This comment has been minimized.

Copy link
@alexanderbez

alexanderbez Apr 30, 2019

Author Contributor

@mossid can you open up a separate PR for this? I'd like to potentially address this outside of this PR as it's not blocking.

@rigelrozanski
Copy link
Member

left a comment

Well done! - Good design, and nice refactors.

general comments on the docs, and code structure - nothing too big.

Show resolved Hide resolved docs/spec/governance/01_concepts.md Outdated
Show resolved Hide resolved docs/spec/governance/02_state.md Outdated
Show resolved Hide resolved docs/spec/governance/02_state.md Outdated
Show resolved Hide resolved docs/spec/governance/02_state.md
Show resolved Hide resolved docs/spec/governance/02_state.md Outdated
Show resolved Hide resolved x/gov/types/msgs.go
Show resolved Hide resolved x/params/types/proposal.go
Show resolved Hide resolved x/params/types/proposal.go Outdated
Show resolved Hide resolved x/params/handler.go Outdated
Show resolved Hide resolved cmd/gaia/app/app.go Outdated

mossid and others added some commits Apr 30, 2019

Update docs/spec/governance/02_state.md
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Update docs/spec/governance/02_state.md
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Update docs/spec/governance/02_state.md
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
@codecov

This comment has been minimized.

Copy link

commented Apr 30, 2019

Codecov Report

Merging #4206 into master will increase coverage by 0.27%.
The diff coverage is 48.33%.

@@            Coverage Diff             @@
##           master    #4206      +/-   ##
==========================================
+ Coverage   60.17%   60.45%   +0.27%     
==========================================
  Files         212      218       +6     
  Lines       15187    15355     +168     
==========================================
+ Hits         9139     9283     +144     
- Misses       5419     5432      +13     
- Partials      629      640      +11

alexanderbez and others added some commits Apr 30, 2019

Update x/gov/client/cli/tx.go
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Show resolved Hide resolved x/gov/client/cli/tx.go Outdated

alexanderbez and others added some commits Apr 30, 2019

Merge branch '3565-param-change-proposal-4' of github.com:cosmos/cosm…
…os-sdk into 3565-param-change-proposal-4
Update x/params/types/proposal.go
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>

@alexanderbez alexanderbez requested review from rigelrozanski and mossid Apr 30, 2019

@alexanderbez

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@rigelrozanski @cwgoes @mossid I've addressed all your comments :)

alexanderbez added some commits Apr 30, 2019

@rigelrozanski
Copy link
Member

left a comment

🥇

@rigelrozanski rigelrozanski merged commit 5ca93ac into master Apr 30, 2019

14 of 15 checks passed

codecov/patch 48.33% of diff hit (target 60.17%)
Details
GolangCI No issues found!
Details
ci/circleci: docker_image Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: localnet Your tests passed on CircleCI!
Details
ci/circleci: setup_dependencies Your tests passed on CircleCI!
Details
ci/circleci: test_cover Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_fast Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_import_export Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_multi_seed Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_nondeterminism Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_simulation_after_import Your tests passed on CircleCI!
Details
ci/circleci: upload_coverage Your tests passed on CircleCI!
Details
codecov/project 60.45% (+0.27%) compared to 9a16e26
Details

@rigelrozanski rigelrozanski deleted the 3565-param-change-proposal-4 branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.