chore: x/mint genesis - audit, clean up and tests (backport #1872) (#…

…2144)

Co-authored-by: Roman <[email protected]>

app/upgrades/v7/upgrades.go

@@ -9,7 +9,6 @@ import (
 
 	"github.com/osmosis-labs/osmosis/v10/app/keepers"
 	lockupkeeper "github.com/osmosis-labs/osmosis/v10/x/lockup/keeper"
-	mintkeeper "github.com/osmosis-labs/osmosis/v10/x/mint/keeper"
 	superfluidtypes "github.com/osmosis-labs/osmosis/v10/x/superfluid/types"
 )
 
@@ -54,7 +53,9 @@ func CreateUpgradeHandler(
 		keepers.SuperfluidKeeper.AddNewSuperfluidAsset(ctx, superfluidAsset)
 
 		// Set the supply offset from the developer vesting account
-		mintkeeper.SetInitialSupplyOffsetDuringMigration(ctx, *keepers.MintKeeper)
+		if err := keepers.MintKeeper.SetInitialSupplyOffsetDuringMigration(ctx); err != nil {
+			panic(err)
+		}
 
 		return newVM, err
 	}

x/mint/keeper/export_test.go

@@ -0,0 +1,11 @@
+package keeper
+
+const (
+	DeveloperVestingAmount = developerVestingAmount
+)
+
+var (
+	ErrAmountCannotBeNilOrZero               = errAmountCannotBeNilOrZero
+	ErrDevVestingModuleAccountAlreadyCreated = errDevVestingModuleAccountAlreadyCreated
+	ErrDevVestingModuleAccountNotCreated     = errDevVestingModuleAccountNotCreated
+)

x/mint/keeper/genesis.go

@@ -6,17 +6,32 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
+const developerVestingAmount = 225_000_000_000_000
+
 // InitGenesis new mint genesis.
-func (k Keeper) InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.BankKeeper, data *types.GenesisState) {
+func (k Keeper) InitGenesis(ctx sdk.Context, data *types.GenesisState) {
+	if data == nil {
+		panic("nil mint genesis state")
+	}
+
 	data.Minter.EpochProvisions = data.Params.GenesisEpochProvisions
 	k.SetMinter(ctx, data.Minter)
 	k.SetParams(ctx, data.Params)
 
-	if !ak.HasAccount(ctx, ak.GetModuleAddress(types.ModuleName)) {
-		ak.GetModuleAccount(ctx, types.ModuleName)
-		totalDeveloperVestingCoins := sdk.NewCoin(data.Params.MintDenom, sdk.NewInt(225_000_000_000_000))
-		k.CreateDeveloperVestingModuleAccount(ctx, totalDeveloperVestingCoins)
-		bk.AddSupplyOffset(ctx, data.Params.MintDenom, sdk.NewInt(225_000_000_000_000).Neg())
+	// The call to GetModuleAccount creates a module account if it does not exist.
+	k.accountKeeper.GetModuleAccount(ctx, types.ModuleName)
+
+	// The account should be exported in the ExportGenesis of the
+	// x/auth SDK module. Therefore, we check for existence here
+	// to avoid overwriting pre-existing genesis account data.
+	if !k.accountKeeper.HasAccount(ctx, k.accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)) {
+		totalDeveloperVestingCoins := sdk.NewCoin(data.Params.MintDenom, sdk.NewInt(developerVestingAmount))
+
+		if err := k.CreateDeveloperVestingModuleAccount(ctx, totalDeveloperVestingCoins); err != nil {
+			panic(err)
+		}
+
+		k.bankKeeper.AddSupplyOffset(ctx, data.Params.MintDenom, sdk.NewInt(developerVestingAmount).Neg())
 	}
 
 	k.SetLastHalvenEpochNum(ctx, data.HalvenStartedEpoch)
@@ -26,6 +41,11 @@ func (k Keeper) InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.Ba
 func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
 	minter := k.GetMinter(ctx)
 	params := k.GetParams(ctx)
+
+	if params.WeightedDeveloperRewardsReceivers == nil {
+		params.WeightedDeveloperRewardsReceivers = make([]types.WeightedAddress, 0)
+	}
+
 	lastHalvenEpoch := k.GetLastHalvenEpochNum(ctx)
 	return types.NewGenesisState(minter, params, lastHalvenEpoch)
 }

x/mint/keeper/genesis_test.go

@@ -3,27 +3,186 @@ package keeper_test
 import (
 	"testing"
 
-	"github.com/stretchr/testify/require"
-	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
+	"github.com/stretchr/testify/suite"
 
-	simapp "github.com/osmosis-labs/osmosis/v10/app"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+
+	"github.com/osmosis-labs/osmosis/v10/x/mint/keeper"
 	"github.com/osmosis-labs/osmosis/v10/x/mint/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
-func TestMintInitGenesis(t *testing.T) {
-	app := simapp.Setup(false)
-	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
+var customGenesis = types.NewGenesisState(
+	types.NewMinter(sdk.ZeroDec()),
+	types.NewParams(
+		"uosmo",                  // denom
+		sdk.NewDec(200),          // epoch provisions
+		"year",                   // epoch identifier
+		sdk.NewDecWithPrec(5, 1), // reduction factor
+		5,                        // reduction perion in epochs
+		types.DistributionProportions{
+			Staking:          sdk.NewDecWithPrec(25, 2),
+			PoolIncentives:   sdk.NewDecWithPrec(25, 2),
+			DeveloperRewards: sdk.NewDecWithPrec(25, 2),
+			CommunityPool:    sdk.NewDecWithPrec(25, 2),
+		},
+		[]types.WeightedAddress{
+			{
+				Address: "osmo14kjcwdwcqsujkdt8n5qwpd8x8ty2rys5rjrdjj",
+				Weight:  sdk.NewDecWithPrec(6, 1),
+			},
+			{
+				Address: "osmo1gw445ta0aqn26suz2rg3tkqfpxnq2hs224d7gq",
+				Weight:  sdk.NewDecWithPrec(4, 1),
+			},
+		},
+		2), // minting reward distribution start epoch
+	3) // halven started epoch
+
+func TestMintGenesisTestSuite(t *testing.T) {
+	suite.Run(t, new(KeeperTestSuite))
+}
+
+// TestMintInitGenesis tests that genesis is initialized correctly
+// with different parameters and state.
+func (suite *KeeperTestSuite) TestMintInitGenesis() {
+	testCases := map[string]struct {
+		mintGenesis                     *types.GenesisState
+		mintDenom                       string
+		ctxHeight                       int64
+		isDeveloperModuleAccountCreated bool
+
+		expectPanic             bool
+		expectedEpochProvisions sdk.Dec
+		// Deltas represent by how much a certain paramets
+		// has changed after calling InitGenesis()
+		expectedSupplyOffsetDelta           sdk.Int
+		expectedSupplyWithOffsetDelta       sdk.Int
+		expectedDeveloperVestingAmountDelta sdk.Int
+		expectedHalvenStartedEpoch          int64
+	}{
+		"default genesis - developer module account is not created prior to InitGenesis() - created during the call": {
+			mintGenesis: types.DefaultGenesisState(),
+			mintDenom:   sdk.DefaultBondDenom,
+
+			expectedEpochProvisions:             types.DefaultGenesisState().Params.GenesisEpochProvisions,
+			expectedSupplyOffsetDelta:           sdk.NewInt(keeper.DeveloperVestingAmount).Neg(),
+			expectedSupplyWithOffsetDelta:       sdk.ZeroInt(),
+			expectedDeveloperVestingAmountDelta: sdk.NewInt(keeper.DeveloperVestingAmount),
+		},
+		"default genesis - developer module account is created prior to InitGenesis() - not created during the call": {
+			mintGenesis:                     types.DefaultGenesisState(),
+			mintDenom:                       sdk.DefaultBondDenom,
+			isDeveloperModuleAccountCreated: true,
+
+			expectedEpochProvisions:             types.DefaultGenesisState().Params.GenesisEpochProvisions,
+			expectedSupplyOffsetDelta:           sdk.ZeroInt(),
+			expectedSupplyWithOffsetDelta:       sdk.ZeroInt(),
+			expectedDeveloperVestingAmountDelta: sdk.ZeroInt(),
+		},
+		"custom genesis": {
+			mintGenesis: customGenesis,
+			mintDenom:   "uosmo",
+
+			expectedEpochProvisions:             sdk.NewDec(200),
+			expectedSupplyOffsetDelta:           sdk.NewInt(keeper.DeveloperVestingAmount).Neg(),
+			expectedSupplyWithOffsetDelta:       sdk.ZeroInt(),
+			expectedDeveloperVestingAmountDelta: sdk.NewInt(keeper.DeveloperVestingAmount),
+			expectedHalvenStartedEpoch:          3,
+		},
+		"nil genesis state - panic": {
+			mintDenom:   sdk.DefaultBondDenom,
+			expectPanic: true,
+		},
+	}
+
+	for name, tc := range testCases {
+		suite.Run(name, func() {
+			// Setup.
+			suite.setupDeveloperVestingModuleAccountTest(tc.ctxHeight, tc.isDeveloperModuleAccountCreated)
+			ctx := suite.Ctx
+			accountKeeper := suite.App.AccountKeeper
+			bankKeeper := suite.App.BankKeeper
+			mintKeeper := suite.App.MintKeeper
+
+			developerAccount := accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)
+
+			originalSupplyOffset := bankKeeper.GetSupplyOffset(ctx, tc.mintDenom)
+			originalSupplyWithOffset := bankKeeper.GetSupplyWithOffset(ctx, tc.mintDenom)
+			originalVestingCoins := bankKeeper.GetBalance(ctx, developerAccount, tc.mintDenom)
+
+			// Test.
+			if tc.expectPanic {
+				suite.Panics(func() {
+					mintKeeper.InitGenesis(ctx, tc.mintGenesis)
+				})
+				return
+			}
+
+			suite.NotPanics(func() {
+				mintKeeper.InitGenesis(ctx, tc.mintGenesis)
+			})
+
+			// Assertions.
+
+			// Module account was created.
+			acc := accountKeeper.GetAccount(ctx, authtypes.NewModuleAddress(types.ModuleName))
+			suite.NotNil(acc)
+
+			// Epoch provisions are set to genesis epoch provisions from params.
+			actualEpochProvisions := mintKeeper.GetMinter(ctx).EpochProvisions
+			suite.Equal(tc.expectedEpochProvisions, actualEpochProvisions)
+
+			// Supply offset is applied to genesis supply.
+			actualSupplyOffset := bankKeeper.GetSupplyOffset(ctx, tc.mintDenom)
+			expectedSupplyOffset := tc.expectedSupplyOffsetDelta.Add(originalSupplyOffset)
+			suite.Equal(expectedSupplyOffset, actualSupplyOffset)
+
+			// Supply with offset is as expected.
+			actualSupplyWithOffset := bankKeeper.GetSupplyWithOffset(ctx, tc.mintDenom).Amount
+			expectedSupplyWithOffset := tc.expectedSupplyWithOffsetDelta.Add(originalSupplyWithOffset.Amount)
+			suite.Equal(expectedSupplyWithOffset.Int64(), actualSupplyWithOffset.Int64())
+
+			// Developer vesting account has the desired amount of tokens.
+			actualVestingCoins := bankKeeper.GetBalance(ctx, developerAccount, tc.mintDenom)
+			expectedDeveloperVestingAmount := tc.expectedDeveloperVestingAmountDelta.Add(originalVestingCoins.Amount)
+			suite.Equal(expectedDeveloperVestingAmount.Int64(), actualVestingCoins.Amount.Int64())
+
+			// Last halven epoch num is set to 0.
+			suite.Equal(tc.expectedHalvenStartedEpoch, mintKeeper.GetLastHalvenEpochNum(ctx))
+		})
+	}
+}
+
+// TestMintExportGenesis tests that genesis is exported correctly.
+// It first initializes genesis to the expected value. Then, attempts
+// to export it. Lastly, compares exported to the expected.
+func (suite *KeeperTestSuite) TestMintExportGenesis() {
+	testCases := map[string]struct {
+		expectedGenesis *types.GenesisState
+	}{
+		"default genesis": {
+			expectedGenesis: types.DefaultGenesisState(),
+		},
+		"custom genesis": {
+			expectedGenesis: customGenesis,
+		},
+	}
+
+	for name, tc := range testCases {
+		suite.Run(name, func() {
+			// Setup.
+			app := suite.App
+			ctx := suite.Ctx
 
-	validateGenesis := types.ValidateGenesis(*types.DefaultGenesisState())
-	require.NoError(t, validateGenesis)
+			app.MintKeeper.InitGenesis(ctx, tc.expectedGenesis)
 
-	developerAccount := app.AccountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)
-	initialVestingCoins := app.BankKeeper.GetBalance(ctx, developerAccount, sdk.DefaultBondDenom)
+			// Test.
+			actualGenesis := app.MintKeeper.ExportGenesis(ctx)
 
-	expectedVestingCoins, ok := sdk.NewIntFromString("225000000000000")
-	require.True(t, ok)
-	require.Equal(t, expectedVestingCoins, initialVestingCoins.Amount)
-	require.Equal(t, int64(0), app.MintKeeper.GetLastHalvenEpochNum(ctx))
+			// Assertions.
+			suite.Equal(tc.expectedGenesis, actualGenesis)
+		})
+	}
 }

x/mint/keeper/hooks_test.go

@@ -38,10 +38,6 @@ func TestEndOfEpochMintedCoinDistribution(t *testing.T) {
 	}
 	app.MintKeeper.SetParams(ctx, mintParams)
 
-	// setup developer rewards account
-	app.MintKeeper.CreateDeveloperVestingModuleAccount(
-		ctx, sdk.NewCoin("stake", sdk.NewInt(156*500000*2)))
-
 	height := int64(1)
 	lastHalvenPeriod := app.MintKeeper.GetLastHalvenEpochNum(ctx)
 	// correct rewards
@@ -121,10 +117,6 @@ func TestMintedCoinDistributionWhenDevRewardsAddressEmpty(t *testing.T) {
 	params := app.IncentivesKeeper.GetParams(ctx)
 	futureCtx := ctx.WithBlockTime(time.Now().Add(time.Minute))
 
-	// setup developer rewards account
-	app.MintKeeper.CreateDeveloperVestingModuleAccount(
-		ctx, sdk.NewCoin("stake", sdk.NewInt(156*500000*2)))
-
 	height := int64(1)
 	lastHalvenPeriod := app.MintKeeper.GetLastHalvenEpochNum(ctx)
 	// correct rewards

x/mint/keeper/keeper.go

@@ -1,6 +1,9 @@
 package keeper
 
 import (
+	"errors"
+	"fmt"
+
 	"github.com/tendermint/tendermint/libs/log"
 
 	"github.com/osmosis-labs/osmosis/v10/x/mint/types"
@@ -12,6 +15,12 @@ import (
 	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
 )
 
+var (
+	errAmountCannotBeNilOrZero               = errors.New("amount cannot be nil or zero")
+	errDevVestingModuleAccountAlreadyCreated = fmt.Errorf("%s module account already exists", types.DeveloperVestingModuleAcctName)
+	errDevVestingModuleAccountNotCreated     = fmt.Errorf("%s module account does not exist", types.DeveloperVestingModuleAcctName)
+)
+
 // Keeper of the mint store.
 type Keeper struct {
 	cdc              codec.BinaryCodec
@@ -54,25 +63,46 @@ func NewKeeper(
 }
 
 // SetInitialSupplyOffsetDuringMigration sets the supply offset based on the balance of the
-// Develop rVesting Module Account.  It should only be called one time during the initial
-// migration to v7.
-func SetInitialSupplyOffsetDuringMigration(ctx sdk.Context, k Keeper) {
+// developer vesting module account. CreateDeveloperVestingModuleAccount must be called
+// prior to calling this method. That is, developer vesting module account must exist when
+// SetInitialSupplyOffsetDuringMigration is called. Also, SetInitialSupplyOffsetDuringMigration
+// should only be called one time during the initial migration to v7. This is done so because
+// we would like to ensure that unvested developer tokens are not returned as part of the supply
+// queries. The method returns an error if current height in ctx is greater than the v7 upgrade height.
+func (k Keeper) SetInitialSupplyOffsetDuringMigration(ctx sdk.Context) error {
+	if !k.accountKeeper.HasAccount(ctx, k.accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)) {
+		return errDevVestingModuleAccountNotCreated
+	}
+
 	moduleAccBalance := k.bankKeeper.GetBalance(ctx, k.accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName), k.GetParams(ctx).MintDenom)
 	k.bankKeeper.AddSupplyOffset(ctx, moduleAccBalance.Denom, moduleAccBalance.Amount.Neg())
+	return nil
 }
 
-// CreateDeveloperVestingModuleAccount creates the module account for developer vesting.
-// Should only be called in initial genesis creation, never again.
-func (k Keeper) CreateDeveloperVestingModuleAccount(ctx sdk.Context, amount sdk.Coin) {
+// CreateDeveloperVestingModuleAccount creates the developer vesting module account
+// and mints amount of tokens to it.
+// Should only be called during the initial genesis creation, never again. Returns nil on success.
+// Returns error in the following cases:
+// - amount is nil or zero.
+// - if ctx has block height greater than 0.
+// - developer vesting module account is already created prior to calling this method.
+func (k Keeper) CreateDeveloperVestingModuleAccount(ctx sdk.Context, amount sdk.Coin) error {
+	if amount.IsNil() || amount.Amount.IsZero() {
+		return errAmountCannotBeNilOrZero
+	}
+	if k.accountKeeper.HasAccount(ctx, k.accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)) {
+		return errDevVestingModuleAccountAlreadyCreated
+	}
+
 	moduleAcc := authtypes.NewEmptyModuleAccount(
 		types.DeveloperVestingModuleAcctName, authtypes.Minter)
-
 	k.accountKeeper.SetModuleAccount(ctx, moduleAcc)
 
 	err := k.bankKeeper.MintCoins(ctx, types.DeveloperVestingModuleAcctName, sdk.NewCoins(amount))
 	if err != nil {
-		panic(err)
+		return err
 	}
+	return nil
 }
 
 // _____________________________________________________________________