Fix epochs modules tests

CHANGELOG.md

@@ -47,8 +47,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#1825](https://github.com/osmosis-labs/osmosis/pull/1825) Fixes Interchain Accounts (host side) by adding it to AppModuleBasics
 * [#1699](https://github.com/osmosis-labs/osmosis/pull/1699) Fixes bug in sig fig rounding on spot price queries for small values
 
-#### golang API breaks
+#### Golang API breaks
 
+* [#1893](https://github.com/osmosis-labs/osmosis/pull/1893) Change `EpochsKeeper.SetEpochInfo` to `AddEpochInfo`, which has more safety checks with it. (Makes it suitable to be called within upgrades)
 * [#1671](https://github.com/osmosis-labs/osmosis/pull/1671) Remove methods that constitute AppModuleSimulation APIs for several modules' AppModules, which implemented no-ops
 * [#1671](https://github.com/osmosis-labs/osmosis/pull/1671) Add hourly epochs to `x/epochs` DefaultGenesis.
 * [#1665](https://github.com/osmosis-labs/osmosis/pull/1665) Delete app/App interface, instead use simapp.App

go.mod

@@ -31,6 +31,8 @@ require (
 	mvdan.cc/gofumpt v0.3.1
 )
 
+require golang.org/x/exp v0.0.0-20220613132600-b0d781184e0d
+
 require (
 	4d63.com/gochecknoglobals v0.1.0 // indirect
 	filippo.io/edwards25519 v1.0.0-beta.2 // indirect

go.sum

@@ -1484,6 +1484,8 @@ golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u0
 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
 golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
 golang.org/x/exp v0.0.0-20200331195152-e8c3332aa8e5/go.mod h1:4M0jN8W1tt0AVLNr8HDosyJCDCDuyL9N9+3m7wDWgKw=
+golang.org/x/exp v0.0.0-20220613132600-b0d781184e0d h1:vtUKgx8dahOomfFzLREU8nSv25YHnTgLBn4rDnWZdU0=
+golang.org/x/exp v0.0.0-20220613132600-b0d781184e0d/go.mod h1:Kr81I6Kryrl9sr8s2FK3vxD90NdsKWRuOIl2O4CvYbA=
 golang.org/x/exp/typeparams v0.0.0-20220218215828-6cf2b201936e h1:qyrTQ++p1afMkO4DPEeLGq/3oTsdlvdH4vqZUBWzUKM=
 golang.org/x/exp/typeparams v0.0.0-20220218215828-6cf2b201936e/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk=
 golang.org/x/image v0.0.0-20180708004352-c73c2afc3b81/go.mod h1:ux5Hcp/YLpHSI86hEcLt0YII63i6oz57MZXIpbrjZUs=

osmoutils/slice_helper.go

@@ -0,0 +1,14 @@
+package osmoutils
+
+import (
+	"sort"
+
+	"golang.org/x/exp/constraints"
+)
+
+// SortSlice sorts a slice of type T elements that implement constraints.Ordered.
+func SortSlice[T constraints.Ordered](s []T) {
+	sort.Slice(s, func(i, j int) bool {
+		return s[i] < s[j]
+	})
+}

x/epochs/keeper/abci.go

@@ -57,7 +57,7 @@ func (k Keeper) BeginBlocker(ctx sdk.Context) {
 				sdk.NewAttribute(types.AttributeEpochStartTime, fmt.Sprintf("%d", epochInfo.CurrentEpochStartTime.Unix())),
 			),
 		)
-		k.SetEpochInfo(ctx, epochInfo)
+		k.setEpochInfo(ctx, epochInfo)
 		k.BeforeEpochStart(ctx, epochInfo.Identifier, epochInfo.CurrentEpoch)
 
 		return false

x/epochs/keeper/abci_test.go

@@ -10,146 +10,98 @@ import (
 	simapp "github.com/osmosis-labs/osmosis/v7/app"
 	"github.com/osmosis-labs/osmosis/v7/x/epochs/types"
 
-	sdk "github.com/cosmos/cosmos-sdk/types"
-)
-
-func TestEpochInfoChangesBeginBlockerAndInitGenesis(t *testing.T) {
-	var app *simapp.OsmosisApp
-	var ctx sdk.Context
-	var epochInfo types.EpochInfo
+	"golang.org/x/exp/maps"
 
-	now := time.Now()
+	"github.com/osmosis-labs/osmosis/v7/osmoutils"
+)
 
-	tests := []struct {
-		expCurrentEpochStartTime   time.Time
-		expCurrentEpochStartHeight int64
-		expCurrentEpoch            int64
-		expInitialEpochStartTime   time.Time
-		fn                         func()
+// This test is responsible for testing how epochs increment based off
+// of their initial conditions, and subsequent block height / times.
+//
+// TODO: Make a new test for init genesis logic
+func (suite KeeperTestSuite) TestEpochInfoBeginBlockChanges() {
+	block1Time := time.Unix(1656907200, 0).UTC()
+	// We run all tests with the same identifier and same duration
+	defaultIdentifier := "hourly"
+	defaultDuration := time.Hour
+	eps := time.Nanosecond
+
+	tests := map[string]struct {
+		// if identifier, duration is not set, we make it defaultIdentifier and defaultDuration.
+		// EpochCountingStarted, if unspecified, is inferred by CurrentEpoch == 0
+		// StartTime is inferred to be block1Time if left blank.
+		initialEpochInfo     types.EpochInfo
+		blockHeightTimePairs map[int]time.Time
+		expEpochInfo         types.EpochInfo
 	}{
-		{
-			// Only advance 2 seconds, do not increment epoch
-			expCurrentEpochStartHeight: 2,
-			expCurrentEpochStartTime:   now,
-			expCurrentEpoch:            1,
-			expInitialEpochStartTime:   now,
-			fn: func() {
-				ctx = ctx.WithBlockHeight(2).WithBlockTime(now.Add(time.Second))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				epochInfo = app.EpochsKeeper.GetEpochInfo(ctx, "monthly")
-			},
+		"First block running at exactly start time sets epoch tick": {
+			initialEpochInfo: types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
+			expEpochInfo:     types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time, CurrentEpochStartHeight: 1},
 		},
-		{
-			expCurrentEpochStartHeight: 2,
-			expCurrentEpochStartTime:   now,
-			expCurrentEpoch:            1,
-			expInitialEpochStartTime:   now,
-			fn: func() {
-				ctx = ctx.WithBlockHeight(2).WithBlockTime(now.Add(time.Second))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				epochInfo = app.EpochsKeeper.GetEpochInfo(ctx, "monthly")
-			},
+		"First block run sets start time, subsequent blocks within interval do not affect": {
+			initialEpochInfo:     types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
+			blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(time.Second), 3: block1Time.Add(time.Minute), 4: block1Time.Add(30 * time.Minute)},
+			expEpochInfo:         types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time, CurrentEpochStartHeight: 1},
 		},
-		{
-			expCurrentEpochStartHeight: 2,
-			expCurrentEpochStartTime:   now,
-			expCurrentEpoch:            1,
-			expInitialEpochStartTime:   now,
-			fn: func() {
-				ctx = ctx.WithBlockHeight(2).WithBlockTime(now.Add(time.Second))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				ctx = ctx.WithBlockHeight(3).WithBlockTime(now.Add(time.Hour * 24 * 31))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				epochInfo = app.EpochsKeeper.GetEpochInfo(ctx, "monthly")
-			},
+		"Second block at exactly interval later does not tick": {
+			initialEpochInfo:     types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
+			blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(time.Hour)},
+			expEpochInfo:         types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time, CurrentEpochStartHeight: 1},
 		},
-		// Test that incrementing _exactly_ 1 month increments the epoch count.
-		{
-			expCurrentEpochStartHeight: 3,
-			expCurrentEpochStartTime:   now.Add(time.Hour * 24 * 31),
-			expCurrentEpoch:            2,
-			expInitialEpochStartTime:   now,
-			fn: func() {
-				ctx = ctx.WithBlockHeight(2).WithBlockTime(now.Add(time.Second))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				ctx = ctx.WithBlockHeight(3).WithBlockTime(now.Add(time.Hour * 24 * 32))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				epochInfo = app.EpochsKeeper.GetEpochInfo(ctx, "monthly")
-			},
+		"Second block at interval + epsilon later does tick": {
+			initialEpochInfo:     types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
+			blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(time.Hour).Add(eps)},
+			expEpochInfo:         types.EpochInfo{StartTime: block1Time, CurrentEpoch: 2, CurrentEpochStartTime: block1Time.Add(time.Hour), CurrentEpochStartHeight: 2},
 		},
-		{
-			expCurrentEpochStartHeight: 3,
-			expCurrentEpochStartTime:   now.Add(time.Hour * 24 * 31),
-			expCurrentEpoch:            2,
-			expInitialEpochStartTime:   now,
-			fn: func() {
-				ctx = ctx.WithBlockHeight(2).WithBlockTime(now.Add(time.Second))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				ctx = ctx.WithBlockHeight(3).WithBlockTime(now.Add(time.Hour * 24 * 32))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				ctx.WithBlockHeight(4).WithBlockTime(now.Add(time.Hour * 24 * 33))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				epochInfo = app.EpochsKeeper.GetEpochInfo(ctx, "monthly")
-			},
+		"Downtime recovery (many intervals), first block causes 1 tick and sets current start time 1 interval ahead": {
+			initialEpochInfo:     types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
+			blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(24 * time.Hour)},
+			expEpochInfo:         types.EpochInfo{StartTime: block1Time, CurrentEpoch: 2, CurrentEpochStartTime: block1Time.Add(time.Hour), CurrentEpochStartHeight: 2},
 		},
-		{
-			expCurrentEpochStartHeight: 3,
-			expCurrentEpochStartTime:   now.Add(time.Hour * 24 * 31),
-			expCurrentEpoch:            2,
-			expInitialEpochStartTime:   now,
-			fn: func() {
-				ctx = ctx.WithBlockHeight(2).WithBlockTime(now.Add(time.Second))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				ctx = ctx.WithBlockHeight(3).WithBlockTime(now.Add(time.Hour * 24 * 32))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				numBlocksSinceStart, _ := app.EpochsKeeper.NumBlocksSinceEpochStart(ctx, "monthly")
-				require.Equal(t, int64(0), numBlocksSinceStart)
-				ctx = ctx.WithBlockHeight(4).WithBlockTime(now.Add(time.Hour * 24 * 33))
-				app.EpochsKeeper.BeginBlocker(ctx)
-				epochInfo = app.EpochsKeeper.GetEpochInfo(ctx, "monthly")
-				numBlocksSinceStart, _ = app.EpochsKeeper.NumBlocksSinceEpochStart(ctx, "monthly")
-				require.Equal(t, int64(1), numBlocksSinceStart)
-			},
+		"Downtime recovery (many intervals), second block is at tick 2, w/ start time 2 intervals ahead": {
+			initialEpochInfo:     types.EpochInfo{StartTime: block1Time, CurrentEpoch: 0, CurrentEpochStartTime: time.Time{}},
+			blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(24 * time.Hour), 3: block1Time.Add(24 * time.Hour).Add(eps)},
+			expEpochInfo:         types.EpochInfo{StartTime: block1Time, CurrentEpoch: 3, CurrentEpochStartTime: block1Time.Add(2 * time.Hour), CurrentEpochStartHeight: 3},
+		},
+		"Many blocks between first and second tick": {
+			initialEpochInfo:     types.EpochInfo{StartTime: block1Time, CurrentEpoch: 1, CurrentEpochStartTime: block1Time},
+			blockHeightTimePairs: map[int]time.Time{2: block1Time.Add(time.Second), 3: block1Time.Add(2 * time.Second), 4: block1Time.Add(time.Hour).Add(eps)},
+			expEpochInfo:         types.EpochInfo{StartTime: block1Time, CurrentEpoch: 2, CurrentEpochStartTime: block1Time.Add(time.Hour), CurrentEpochStartHeight: 4},
 		},
 	}
-
-	for _, test := range tests {
-		app = simapp.Setup(false)
-		ctx = app.BaseApp.NewContext(false, tmproto.Header{})
-
-		// On init genesis, default epochs information is set
-		// To check init genesis again, should make it fresh status
-		epochInfos := app.EpochsKeeper.AllEpochInfos(ctx)
-		for _, epochInfo := range epochInfos {
-			app.EpochsKeeper.DeleteEpochInfo(ctx, epochInfo.Identifier)
-		}
-
-		ctx = ctx.WithBlockHeight(1).WithBlockTime(now)
-		// check init genesis
-		app.EpochsKeeper.InitGenesis(ctx, types.GenesisState{
-			Epochs: []types.EpochInfo{
-				{
-					Identifier:              "monthly",
-					StartTime:               time.Time{},
-					Duration:                time.Hour * 24 * 31,
-					CurrentEpoch:            0,
-					CurrentEpochStartHeight: ctx.BlockHeight(),
-					CurrentEpochStartTime:   time.Time{},
-					EpochCountingStarted:    false,
-				},
-			},
+	for name, test := range tests {
+		suite.Run(name, func() {
+			suite.SetupTest()
+			suite.Ctx = suite.Ctx.WithBlockHeight(1).WithBlockTime(block1Time)
+			initialEpoch := initializeBlankEpochInfoFields(test.initialEpochInfo, defaultIdentifier, defaultDuration)
+			suite.App.EpochsKeeper.AddEpochInfo(suite.Ctx, initialEpoch)
+			suite.App.EpochsKeeper.BeginBlocker(suite.Ctx)
+
+			// get sorted heights
+			heights := maps.Keys(test.blockHeightTimePairs)
+			osmoutils.SortSlice(heights)
+			for _, h := range heights {
+				// for each height in order, run begin block
+				suite.Ctx = suite.Ctx.WithBlockHeight(int64(h)).WithBlockTime(test.blockHeightTimePairs[h])
+				suite.App.EpochsKeeper.BeginBlocker(suite.Ctx)
+			}
+			expEpoch := initializeBlankEpochInfoFields(test.expEpochInfo, initialEpoch.Identifier, initialEpoch.Duration)
+			actEpoch := suite.App.EpochsKeeper.GetEpochInfo(suite.Ctx, initialEpoch.Identifier)
+			suite.Require().Equal(expEpoch, actEpoch)
 		})
+	}
+}
 
-		test.fn()
-
-		require.Equal(t, epochInfo.Identifier, "monthly")
-		require.Equal(t, epochInfo.StartTime.UTC().String(), test.expInitialEpochStartTime.UTC().String())
-		require.Equal(t, epochInfo.Duration, time.Hour*24*31)
-		require.Equal(t, epochInfo.CurrentEpoch, test.expCurrentEpoch)
-		require.Equal(t, epochInfo.CurrentEpochStartHeight, test.expCurrentEpochStartHeight)
-		require.Equal(t, epochInfo.CurrentEpochStartTime.UTC().String(), test.expCurrentEpochStartTime.UTC().String())
-		require.Equal(t, epochInfo.EpochCountingStarted, true)
+// set identifier, duration and epochCountingStarted if blank in epoch
+func initializeBlankEpochInfoFields(epoch types.EpochInfo, identifier string, duration time.Duration) types.EpochInfo {
+	if epoch.Identifier == "" {
+		epoch.Identifier = identifier
+	}
+	if epoch.Duration == time.Duration(0) {
+		epoch.Duration = duration
 	}
+	epoch.EpochCountingStarted = (epoch.CurrentEpoch != 0)
+	return epoch
 }
 
 func TestEpochStartingOneMonthAfterInitGenesis(t *testing.T) {
@@ -212,45 +164,3 @@ func TestEpochStartingOneMonthAfterInitGenesis(t *testing.T) {
 	require.Equal(t, epochInfo.CurrentEpochStartTime.UTC().String(), now.Add(month).UTC().String())
 	require.Equal(t, epochInfo.EpochCountingStarted, true)
 }
-
-// This test ensures legacy EpochInfo messages will not throw errors via InitGenesis and BeginBlocker
-func TestLegacyEpochSerialization(t *testing.T) {
-	// Legacy Epoch Info message - without CurrentEpochStartHeight property
-	legacyEpochInfo := types.EpochInfo{
-		Identifier:            "monthly",
-		StartTime:             time.Time{},
-		Duration:              time.Hour * 24 * 31,
-		CurrentEpoch:          0,
-		CurrentEpochStartTime: time.Time{},
-		EpochCountingStarted:  false,
-	}
-
-	now := time.Now()
-	app := simapp.Setup(false)
-	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
-
-	// On init genesis, default epochs information is set
-	// To check init genesis again, should make it fresh status
-	epochInfos := app.EpochsKeeper.AllEpochInfos(ctx)
-	for _, epochInfo := range epochInfos {
-		app.EpochsKeeper.DeleteEpochInfo(ctx, epochInfo.Identifier)
-	}
-
-	ctx = ctx.WithBlockHeight(1).WithBlockTime(now)
-
-	// check init genesis
-	app.EpochsKeeper.InitGenesis(ctx, types.GenesisState{
-		Epochs: []types.EpochInfo{legacyEpochInfo},
-	})
-
-	// Do not increment epoch
-	ctx = ctx.WithBlockHeight(2).WithBlockTime(now.Add(time.Second))
-	app.EpochsKeeper.BeginBlocker(ctx)
-
-	// Increment epoch
-	ctx = ctx.WithBlockHeight(3).WithBlockTime(now.Add(time.Hour * 24 * 32))
-	app.EpochsKeeper.BeginBlocker(ctx)
-	epochInfo := app.EpochsKeeper.GetEpochInfo(ctx, "monthly")
-
-	require.NotEqual(t, epochInfo.CurrentEpochStartHeight, int64(0))
-}

x/epochs/keeper/epoch.go

@@ -2,8 +2,10 @@ package keeper
 
 import (
 	"fmt"
+	"time"
 
 	"github.com/gogo/protobuf/proto"
+
 	"github.com/osmosis-labs/osmosis/v7/x/epochs/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -24,8 +26,28 @@ func (k Keeper) GetEpochInfo(ctx sdk.Context, identifier string) types.EpochInfo
 	return epoch
 }
 
-// SetEpochInfo set epoch info.
-func (k Keeper) SetEpochInfo(ctx sdk.Context, epoch types.EpochInfo) {
+// AddEpochInfo adds a new epoch info.
+func (k Keeper) AddEpochInfo(ctx sdk.Context, epoch types.EpochInfo) error {
+	err := epoch.Validate()
+	if err != nil {
+		return err
+	}
+	// Check if identifier already exists
+	if (k.GetEpochInfo(ctx, epoch.Identifier) != types.EpochInfo{}) {
+		return fmt.Errorf("epoch with identifier %s already exists", epoch.Identifier)
+	}
+
+	// Initialize empty and default epoch values
+	if epoch.StartTime.Equal(time.Time{}) {
+		epoch.StartTime = ctx.BlockTime()
+	}
+	epoch.CurrentEpochStartHeight = ctx.BlockHeight()
+	k.setEpochInfo(ctx, epoch)
+	return nil
+}
+
+// setEpochInfo set epoch info.
+func (k Keeper) setEpochInfo(ctx sdk.Context, epoch types.EpochInfo) {
 	store := ctx.KVStore(k.storeKey)
 	value, err := proto.Marshal(&epoch)
 	if err != nil {

x/epochs/keeper/epoch_test.go

@@ -10,9 +10,13 @@ func (suite *KeeperTestSuite) TestEpochLifeCycle() {
 	suite.SetupTest()
 
 	epochInfo := types.NewGenesisEpochInfo("monthly", time.Hour*24*30)
-	suite.App.EpochsKeeper.SetEpochInfo(suite.Ctx, epochInfo)
+	suite.App.EpochsKeeper.AddEpochInfo(suite.Ctx, epochInfo)
 	epochInfoSaved := suite.App.EpochsKeeper.GetEpochInfo(suite.Ctx, "monthly")
-	suite.Require().Equal(epochInfo, epochInfoSaved)
+	// setup expected epoch info
+	expectedEpochInfo := epochInfo
+	expectedEpochInfo.StartTime = suite.Ctx.BlockTime()
+	expectedEpochInfo.CurrentEpochStartHeight = suite.Ctx.BlockHeight()
+	suite.Require().Equal(expectedEpochInfo, epochInfoSaved)
 
 	allEpochs := suite.App.EpochsKeeper.AllEpochInfos(suite.Ctx)
 	suite.Require().Len(allEpochs, 4)