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

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

Merged
merged 8 commits into from
Jul 27, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
24bae44
moved governance parameters to globalparams store and can be set in g…
sunnya97 Jul 16, 2018
cda318c
undid Gopkg.lock changes
sunnya97 Jul 16, 2018
c1861a5
changelog
sunnya97 Jul 16, 2018
3860997
added constants for key strings
sunnya97 Jul 16, 2018
c6af61a
Gopkg.lock
sunnya97 Jul 26, 2018
874440c
rebased develop
sunnya97 Jul 26, 2018
b9d6e86
Changelog -> PENDING
sunnya97 Jul 26, 2018
422fb4b
Changelog -> pending, second entry
cwgoes Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 26 additions & 27 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ BREAKING CHANGES
* `gaiacli gov deposit --depositer`
* `gaiacli gov vote --voter`
* [x/gov] Added tags sub-package, changed tags to use dash-case
* [x/gov] Governance parameters are now stored in globalparams store

FEATURES
* [lcd] Can now query governance proposals by ProposalStatus
Expand All @@ -47,6 +48,7 @@ IMPROVEMENTS
* [x/bank] Unit tests are now table-driven
* [tests] Fixes ansible scripts to work with AWS too
* [tests] \#1806 CLI tests are now behind the build flag 'cli_test', so go test works on a new repo
* [x/gov] Initial governance parameters can now be set in the genesis file

BUG FIXES
* \#1666 Add intra-tx counter to the genesis validators
Expand All @@ -55,4 +57,4 @@ BUG FIXES
* \#1766 Fixes bad example for keybase identity
* \#1804 Fixes gen-tx genesis generation logic temporarily until upstream updates
* \#1799 Fix `gaiad export`
* \#1828 Force user to specify amount on create-validator command by removing default
* \#1828 Force user to specify amount on create-validator command by removing default
2 changes: 1 addition & 1 deletion cmd/gaia/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptio
app.ibcMapper = ibc.NewMapper(app.cdc, app.keyIBC, app.RegisterCodespace(ibc.DefaultCodespace))
app.paramsKeeper = params.NewKeeper(app.cdc, app.keyParams)
app.stakeKeeper = stake.NewKeeper(app.cdc, app.keyStake, app.coinKeeper, app.RegisterCodespace(stake.DefaultCodespace))
app.govKeeper = gov.NewKeeper(app.cdc, app.keyGov, app.coinKeeper, app.stakeKeeper, app.RegisterCodespace(gov.DefaultCodespace))
app.govKeeper = gov.NewKeeper(app.cdc, app.keyGov, app.paramsKeeper.Setter(), app.coinKeeper, app.stakeKeeper, app.RegisterCodespace(gov.DefaultCodespace))
app.feeCollectionKeeper = auth.NewFeeCollectionKeeper(app.cdc, app.keyFeeCollection)
app.slashingKeeper = slashing.NewKeeper(app.cdc, app.keySlashing, app.stakeKeeper, app.paramsKeeper.Getter(), app.RegisterCodespace(slashing.DefaultCodespace))

Expand Down
35 changes: 31 additions & 4 deletions x/gov/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,37 @@ import (

// GenesisState - all staking state that must be provided at genesis
type GenesisState struct {
StartingProposalID int64 `json:"starting_proposalID"`
StartingProposalID int64 `json:"starting_proposalID"`
DepositProcedure DepositProcedure `json:"deposit_period"`
VotingProcedure VotingProcedure `json:"voting_period"`
TallyingProcedure TallyingProcedure `json:"tallying_procedure"`
}

func NewGenesisState(startingProposalID int64) GenesisState {
func NewGenesisState(startingProposalID int64, dp DepositProcedure, vp VotingProcedure, tp TallyingProcedure) GenesisState {
return GenesisState{
StartingProposalID: startingProposalID,
DepositProcedure: dp,
VotingProcedure: vp,
TallyingProcedure: tp,
}
}

// get raw genesis raw message for testing
func DefaultGenesisState() GenesisState {
return GenesisState{
StartingProposalID: 1,
DepositProcedure: DepositProcedure{
MinDeposit: sdk.Coins{sdk.NewCoin("steak", 10)},
MaxDepositPeriod: 200,
},
VotingProcedure: VotingProcedure{
VotingPeriod: 200,
},
TallyingProcedure: TallyingProcedure{
Threshold: sdk.NewRat(1, 2),
Veto: sdk.NewRat(1, 3),
GovernancePenalty: sdk.NewRat(1, 100),
},
Copy link
Contributor

@rigelrozanski rigelrozanski Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand All @@ -29,13 +47,22 @@ func InitGenesis(ctx sdk.Context, k Keeper, data GenesisState) {
// TODO: Handle this with #870
panic(err)
}
k.setDepositProcedure(ctx, data.DepositProcedure)
k.setVotingProcedure(ctx, data.VotingProcedure)
k.setTallyingProcedure(ctx, data.TallyingProcedure)
}

// WriteGenesis - output genesis parameters
func WriteGenesis(ctx sdk.Context, k Keeper) GenesisState {
initalProposalID, _ := k.getNewProposalID(ctx)
startingProposalID, _ := k.getNewProposalID(ctx)
depositProcedure := k.GetDepositProcedure(ctx)
votingProcedure := k.GetVotingProcedure(ctx)
tallyingProcedure := k.GetTallyingProcedure(ctx)

return GenesisState{
initalProposalID,
StartingProposalID: startingProposalID,
DepositProcedure: depositProcedure,
VotingProcedure: votingProcedure,
TallyingProcedure: tallyingProcedure,
}
}
6 changes: 3 additions & 3 deletions x/gov/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func EndBlocker(ctx sdk.Context, keeper Keeper) (resTags sdk.Tags, nonVotingVals
activeProposal := keeper.ActiveProposalQueuePop(ctx)

proposalStartBlock := activeProposal.GetVotingStartBlock()
votingPeriod := keeper.GetVotingProcedure().VotingPeriod
votingPeriod := keeper.GetVotingProcedure(ctx).VotingPeriod
if ctx.BlockHeight() < proposalStartBlock+votingPeriod {
continue
}
Expand All @@ -144,7 +144,7 @@ func EndBlocker(ctx sdk.Context, keeper Keeper) (resTags sdk.Tags, nonVotingVals
return resTags, nonVotingVals
}
func shouldPopInactiveProposalQueue(ctx sdk.Context, keeper Keeper) bool {
depositProcedure := keeper.GetDepositProcedure()
depositProcedure := keeper.GetDepositProcedure(ctx)
peekProposal := keeper.InactiveProposalQueuePeek(ctx)

if peekProposal == nil {
Expand All @@ -158,7 +158,7 @@ func shouldPopInactiveProposalQueue(ctx sdk.Context, keeper Keeper) bool {
}

func shouldPopActiveProposalQueue(ctx sdk.Context, keeper Keeper) bool {
votingProcedure := keeper.GetVotingProcedure()
votingProcedure := keeper.GetVotingProcedure(ctx)
peekProposal := keeper.ActiveProposalQueuePeek(ctx)

if peekProposal == nil {
Expand Down
Loading