ready-for-review: Un-hardcode in governance parameters #1688
Conversation
sunnya97
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
Codecov Report
@@ 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 |
|
Alternative to merging this for gaia 7000 would be to just change the voting period to a larger number |
x/gov/keeper.go
Outdated
| } | ||
|
|
||
| func (keeper Keeper) setDepositProcedure(ctx sdk.Context, depositProcedure DepositProcedure) { | ||
| keeper.ps.Set(ctx, "gov/depositprocedure", &depositProcedure) |
There was a problem hiding this comment.
these strings should be well defined constants at top of file.
Also might be nicer as gov/procedure/deposit
| Threshold: sdk.NewRat(1, 2), | ||
| Veto: sdk.NewRat(1, 3), | ||
| GovernancePenalty: sdk.NewRat(1, 100), | ||
| }, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
👍 seems fine to leave as is for now, thanks for the insight
|
Can you update your dep, and re-run make get_vendor_deps and see if that makes your gopkg.lock match? |
|
I did that. It still gave me this Gopkg.lock |
|
@sunnya97 you've been slashed! - for including the checkbox applied appropriate labels.. when really no labels have been applied! |
|
Ohhh, I thought that meant to the title of the PR lol |
CHANGELOG.md
Outdated
| @@ -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 | |||
| @@ -2,78 +2,59 @@ | |||
|
|
|||
There was a problem hiding this comment.
Update dep version & re-run make get_vendor_deps.
sunnya97
force-pushed
the
sunny/gov_params
branch
from
5ddc9d0
to
874440c
Compare
moved governance parameters to globalparams store and can be set in genesis
docs/)CHANGELOG.mdcmd/gaiaandexamples/For Admin Use: