[CORE-850] Update default values for new Gov parameters #34
Conversation
|
@teddyding your pull request is missing a changelog! |
teddyding
changed the title
| @@ -36,7 +36,7 @@ func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, c | |||
| } | |||
There was a problem hiding this comment.
Note: this MigrationStore is registered here, which is run in RunMigrations during the v_4_0_0_0 upgrade.
There was a problem hiding this comment.
can we make sure that this gets tested during our qualification in staging + testnet?
x/gov/types/v1/params.go
Outdated
| DefaultVetoThreshold = sdkmath.LegacyNewDecWithPrec(334, 3) | ||
| DefaultMinInitialDepositRatio = sdkmath.LegacyZeroDec() | ||
| // (New default value for v0.50 migration) 100% of deposit will not be returned to the depositors, | ||
| // if the proposal is cancelled. Also, `MsgCancelProposal` is disabled in application. |
There was a problem hiding this comment.
Disabling MsgCancelProposal will be done in the follow-up PR that points v4-chain to this commit.
| @@ -36,7 +36,7 @@ func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, c | |||
| } | |||
There was a problem hiding this comment.
can we make sure that this gets tested during our qualification in staging + testnet?
x/gov/migrations/v5/store.go
Outdated
| @@ -36,7 +36,7 @@ func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, c | |||
| } | |||
|
|
|||
| defaultParams := govv1.DefaultParams() | |||
| params.ExpeditedMinDeposit = defaultParams.ExpeditedMinDeposit | |||
| params.ExpeditedMinDeposit = params.MinDeposit // Use regular `min_deposit` as `expedited_min_deposit` | |||
There was a problem hiding this comment.
how come we only update the min deposit but not the expedited voting period and threshold?
There was a problem hiding this comment.
We update these other params by though updated value of defaultParams := govv1.DefaultParams(). I added a comment to highlight
Only ExpeditedMinDeposit needs to be explicitly updated here since we need the min_deposit value from state.
There was a problem hiding this comment.
makes sense. can we update the comment to highlight "need to use min_deposit from state to use the same denom"?
the current comment doesn't explain why we are doing this
x/gov/migrations/v5/store_test.go
Outdated
| require.Equal(t, v1.DefaultParams().ExpeditedThreshold, params.ExpeditedThreshold) | ||
| require.Equal(t, v1.DefaultParams().ExpeditedVotingPeriod, params.ExpeditedVotingPeriod) |
There was a problem hiding this comment.
These lines weren't updated since both v1.DefaultParams().XXXX and params.ExpeditedVotingPeriod were updated. I changed them to literals to be explicit
x/gov/migrations/v5/store.go
Outdated
| @@ -36,7 +36,7 @@ func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, c | |||
| } | |||
|
|
|||
| defaultParams := govv1.DefaultParams() | |||
| params.ExpeditedMinDeposit = defaultParams.ExpeditedMinDeposit | |||
| params.ExpeditedMinDeposit = params.MinDeposit // Use regular `min_deposit` as `expedited_min_deposit` | |||
There was a problem hiding this comment.
makes sense. can we update the comment to highlight "need to use min_deposit from state to use the same denom"?
the current comment doesn't explain why we are doing this
Description
Update default values for new gov parameters in v0.50 migration. See values and rationale here.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change