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

[CORE-850] Update default values for new Gov parameters #34

Merged
merged 5 commits into from
Jan 26, 2024
Merged

[CORE-850] Update default values for new Gov parameters #34

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48ff5c6
Update default gov parameters; update migration logic
teddyding Jan 11, 2024
73d3cd5
lint`
teddyding Jan 11, 2024
abcb593
nit
teddyding Jan 11, 2024
305dce4
comments
teddyding Jan 12, 2024
d1702e0
Address comments
teddyding Jan 26, 2024
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
2 changes: 1 addition & 1 deletion x/gov/migrations/v5/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func MigrateStore(ctx sdk.Context, storeService corestoretypes.KVStoreService, c
}
Copy link
Author

Choose a reason for hiding this comment

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

Note: this MigrationStore is registered here, which is run in RunMigrations during the v_4_0_0_0 upgrade.

Copy link

Choose a reason for hiding this comment

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

can we make sure that this gets tested during our qualification in staging + testnet?


defaultParams := govv1.DefaultParams()
params.ExpeditedMinDeposit = defaultParams.ExpeditedMinDeposit
params.ExpeditedMinDeposit = params.MinDeposit // Use regular `min_deposit` as `expedited_min_deposit`
Copy link

Choose a reason for hiding this comment

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

how come we only update the min deposit but not the expedited voting period and threshold?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

params.ExpeditedVotingPeriod = defaultParams.ExpeditedVotingPeriod
ttl33 marked this conversation as resolved.
Show resolved Hide resolved
params.ExpeditedThreshold = defaultParams.ExpeditedThreshold
params.ProposalCancelRatio = defaultParams.ProposalCancelRatio
Expand Down
3 changes: 2 additions & 1 deletion x/gov/migrations/v5/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ func TestMigrateStore(t *testing.T) {
bz = store.Get(v4.ParamsKey)
require.NoError(t, cdc.Unmarshal(bz, &params))
require.NotNil(t, params)
require.Equal(t, v1.DefaultParams().ExpeditedMinDeposit, params.ExpeditedMinDeposit)
// Expect ExpeditedMinDeposit to equal previous MinDeposit after migraiton.
require.Equal(t, params.MinDeposit, params.ExpeditedMinDeposit)
require.Equal(t, v1.DefaultParams().ExpeditedThreshold, params.ExpeditedThreshold)
require.Equal(t, v1.DefaultParams().ExpeditedVotingPeriod, params.ExpeditedVotingPeriod)
Copy link

Choose a reason for hiding this comment

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

shouldn't some of these values change?

Copy link
Author

Choose a reason for hiding this comment

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

These lines weren't updated since both v1.DefaultParams().XXXX and params.ExpeditedVotingPeriod were updated. I changed them to literals to be explicit

require.Equal(t, v1.DefaultParams().MinDepositRatio, params.MinDepositRatio)
Expand Down
19 changes: 13 additions & 6 deletions x/gov/types/v1/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,28 @@

// Default period for deposits & voting
const (
DefaultPeriod time.Duration = time.Hour * 24 * 2 // 2 days
DefaultPeriod time.Duration = time.Hour * 24 * 2 // 2 days
// (New default value for v0.50 migration) 24 hours voting period for expedited proposals.
DefaultExpeditedPeriod time.Duration = time.Hour * 24 * 1 // 1 day
DefaultMinExpeditedDepositTokensRatio = 5
)

// Default governance params
var (
DefaultMinDepositTokens = sdkmath.NewInt(10000000)
DefaultMinDepositTokens = sdkmath.NewInt(10000000)
// During v0.50 migration, this default value is overwritten with existing value of `MinDeposit`.
DefaultMinExpeditedDepositTokens = DefaultMinDepositTokens.Mul(sdkmath.NewInt(DefaultMinExpeditedDepositTokensRatio))
DefaultQuorum = sdkmath.LegacyNewDecWithPrec(334, 3)
DefaultThreshold = sdkmath.LegacyNewDecWithPrec(5, 1)
DefaultExpeditedThreshold = sdkmath.LegacyNewDecWithPrec(667, 3)
DefaultVetoThreshold = sdkmath.LegacyNewDecWithPrec(334, 3)
DefaultMinInitialDepositRatio = sdkmath.LegacyZeroDec()
DefaultProposalCancelRatio = sdkmath.LegacyMustNewDecFromStr("0.5")
// (New default value for v0.50 migration) 75% of Yes votes required for an expedited proposal to pass.
ttl33 marked this conversation as resolved.
Show resolved Hide resolved
DefaultExpeditedThreshold = sdkmath.LegacyNewDecWithPrec(75, 2)
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.

Check failure on line 32 in x/gov/types/v1/params.go

View workflow job for this annotation

GitHub Actions / Analyze 

`cancelled` is a misspelling of `canceled` (misspell)
Copy link
Author

Choose a reason for hiding this comment

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

Disabling MsgCancelProposal will be done in the follow-up PR that points v4-chain to this commit.

DefaultProposalCancelRatio = sdkmath.LegacyMustNewDecFromStr("1.0")
// (New default value for v0.50 migration) 100% of deposit is burned if the proposal is cancelled.

Check failure on line 34 in x/gov/types/v1/params.go

View workflow job for this annotation

GitHub Actions / Analyze 

`cancelled` is a misspelling of `canceled` (misspell)
// Also, `MsgCancelProposal` is disabled in application.
DefaultProposalCancelDestAddress = ""
DefaultBurnProposalPrevote = false // set to false to replicate behavior of when this change was made (0.47)
DefaultBurnVoteQuorom = false // set to false to replicate behavior of when this change was made (0.47)
Expand Down
Loading