ready-for-review: Un-hardcode in governance parameters #1688

sunnya97 · 2018-07-16T04:26:30Z

moved governance parameters to globalparams store and can be set in genesis

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)

codecov · 2018-07-16T04:32:47Z

Codecov Report

Merging #1688 into develop will increase coverage by 0.04%.
The diff coverage is 78.57%.

@@             Coverage Diff             @@
##           develop    #1688      +/-   ##
===========================================
+ Coverage    63.42%   63.47%   +0.04%     
===========================================
  Files          117      117              
  Lines         6940     6973      +33     
===========================================
+ Hits          4402     4426      +24     
- Misses        2283     2292       +9     
  Partials       255      255

zmanian · 2018-07-16T07:33:25Z

Alternative to merging this for gaia 7000 would be to just change the voting period to a larger number

ebuchman · 2018-07-16T10:37:24Z

x/gov/keeper.go

+}
+
+func (keeper Keeper) setDepositProcedure(ctx sdk.Context, depositProcedure DepositProcedure) {
+	keeper.ps.Set(ctx, "gov/depositprocedure", &depositProcedure)


these strings should be well defined constants at top of file.

Also might be nicer as gov/procedure/deposit

rigelrozanski · 2018-07-16T14:00:43Z

x/gov/genesis.go

+			Threshold:         sdk.NewRat(1, 2),
+			Veto:              sdk.NewRat(1, 3),
+			GovernancePenalty: sdk.NewRat(1, 100),
+		},


Is there any other point to splitting up the governance parameters into these Procedure structs besides organizational? If not I'd almost prefer to see all the parameters in one struct - seems a bit confusing to add another layer of abstraction where it's not a requirement of codes functionality

Voting params, DepositParams, and TallyingParams are used at different times. No need to Unmarshal the voting params when I'm just worried about tallying. Also, in future, there will be different types of "tallying params" and stuff for different types of proposals

Interesting so in the future tallying params should actually be an interface as opposed to a struct to allow for different parameters - then functions such as CheckVeto could be functions on TallyingProceedure

👍 seems fine to leave as is for now, thanks for the insight

ValarDragon · 2018-07-16T21:30:58Z

Can you update your dep, and re-run make get_vendor_deps and see if that makes your gopkg.lock match?

sunnya97 · 2018-07-16T22:04:30Z

I did that. It still gave me this Gopkg.lock

rigelrozanski · 2018-07-20T16:03:07Z

@sunnya97 you've been slashed! - for including the checkbox applied appropriate labels.. when really no labels have been applied!

sunnya97 · 2018-07-20T18:58:24Z

Ohhh, I thought that meant to the title of the PR lol

cwgoes

PENDING.md & dep upgrade, plus file conflicts, otherwise LGTM.

cwgoes · 2018-07-20T22:17:10Z

CHANGELOG.md

@@ -13,6 +13,8 @@ BREAKING CHANGES

 FEATURES
 * [baseapp] NewBaseApp now takes option functions as parameters
+* [x/gov] Governance parameters are now stored in globalparams store


Move to PENDING.md

cwgoes · 2018-07-20T22:17:29Z

Gopkg.lock

@@ -2,78 +2,59 @@



Update dep version & re-run make get_vendor_deps.

…enesis

cwgoes

utACK

…s#1688) Bumps [github.com/cosmos/ibc-go/v5](https://github.com/cosmos/ibc-go) from 5.0.0-beta1 to 5.0.0-rc0. <details> <summary>Release notes</summary> Sourced from <a href="https://github.com/cosmos/ibc-go/releases">github.com/cosmos/ibc-go/v5's releases</a>. <blockquote> <h2>v5.0.0-rc0</h2> This release bumps the Cosmos SDK to v0.46 (for more information, see <a href="https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.0">Cosmos SDK v0.46 Release Notes</a>) and Tendermint to v0.34.20 (for more information, see <a href="https://github.com/tendermint/tendermint/blob/v0.34.20/CHANGELOG.md#v03414">Tendermint 0.34.20 changelog</a>). It also introduces several code style improvements flagged by linting tools. Please see the <a href="https://github.com/cosmos/ibc-go/blob/v5.0.0-rc0/CHANGELOG.md">v5.0.0 changelog</a> for the full set of changes included in this pre-release. Special thanks to our external contributors on this release: <a href="https://github.com/faddat"><code>@faddat</code></a> <hr /> To learn more about ibc-go versioning, please read our <a href="https://github.com/cosmos/ibc-go/blob/main/RELEASES.md">RELEASES.md</a>. IMPORTANT: Please read the migration guides for any versions of ibc-go that you might be going through when upgrading to this version. For example: if you upgrade from the IBC module contained in the Cosmos SDK 0.42.0 to SDK v0.46 and ibc-go v5.0.0-rc0, please follow: <ol> <li>The <a href="https://github.com/cosmos/ibc-go/blob/v5.0.0-beta1/docs/migrations/sdk-to-v1.md">migration from SDK 0.41.x or 0.42.x to the IBC module in the ibc-go repository based on the SDK v0.44.x</a>.</li> <li>The <a href="https://github.com/cosmos/ibc-go/blob/v5.0.0-beta1/docs/migrations/v1-to-v2.md">migration from ibc-go v1 to v2</a>.</li> <li>The <a href="https://github.com/cosmos/ibc-go/blob/v5.0.0-beta1/docs/migrations/v2-to-v3.md">migration from ibc-go v2 to v3</a>.</li> <li>The <a href="https://github.com/cosmos/ibc-go/blob/v5.0.0-beta1/docs/migrations/v3-to-v4.md">migration from ibc-go v3 to v4</a>.</li> <li>The migration from ibc-go v4 to v5 (not available yet, but work is in progress <a href="https://github-redirect.dependabot.com/cosmos/ibc-go/pull/1826">here</a>).</li> </ol> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/cosmos/ibc-go/commit/f106b747a0f3895e9b468d25057f2d949cfdb9a7"><code>f106b74</code></a> fix broken link (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/2059">#2059</a>) (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/2098">#2098</a>)</li> <li><a href="https://github.com/cosmos/ibc-go/commit/e1f7e89dc1e28330474fcf4c5694c4f5c759bb92"><code>e1f7e89</code></a> chore: update fee middleware docs to be more explicit about reasoning for rem...</li> <li><a href="https://github.com/cosmos/ibc-go/commit/2f3aa53070b5048b280eaecd3aa7e5309d6f3b06"><code>2f3aa53</code></a> fix: "acknowledgement written" logs unsupported type (backport <a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1919">#1919</a>) (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1962">#1962</a>)</li> <li><a href="https://github.com/cosmos/ibc-go/commit/07d3f555b1bd27983e6f47fbe4be01925ffd45ca"><code>07d3f55</code></a> fix: adding check for blocked addresses before escrowing fees (backport <a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1890">#1890</a>...</li> <li><a href="https://github.com/cosmos/ibc-go/commit/c9894c932f6c663c0e017f0e6f306a61d239b595"><code>c9894c9</code></a> add mock module acc balance and refactor test case (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1942">#1942</a>) (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1953">#1953</a>)</li> <li><a href="https://github.com/cosmos/ibc-go/commit/f400db4699c1bd26a5bf1b9e81da946b297c4469"><code>f400db4</code></a> fix: prevent blocked addresses from sending ICS 20 transfers (backport <a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1907">#1907</a>)...</li> <li><a href="https://github.com/cosmos/ibc-go/commit/b579e3b859b8abe79b1f3c773ab3c08627792114"><code>b579e3b</code></a> fix: ics27 check packet data length explicitly over nil check (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1882">#1882</a>) (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1899">#1899</a>)</li> <li><a href="https://github.com/cosmos/ibc-go/commit/c57b77419fd8e4d131f1f81ea7bbfe5417a6344b"><code>c57b774</code></a> fix: make format and golangci-lint errors (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1894">#1894</a>) (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1903">#1903</a>)</li> <li><a href="https://github.com/cosmos/ibc-go/commit/4d99267f0cd63ca04e8f61faab78b8829ab83739"><code>4d99267</code></a> remove changelog entries that will be released as part of v4</li> <li><a href="https://github.com/cosmos/ibc-go/commit/cc1f70472f06ae9b09b9583aadb81e3486106530"><code>cc1f704</code></a> build(deps): bump cosmossdk.io/math from 1.0.0-beta.2 to 1.0.0-beta.3 (<a href="https://github-redirect.dependabot.com/cosmos/ibc-go/issues/1842">#1842</a>)...</li> <li>See full diff in <a href="https://github.com/cosmos/ibc-go/compare/v5.0.0-beta1...v5.0.0-rc0">compare view</a></li> </ul> </details> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=github.com/cosmos/ibc-go/v5&package-manager=go_modules&previous-version=5.0.0-beta1&new-version=5.0.0-rc0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>

sunnya97 requested review from cwgoes, ebuchman and rigelrozanski as code owners July 16, 2018 04:26

ebuchman reviewed Jul 16, 2018

View reviewed changes

rigelrozanski reviewed Jul 16, 2018

View reviewed changes

ebuchman changed the base branch from master to develop July 16, 2018 15:35

ValarDragon mentioned this pull request Jul 16, 2018

Automatic Changelogs tendermint/coding#69

Open

rigelrozanski added ready-for-review C:x/gov labels Jul 20, 2018

cwgoes suggested changes Jul 20, 2018

View reviewed changes

sunnya97 added 6 commits July 26, 2018 15:30

moved governance parameters to globalparams store and can be set in g…

24bae44

…enesis

undid Gopkg.lock changes

cda318c

changelog

c1861a5

added constants for key strings

3860997

Gopkg.lock

c6af61a

rebased develop

874440c

sunnya97 force-pushed the sunny/gov_params branch from 5ddc9d0 to 874440c Compare July 26, 2018 23:16

Changelog -> PENDING

b9d6e86

sunnya97 assigned cwgoes Jul 27, 2018

Changelog -> pending, second entry

422fb4b

cwgoes approved these changes Jul 27, 2018

View reviewed changes

cwgoes merged commit 55ef898 into develop Jul 27, 2018

cwgoes deleted the sunny/gov_params branch July 27, 2018 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ready-for-review: Un-hardcode in governance parameters #1688

ready-for-review: Un-hardcode in governance parameters #1688

sunnya97 commented Jul 16, 2018 •

edited by cwgoes

Loading

codecov bot commented Jul 16, 2018 •

edited

Loading

zmanian commented Jul 16, 2018

ebuchman Jul 16, 2018

sunnya97 Jul 16, 2018

rigelrozanski Jul 16, 2018 •

edited

Loading

sunnya97 Jul 16, 2018

rigelrozanski Jul 16, 2018

rigelrozanski Jul 16, 2018

ValarDragon commented Jul 16, 2018 •

edited

Loading

sunnya97 commented Jul 16, 2018

rigelrozanski commented Jul 20, 2018

sunnya97 commented Jul 20, 2018

cwgoes left a comment

cwgoes Jul 20, 2018

cwgoes Jul 20, 2018

cwgoes left a comment

ready-for-review: Un-hardcode in governance parameters #1688

ready-for-review: Un-hardcode in governance parameters #1688

Conversation

sunnya97 commented Jul 16, 2018 • edited by cwgoes Loading

codecov bot commented Jul 16, 2018 • edited Loading

Codecov Report

zmanian commented Jul 16, 2018

ebuchman Jul 16, 2018

Choose a reason for hiding this comment

sunnya97 Jul 16, 2018

Choose a reason for hiding this comment

rigelrozanski Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

sunnya97 Jul 16, 2018

Choose a reason for hiding this comment

rigelrozanski Jul 16, 2018

Choose a reason for hiding this comment

rigelrozanski Jul 16, 2018

Choose a reason for hiding this comment

ValarDragon commented Jul 16, 2018 • edited Loading

sunnya97 commented Jul 16, 2018

rigelrozanski commented Jul 20, 2018

sunnya97 commented Jul 20, 2018

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes Jul 20, 2018

Choose a reason for hiding this comment

cwgoes Jul 20, 2018

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

sunnya97 commented Jul 16, 2018 •

edited by cwgoes

Loading

codecov bot commented Jul 16, 2018 •

edited

Loading

rigelrozanski Jul 16, 2018 •

edited

Loading

ValarDragon commented Jul 16, 2018 •

edited

Loading