Add structs for Upgrades and Forks structs (#1379)

Closes: #1375 ## What is the purpose of the change Simplifies a lot of the upgrades management logic / boilerplate. The goal is that every new upgrade should from the app.go level, just require a one line update. And all the complexity would otherwise be held to within each upgrades module. This PR is marked as draft until Changelog update + docs update to the `app/upgrades` README. ## Brief change log * Introduces an Upgrade and Fork struct * Adapt existing upgrades to use it ## Testing and Verifying This should be covered by the existing test for the v4 upgrade, which tests that upgrade handler execution happens. ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? no - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? Needs to be Done - How is the feature or change documented? (not applicable / specification (`x/<module>/spec/`) / [Osmosis docs repo](https://github.com/osmosis-labs/docs) / not documented)
osmosis-labs · May 4, 2022 · b71507c · b71507c
1 parent 18a3bb3
commit b71507c
Show file tree

Hide file tree

Showing 18 changed files with 198 additions and 117 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -50,6 +50,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * 
 ### Minor improvements & Bug Fixes
 
+* [#1379](https://github.com/osmosis-labs/osmosis/pull/1379) Introduce `Upgrade` and `Fork` structs, to simplify upgrade logic.
 * [#1363](https://github.com/osmosis-labs/osmosis/pull/1363) Switch e2e test setup to create genesis and configs via Dockertest
 * [#1335](https://github.com/osmosis-labs/osmosis/pull/1335) Add utility for deriving total orderings from partial orderings.
 * [#1308](https://github.com/osmosis-labs/osmosis/pull/1308) Make panics inside of epochs no longer chain halt by default.

diff --git a/app/app.go b/app/app.go
@@ -28,7 +28,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/server/api"
 	"github.com/cosmos/cosmos-sdk/server/config"
 	servertypes "github.com/cosmos/cosmos-sdk/server/types"
-	store "github.com/cosmos/cosmos-sdk/store/types"
 	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/module"
@@ -42,12 +41,14 @@ import (
 
 	"github.com/osmosis-labs/osmosis/v7/app/keepers"
 	appparams "github.com/osmosis-labs/osmosis/v7/app/params"
+	"github.com/osmosis-labs/osmosis/v7/app/upgrades"
+	v3 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v3"
 	v4 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v4"
 	v5 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v5"
+	v6 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v6"
 	v7 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v7"
 	v8 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v8"
 	_ "github.com/osmosis-labs/osmosis/v7/client/docs/statik"
-	superfluidtypes "github.com/osmosis-labs/osmosis/v7/x/superfluid/types"
 )
 
 const appName = "OsmosisApp"
@@ -83,6 +84,9 @@ var (
 	EmptyWasmOpts []wasm.Option
 
 	_ App = (*OsmosisApp)(nil)
+
+	Upgrades = []upgrades.Upgrade{v4.Upgrade, v5.Upgrade, v7.Upgrade, v8.Upgrade}
+	Forks    = []upgrades.Fork{v3.Fork, v6.Fork}
 )
 
 // GetWasmEnabledProposals parses the WasmProposalsEnabled and
@@ -392,6 +396,7 @@ func (app *OsmosisApp) RegisterTendermintService(clientCtx client.Context) {
 	tmservice.RegisterTendermintService(app.BaseApp.GRPCQueryRouter(), clientCtx, app.interfaceRegistry)
 }
 
+// configure store loader that checks if version == upgradeHeight and applies store upgrades
 func (app *OsmosisApp) setupUpgradeStoreLoaders() {
 	upgradeInfo, err := app.UpgradeKeeper.ReadUpgradeInfoFromDisk()
 	if err != nil {
@@ -402,71 +407,24 @@ func (app *OsmosisApp) setupUpgradeStoreLoaders() {
 		return
 	}
 
-	storeUpgrades := store.StoreUpgrades{}
-
-	if upgradeInfo.Name == v7.UpgradeName {
-		storeUpgrades = store.StoreUpgrades{
-			Added: []string{wasm.ModuleName, superfluidtypes.ModuleName},
-		}
-	}
-
-	if upgradeInfo.Name == v8.UpgradeName {
-		storeUpgrades = store.StoreUpgrades{
-			Deleted: []string{v8.ClaimsModuleName},
+	for _, upgrade := range Upgrades {
+		if upgradeInfo.Name == upgrade.UpgradeName {
+			app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &upgrade.StoreUpgrades))
 		}
 	}
-
-	// configure store loader that checks if version == upgradeHeight and applies store upgrades
-	app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
 }
 
 func (app *OsmosisApp) setupUpgradeHandlers() {
-	// this configures a no-op upgrade handler for the v4 upgrade,
-	// which improves the lockup module's store management.
-	app.UpgradeKeeper.SetUpgradeHandler(
-		v4.UpgradeName,
-		v4.CreateUpgradeHandler(
-			app.mm,
-			app.configurator,
-			*app.BankKeeper,
-			app.DistrKeeper,
-			app.GAMMKeeper,
-		),
-	)
-
-	app.UpgradeKeeper.SetUpgradeHandler(
-		v5.UpgradeName,
-		v5.CreateUpgradeHandler(
-			app.mm,
-			app.configurator,
-			&app.IBCKeeper.ConnectionKeeper,
-			app.TxFeesKeeper,
-			app.GAMMKeeper,
-			app.StakingKeeper,
-		),
-	)
-
-	app.UpgradeKeeper.SetUpgradeHandler(
-		v7.UpgradeName,
-		v7.CreateUpgradeHandler(
-			app.mm,
-			app.configurator,
-			app.WasmKeeper,
-			app.SuperfluidKeeper,
-			app.EpochsKeeper,
-			app.LockupKeeper,
-			app.MintKeeper,
-			app.AccountKeeper,
-		),
-	)
-
-	app.UpgradeKeeper.SetUpgradeHandler(
-		v8.UpgradeName,
-		v8.CreateUpgradeHandler(
-			app.mm,
-			app.configurator,
-		),
-	)
+	for _, upgrade := range Upgrades {
+		app.UpgradeKeeper.SetUpgradeHandler(
+			upgrade.UpgradeName,
+			upgrade.CreateUpgradeHandler(
+				app.mm,
+				app.configurator,
+				&app.AppKeepers,
+			),
+		)
+	}
 }
 
 // RegisterSwaggerAPI registers swagger route with API Server.

diff --git a/app/forks.go b/app/forks.go
@@ -2,22 +2,14 @@ package app
 
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
-
-	v3 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v3"
-	v6 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v6"
 )
 
 // BeginBlockForks is intended to be ran in.
 func BeginBlockForks(ctx sdk.Context, app *OsmosisApp) {
-	switch ctx.BlockHeight() {
-	case v3.UpgradeHeight:
-		v3.RunForkLogic(ctx, app.GovKeeper, app.StakingKeeper)
-
-	case v6.UpgradeHeight:
-		v6.RunForkLogic(ctx)
-
-	default:
-		// do nothing
-		return
+	for _, fork := range Forks {
+		if ctx.BlockHeight() == fork.UpgradeHeight {
+			fork.BeginForkLogic(ctx, &app.AppKeepers)
+			return
+		}
 	}
 }
diff --git a/app/keepers/keepers.go b/app/keepers/keepers.go
@@ -339,7 +339,6 @@ func (appKeepers *AppKeepers) InitNormalKeepers(
 	appKeepers.IBCKeeper.SetRouter(ibcRouter)
 
 	// register the proposal types
-	// TODO: This appears to be missing tx fees proposal type
 	govRouter := govtypes.NewRouter()
 	govRouter.AddRoute(govtypes.RouterKey, govtypes.ProposalHandler).
 		AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(*appKeepers.ParamsKeeper)).

diff --git a/app/upgrades/README.md b/app/upgrades/README.md
@@ -1,6 +1,10 @@
 # Osmosis Upgrades
 
-This folder contains logic for every osmosis upgrade. (Both state migrations, and hard forks)
+This folder contains sub-folders for every osmosis upgrade. (Both state migrations, and hard forks)
+It also defines upgrade & hard fork structs, that each upgrade implements. 
+These then get included in the application app.go to run the upgrade.
+
+## Version History
 
 * v1 - Initial version
 * v3 - short blurb on prop19 and the fork
@@ -10,4 +14,31 @@ This folder contains logic for every osmosis upgrade. (Both state migrations, an
 * v7 - Carbon State migration
 * v8 - Nitrogen State migration
 
-## TODO: Make a fork-upgrade struct and a state-migration upgrade struct
+## Upgrade types 
+
+There are two upgrade types exposed, `Upgrade` and `Fork`. 
+An `Upgrade` defines an upgrade that is to be acted upon by state migrations from the SDK `x/upgrade` module.
+A `Fork` defines a hard fork that changes some logic at a block height. 
+If the goal is to have a new binary be compatible with the old binary prior to the upgrade height,
+as is the case for all osmosis `Fork`s, then all logic changes must be height-gated or in the `BeginForkLogic` code.
+
+```go
+type Upgrade struct {
+	// Upgrade version name, for the upgrade handler, e.g. `v7`
+	UpgradeName string
+	// Function that creates an upgrade handler
+	CreateUpgradeHandler func(mm *module.Manager, configurator module.Configurator, keepers *keepers.AppKeepers) upgradetypes.UpgradeHandler
+	// Store upgrades, should be used for any new modules introduced, new modules deleted, or store names renamed.
+	StoreUpgrades store.StoreUpgrades
+}
+
+type Fork struct {
+	// Upgrade version name, for the upgrade handler, e.g. `v7`
+	UpgradeName string
+	// height the upgrade occurs at
+	UpgradeHeight int64
+
+	// Function that runs some custom state transition code at the beginning of a fork.
+	BeginForkLogic func(ctx sdk.Context, keepers *keepers.AppKeepers)
+}
+```
diff --git a/app/upgrades/types.go b/app/upgrades/types.go
@@ -0,0 +1,40 @@
+package upgrades
+
+import (
+	"github.com/cosmos/cosmos-sdk/types/module"
+
+	store "github.com/cosmos/cosmos-sdk/store/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
+
+	"github.com/osmosis-labs/osmosis/v7/app/keepers"
+)
+
+// Upgrade defines a struct containing necessary fields that a SoftwareUpgradeProposal
+// must have written, in order for the state migration to go smoothly.
+// An upgrade must implement this struct, and then set it in the app.go.
+// The app.go will then define the handler.
+type Upgrade struct {
+	// Upgrade version name, for the upgrade handler, e.g. `v7`
+	UpgradeName string
+
+	// Function that creates an upgrade handler
+	CreateUpgradeHandler func(mm *module.Manager, configurator module.Configurator, keepers *keepers.AppKeepers) upgradetypes.UpgradeHandler
+	// Store upgrades, should be used for any new modules introduced, new modules deleted, or store names renamed.
+	StoreUpgrades store.StoreUpgrades
+}
+
+// Fork defines a struct containing the requisite fields for a non-software upgrade proposal
+// Hard Fork at a given height to implement.
+// There is one time code that can be added for the start of the Fork, in `BeginForkLogic`.
+// Any other change in the code should be height-gated, if the goal is to have old and new binaries
+// to be compatible prior to the upgrade height.
+type Fork struct {
+	// Upgrade version name, for the upgrade handler, e.g. `v7`
+	UpgradeName string
+	// height the upgrade occurs at
+	UpgradeHeight int64
+
+	// Function that runs some custom state transition code at the beginning of a fork.
+	BeginForkLogic func(ctx sdk.Context, keepers *keepers.AppKeepers)
+}
diff --git a/app/upgrades/v3/constants.go b/app/upgrades/v3/constants.go
@@ -1,5 +1,7 @@
 package v3
 
+import "github.com/osmosis-labs/osmosis/v7/app/upgrades"
+
 const (
 	// UpgradeName defines the on-chain upgrade name for the Osmosis v3 upgrade.
 	UpgradeName = "v3"
@@ -8,3 +10,9 @@ const (
 	// triggered.
 	UpgradeHeight = 712_000
 )
+
+var Fork = upgrades.Fork{
+	UpgradeName:    UpgradeName,
+	UpgradeHeight:  UpgradeHeight,
+	BeginForkLogic: RunForkLogic,
+}
diff --git a/app/upgrades/v3/forks.go b/app/upgrades/v3/forks.go
@@ -4,16 +4,18 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	govkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper"
 	stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper"
+
+	"github.com/osmosis-labs/osmosis/v7/app/keepers"
 )
 
 // RunForkLogic executes height-gated on-chain fork logic for the Osmosis v3
 // upgrade.
-func RunForkLogic(ctx sdk.Context, gov *govkeeper.Keeper, staking *stakingkeeper.Keeper) {
+func RunForkLogic(ctx sdk.Context, keepers *keepers.AppKeepers) {
 	ctx.Logger().Info("Applying Osmosis v3 upgrade." +
 		" Fixing governance deposit so proposals can be voted upon," +
 		" and fixing validator min commission rate.")
-	FixMinDepositDenom(ctx, gov)
-	FixMinCommisionRate(ctx, staking)
+	FixMinDepositDenom(ctx, keepers.GovKeeper)
+	FixMinCommisionRate(ctx, keepers.StakingKeeper)
 }
 
 // Fixes an error where minimum deposit was set to "500 osmo". This denom does

diff --git a/app/upgrades/v4/constants.go b/app/upgrades/v4/constants.go
@@ -1,4 +1,16 @@
 package v4
 
+import (
+	"github.com/osmosis-labs/osmosis/v7/app/upgrades"
+
+	store "github.com/cosmos/cosmos-sdk/store/types"
+)
+
 // UpgradeName defines the on-chain upgrade name for the Osmosis v4 upgrade.
 const UpgradeName = "v4"
+
+var Upgrade = upgrades.Upgrade{
+	UpgradeName:          UpgradeName,
+	CreateUpgradeHandler: CreateUpgradeHandler,
+	StoreUpgrades:        store.StoreUpgrades{},
+}
diff --git a/app/upgrades/v4/upgrades.go b/app/upgrades/v4/upgrades.go
@@ -3,11 +3,9 @@ package v4
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/module"
-	bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
-	distrkeeper "github.com/cosmos/cosmos-sdk/x/distribution/keeper"
 	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 
-	gammkeeper "github.com/osmosis-labs/osmosis/v7/x/gamm/keeper"
+	"github.com/osmosis-labs/osmosis/v7/app/keepers"
 	gammtypes "github.com/osmosis-labs/osmosis/v7/x/gamm/types"
 )
 
@@ -19,15 +17,13 @@ import (
 func CreateUpgradeHandler(
 	mm *module.Manager,
 	configurator module.Configurator,
-	bank bankkeeper.Keeper,
-	distr *distrkeeper.Keeper,
-	gamm *gammkeeper.Keeper,
+	keepers *keepers.AppKeepers,
 ) upgradetypes.UpgradeHandler {
 	return func(ctx sdk.Context, _plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
 		// configure upgrade for x/gamm module pool creation fee param
-		gamm.SetParams(ctx, gammtypes.NewParams(sdk.Coins{sdk.NewInt64Coin("uosmo", 1)})) // 1 uOSMO
+		keepers.GAMMKeeper.SetParams(ctx, gammtypes.NewParams(sdk.Coins{sdk.NewInt64Coin("uosmo", 1)})) // 1 uOSMO
 
-		Prop12(ctx, bank, distr)
+		Prop12(ctx, keepers.BankKeeper, keepers.DistrKeeper)
 
 		return vm, nil
 	}

diff --git a/app/upgrades/v5/constants.go b/app/upgrades/v5/constants.go
@@ -1,4 +1,16 @@
 package v5
 
+import (
+	"github.com/osmosis-labs/osmosis/v7/app/upgrades"
+
+	store "github.com/cosmos/cosmos-sdk/store/types"
+)
+
 // UpgradeName defines the on-chain upgrade name for the Osmosis v5 upgrade.
 const UpgradeName = "v5"
+
+var Upgrade = upgrades.Upgrade{
+	UpgradeName:          UpgradeName,
+	CreateUpgradeHandler: CreateUpgradeHandler,
+	StoreUpgrades:        store.StoreUpgrades{},
+}