refactor: expedited proposals parameter migrations #303

xBalbinus · 2022-08-01T18:27:20Z

Closes: #2249

What is the purpose of the change

For expedited proposals we added 2 new parameters:

MinExpeditedDeposit
ExpeditedVotingPeriod
ExpeditedThreshold

However, these parameters have not been set / migrated to. We need to implement the migrations to set the parameters to the desired values according to: https://wallet.keplr.app/chains/osmosis/proposals/278 and its respective common wealth thread.

Brief Changelog

expedited proposal parameter migrations are implemented in the SDK fork
parameters are set to the desired values from the gov prop
the upgraded sdk change is integrated into Osmosis
e2e upgrade tests continue to pass

Testing and Verifying

This change is already covered by existing tests, such as the CI tests as well as tests written in gov/legacy/v4 module

Added unit test that validates params settings and storage.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

p0mvn

Great work, had some comments. Please take a look

p0mvn · 2022-08-03T14:49:27Z

x/gov/legacy/v4/export_test.go

@@ -0,0 +1,6 @@
+package v4
+
+var MinInitialDepositRatio = minInitialDepositRatio


let's group these vars and add whitespace at the end of the file please

p0mvn · 2022-08-03T14:49:58Z

x/gov/legacy/v4/store.go

+var minInitialDepositRatio = sdk.NewDec(25).Quo(sdk.NewDec(100))
+var minExpeditedDeposit = sdk.NewCoins(sdk.NewCoin("osmo", sdk.NewInt(5000)))
+var expeditedVotingPeriod = time.Duration(time.Hour * 24)
+var expeditedThreshold = sdk.NewDec(2).Quo(sdk.NewDec(3))


please group the vars:

var ( ... )

p0mvn · 2022-08-03T14:50:08Z

x/gov/legacy/v4/store.go

+var expeditedVotingPeriod = time.Duration(time.Hour * 24)
+var expeditedThreshold = sdk.NewDec(2).Quo(sdk.NewDec(3))
+
+// MigrateStore performs in-place store migrations for consensus version 3


Suggested change

// MigrateStore performs in-place store migrations for consensus version 3

// MigrateStore performs in-place store migrations for consensus version 4

p0mvn · 2022-08-03T14:50:24Z

x/gov/legacy/v4/store.go

+// Please note that this is the first version that switches from using
+// SDK versioning (v043 etc) for package names to consensus versioning
+// of the gov module.


Suggested change

// Please note that this is the first version that switches from using

// SDK versioning (v043 etc) for package names to consensus versioning

// of the gov module.

p0mvn · 2022-08-03T14:50:42Z

x/gov/legacy/v4/store.go

+// of the gov module.
+// The migration includes:
+//
+// - Setting the minimum deposit param in the paramstore.


Let's update the comment please

p0mvn · 2022-08-03T14:54:32Z

x/gov/legacy/v4/store.go

+
+	//Set depositParams
+	paramstore.Get(ctx, types.ParamStoreKeyDepositParams, &depositParams)
+	depositParams.MinInitialDepositRatio = minInitialDepositRatio


This should have been set during the previous migration (v3)

Suggested change

depositParams.MinInitialDepositRatio = minInitialDepositRatio

p0mvn · 2022-08-03T15:01:38Z

x/gov/legacy/v4/store.go

+// increased to 5000 OSMO. The proposal will have 24 hours to achieve
+// a two-thirds majority of all staked OSMO voting power voting YES.
+var minInitialDepositRatio = sdk.NewDec(25).Quo(sdk.NewDec(100))
+var minExpeditedDeposit = sdk.NewCoins(sdk.NewCoin("osmo", sdk.NewInt(5000)))


Hmm this is not ideal that we have this coupling in the sdk on osmosis by having to hardcode the osmo denom here.

This is a flaw in the original design though and regular deposits have the same problem. I think it's okay to hard code it here, just commenting to keep this issue in mind

p0mvn · 2022-08-03T15:02:25Z

x/gov/legacy/v4/store.go

+// increased to 5000 OSMO. The proposal will have 24 hours to achieve
+// a two-thirds majority of all staked OSMO voting power voting YES.
+var minInitialDepositRatio = sdk.NewDec(25).Quo(sdk.NewDec(100))
+var minExpeditedDeposit = sdk.NewCoins(sdk.NewCoin("osmo", sdk.NewInt(5000)))


Suggested change

var minExpeditedDeposit = sdk.NewCoins(sdk.NewCoin("osmo", sdk.NewInt(5000)))

var minExpeditedDeposit = sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(5000 * 1_000_000)))

p0mvn · 2022-08-03T15:02:31Z

x/gov/legacy/v4/store.go

+// If expedited, the deposit to enter voting period will be
+// increased to 5000 OSMO. The proposal will have 24 hours to achieve
+// a two-thirds majority of all staked OSMO voting power voting YES.
+var minInitialDepositRatio = sdk.NewDec(25).Quo(sdk.NewDec(100))


Suggested change

var minInitialDepositRatio = sdk.NewDec(25).Quo(sdk.NewDec(100))

p0mvn · 2022-08-03T15:03:26Z

x/gov/legacy/v4/store_test.go

+
+	// We assume that all deposit params are set besides the MinInitialDepositRatio
+	originalDepositParams := types.DefaultDepositParams()
+	originalDepositParams.MinInitialDepositRatio = sdk.ZeroDec()


let's remove everything related to min initial deposit ratio please

p0mvn · 2022-08-08T14:30:29Z

x/gov/keeper/migrations.go

@@ -25,3 +26,8 @@ func (m Migrator) Migrate1to2(ctx sdk.Context) error {
 func (m Migrator) Migrate2to3(ctx sdk.Context) error {
 	return v3.MigrateStore(ctx, m.keeper.paramSpace)
 }
+
+// Migrate2to3 migrates from version 3 to 4.


Suggested change

// Migrate2to3 migrates from version 3 to 4.

// Migrate3to4 migrates from version 3 to 4.

p0mvn · 2022-08-08T14:31:41Z

x/gov/legacy/v4/store.go

+	var depositParams types.DepositParams
+	var votingParams types.VotingParams
+	var tallyParams types.TallyParams


Let's group vars please

…smos-sdk into xiangan/refactor

x/gov/legacy/v4/store.go

migration to v4

86c3685

github-actions bot added the C:x/gov label Aug 1, 2022

xBalbinus mentioned this pull request Aug 1, 2022

refactor(x/gov): expedited proposals parameter migrations osmosis-labs/osmosis#2249

Closed

added value & migrated params

841be43

xBalbinus marked this pull request as ready for review August 1, 2022 20:25

xBalbinus requested a review from a team August 1, 2022 20:25

xBalbinus marked this pull request as draft August 1, 2022 20:25

xBalbinus changed the title ~~migration to v4~~ [refactor] expedited proposals parameter migrations Aug 2, 2022

Test additions and CI fixes

fee14de

xBalbinus changed the title ~~[refactor] expedited proposals parameter migrations~~ refactor: expedited proposals parameter migrations Aug 2, 2022

xBalbinus added 2 commits August 2, 2022 09:16

additional fixes

02a8e38

table mismatch fixes

a984784

xBalbinus marked this pull request as ready for review August 2, 2022 14:18

p0mvn mentioned this pull request Aug 3, 2022

refactor: expedited proposals parameter migrations osmosis-labs/osmosis#2274

Closed

p0mvn reviewed Aug 3, 2022

View reviewed changes

xBalbinus and others added 2 commits August 5, 2022 10:01

feedback implemented

31b3483

Merge branch 'osmosis-main' into xiangan/refactor

8159cbc

p0mvn approved these changes Aug 8, 2022

View reviewed changes

xBalbinus added 2 commits August 8, 2022 11:19

feedback implemented

f3261c1

Merge branch 'xiangan/refactor' of https://github.com/osmosis-labs/co…

8c45a57

…smos-sdk into xiangan/refactor

czarcas7ic approved these changes Aug 8, 2022

View reviewed changes

x/gov/legacy/v4/store.go Outdated Show resolved Hide resolved

x/gov/legacy/v4/store.go Outdated Show resolved Hide resolved

x/gov/legacy/v4/store.go Outdated Show resolved Hide resolved

Apply suggestions from code review

7e92fe4

czarcas7ic added the A:Automerge label Aug 8, 2022

czarcas7ic merged commit 02273b8 into osmosis-main Aug 8, 2022

czarcas7ic deleted the xiangan/refactor branch August 8, 2022 17:36

czarcas7ic removed the A:Automerge label Aug 8, 2022

czarcas7ic restored the xiangan/refactor branch August 8, 2022 17:51

czarcas7ic mentioned this pull request Aug 9, 2022

e2e: expedited proposal test osmosis-labs/osmosis#2341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: expedited proposals parameter migrations #303

refactor: expedited proposals parameter migrations #303

xBalbinus commented Aug 1, 2022 •

edited

Loading

p0mvn left a comment

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 3, 2022

p0mvn Aug 8, 2022

p0mvn Aug 8, 2022

		@@ -0,0 +1,6 @@
		package v4

		var MinInitialDepositRatio = minInitialDepositRatio

	// MigrateStore performs in-place store migrations for consensus version 3
	// MigrateStore performs in-place store migrations for consensus version 4

	// Please note that this is the first version that switches from using
	// SDK versioning (v043 etc) for package names to consensus versioning
	// of the gov module.

	var minExpeditedDeposit = sdk.NewCoins(sdk.NewCoin("osmo", sdk.NewInt(5000)))
	var minExpeditedDeposit = sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(5000 * 1_000_000)))

	// Migrate2to3 migrates from version 3 to 4.
	// Migrate3to4 migrates from version 3 to 4.

refactor: expedited proposals parameter migrations #303

refactor: expedited proposals parameter migrations #303

Conversation

xBalbinus commented Aug 1, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xBalbinus commented Aug 1, 2022 •

edited

Loading