error returning epoch hooks

CHANGELOG.md

@@ -106,6 +106,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Improvements
 * [#2214](https://github.com/osmosis-labs/osmosis/pull/2214) Speedup epoch distribution, superfluid component
 * [#2515](https://github.com/osmosis-labs/osmosis/pull/2515) Emit events from functions implementing epoch hooks' `panicCatchingEpochHook` cacheCtx
+* [#2526](https://github.com/osmosis-labs/osmosis/pull/2526) EpochHooks interface methods (and hence modules implementing the hooks) return error instead of panic
 
 ### SDK Upgrades
 * [#2245](https://github.com/osmosis-labs/osmosis/pull/2244) Upgrade SDK with the following major changes:

x/epochs/keeper/abci.go

@@ -43,7 +43,8 @@ func (k Keeper) BeginBlocker(ctx sdk.Context) {
 					sdk.NewAttribute(types.AttributeEpochNumber, fmt.Sprintf("%d", epochInfo.CurrentEpoch)),
 				),
 			)
-			k.AfterEpochEnd(ctx, epochInfo.Identifier, epochInfo.CurrentEpoch)
+			// Error is not handled as AfterEpochEnd Hooks use osmoutils.ApplyFuncIfNoError()
+			_ = k.AfterEpochEnd(ctx, epochInfo.Identifier, epochInfo.CurrentEpoch)
 			epochInfo.CurrentEpoch += 1
 			epochInfo.CurrentEpochStartTime = epochInfo.CurrentEpochStartTime.Add(epochInfo.Duration)
 			logger.Info(fmt.Sprintf("Starting epoch with identifier %s epoch number %d", epochInfo.Identifier, epochInfo.CurrentEpoch))
@@ -58,7 +59,8 @@ func (k Keeper) BeginBlocker(ctx sdk.Context) {
 			),
 		)
 		k.setEpochInfo(ctx, epochInfo)
-		k.BeforeEpochStart(ctx, epochInfo.Identifier, epochInfo.CurrentEpoch)
+		// Error is not handled as BeforeEpochStart Hooks use osmoutils.ApplyFuncIfNoError()
+		_ = k.BeforeEpochStart(ctx, epochInfo.Identifier, epochInfo.CurrentEpoch)
 
 		return false
 	})

x/epochs/keeper/hooks.go

@@ -5,11 +5,11 @@ import (
 )
 
 // AfterEpochEnd gets called at the end of the epoch, end of epoch is the timestamp of first block produced after epoch duration.
-func (k Keeper) AfterEpochEnd(ctx sdk.Context, identifier string, epochNumber int64) {
-	k.hooks.AfterEpochEnd(ctx, identifier, epochNumber)
+func (k Keeper) AfterEpochEnd(ctx sdk.Context, identifier string, epochNumber int64) error {
+	return k.hooks.AfterEpochEnd(ctx, identifier, epochNumber)
 }
 
 // BeforeEpochStart new epoch is next block of epoch end block
-func (k Keeper) BeforeEpochStart(ctx sdk.Context, identifier string, epochNumber int64) {
-	k.hooks.BeforeEpochStart(ctx, identifier, epochNumber)
+func (k Keeper) BeforeEpochStart(ctx sdk.Context, identifier string, epochNumber int64) error {
+	return k.hooks.BeforeEpochStart(ctx, identifier, epochNumber)
 }

x/epochs/types/hooks.go

@@ -10,9 +10,9 @@ import (
 
 type EpochHooks interface {
 	// the first block whose timestamp is after the duration is counted as the end of the epoch
-	AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64)
+	AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error
 	// new epoch is next block of epoch end block
-	BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64)
+	BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error
 }
 
 var _ EpochHooks = MultiEpochHooks{}
@@ -25,28 +25,29 @@ func NewMultiEpochHooks(hooks ...EpochHooks) MultiEpochHooks {
 }
 
 // AfterEpochEnd is called when epoch is going to be ended, epochNumber is the number of epoch that is ending.
-func (h MultiEpochHooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (h MultiEpochHooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	for i := range h {
 		panicCatchingEpochHook(ctx, h[i].AfterEpochEnd, epochIdentifier, epochNumber)
 	}
+	return nil
 }
 
 // BeforeEpochStart is called when epoch is going to be started, epochNumber is the number of epoch that is starting.
-func (h MultiEpochHooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (h MultiEpochHooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	for i := range h {
 		panicCatchingEpochHook(ctx, h[i].BeforeEpochStart, epochIdentifier, epochNumber)
 	}
+	return nil
 }
 
 func panicCatchingEpochHook(
 	ctx sdk.Context,
-	hookFn func(ctx sdk.Context, epochIdentifier string, epochNumber int64),
+	hookFn func(ctx sdk.Context, epochIdentifier string, epochNumber int64) error,
 	epochIdentifier string,
 	epochNumber int64,
 ) {
 	wrappedHookFn := func(ctx sdk.Context) error {
-		hookFn(ctx, epochIdentifier, epochNumber)
-		return nil
+		return hookFn(ctx, epochIdentifier, epochNumber)
 	}
 	// TODO: Thread info for which hook this is, may be dependent on larger hook system refactoring
 	err := osmoutils.ApplyFuncIfNoError(ctx, wrappedHookFn)

x/epochs/types/hooks_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/stretchr/testify/suite"
 
 	"github.com/osmosis-labs/osmosis/v11/app/apptesting"
@@ -43,49 +44,65 @@ func dummyBeforeEpochStartEvent(epochIdentifier string, epochNumber int64) sdk.E
 	)
 }
 
+var dummyErr = errors.New("9", 9, "dummyError")
+
 // dummyEpochHook is a struct satisfying the epoch hook interface,
 // that maintains a counter for how many times its been succesfully called,
 // and a boolean for whether it should panic during its execution.
 type dummyEpochHook struct {
 	successCounter int
 	shouldPanic    bool
+	shouldError    bool
 }
 
-func (hook *dummyEpochHook) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (hook *dummyEpochHook) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	if hook.shouldPanic {
 		panic("dummyEpochHook is panicking")
 	}
+	if hook.shouldError {
+		return dummyErr
+	}
 	hook.successCounter += 1
 	ctx.EventManager().EmitEvent(dummyAfterEpochEndEvent(epochIdentifier, epochNumber))
+	return nil
 }
 
-func (hook *dummyEpochHook) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (hook *dummyEpochHook) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	if hook.shouldPanic {
 		panic("dummyEpochHook is panicking")
 	}
+	if hook.shouldError {
+		return dummyErr
+	}
 	hook.successCounter += 1
 	ctx.EventManager().EmitEvent(dummyBeforeEpochStartEvent(epochIdentifier, epochNumber))
+	return nil
 
 }
 
 func (hook *dummyEpochHook) Clone() *dummyEpochHook {
-	newHook := dummyEpochHook{shouldPanic: hook.shouldPanic, successCounter: hook.successCounter}
+	newHook := dummyEpochHook{shouldPanic: hook.shouldPanic, successCounter: hook.successCounter, shouldError: hook.shouldError}
 	return &newHook
 }
 
 var _ types.EpochHooks = &dummyEpochHook{}
 
-func (suite *KeeperTestSuite) TestHooksPanicRecoveryAndEvents() {
+func (suite *KeeperTestSuite) TestHooksPanicRecovery() {
 	panicHook := dummyEpochHook{shouldPanic: true}
 	noPanicHook := dummyEpochHook{shouldPanic: false}
-	simpleHooks := []dummyEpochHook{panicHook, noPanicHook}
+	errorHook := dummyEpochHook{shouldError: true}
+	noErrorHook := dummyEpochHook{shouldError: false} //same as nopanic
+	simpleHooks := []dummyEpochHook{panicHook, noPanicHook, errorHook, noErrorHook}
 
 	tests := []struct {
 		hooks                 []dummyEpochHook
 		expectedCounterValues []int
+		lenEvents             int
 	}{
-		{[]dummyEpochHook{noPanicHook}, []int{1}},
-		{simpleHooks, []int{0, 1}},
+		{[]dummyEpochHook{noPanicHook}, []int{1}, 1},
+		{[]dummyEpochHook{panicHook}, []int{0}, 0},
+		{[]dummyEpochHook{errorHook}, []int{0}, 0},
+		{simpleHooks, []int{0, 1, 0, 1}, 2},
 	}
 
 	for tcIndex, tc := range tests {
@@ -98,16 +115,24 @@ func (suite *KeeperTestSuite) TestHooksPanicRecoveryAndEvents() {
 			}
 
 			hooks := types.NewMultiEpochHooks(hookRefs...)
+
+			events := func(epochID string, epochNumber int64, dummyEvent func(id string, number int64) sdk.Event) sdk.Events {
+				evts := make(sdk.Events, tc.lenEvents)
+				for i := 0; i < tc.lenEvents; i++ {
+					evts[i] = dummyEvent(epochID, epochNumber)
+				}
+				return evts
+			}
+
 			suite.NotPanics(func() {
 				if epochActionSelector == 0 {
 					hooks.BeforeEpochStart(suite.Ctx, "id", 0)
-					suite.Require().Equal(suite.Ctx.EventManager().Events(), sdk.Events{dummyBeforeEpochStartEvent("id", 0)},
+					suite.Require().Equal(events("id", 0, dummyBeforeEpochStartEvent), suite.Ctx.EventManager().Events(),
 						"test case index %d, before epoch event check", tcIndex)
 				} else if epochActionSelector == 1 {
 					hooks.AfterEpochEnd(suite.Ctx, "id", 0)
-					suite.Require().Equal(suite.Ctx.EventManager().Events(), sdk.Events{dummyAfterEpochEndEvent("id", 0)},
+					suite.Require().Equal(events("id", 0, dummyAfterEpochEndEvent), suite.Ctx.EventManager().Events(),
 						"test case index %d, after epoch event check", tcIndex)
-
 				}
 			})
 

x/incentives/keeper/hooks.go

@@ -9,19 +9,20 @@ import (
 )
 
 // BeforeEpochStart is the epoch start hook.
-func (k Keeper) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (k Keeper) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return nil
 }
 
 // AfterEpochEnd is the epoch end hook.
-func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	params := k.GetParams(ctx)
 	if epochIdentifier == params.DistrEpochIdentifier {
 		// begin distribution if it's start time
 		gauges := k.GetUpcomingGauges(ctx)
 		for _, gauge := range gauges {
 			if !ctx.BlockTime().Before(gauge.StartTime) {
 				if err := k.moveUpcomingGaugeToActiveGauge(ctx, gauge); err != nil {
-					panic(err)
+					return err
 				}
 			}
 		}
@@ -41,9 +42,10 @@ func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumb
 		}
 		_, err := k.Distribute(ctx, distrGauges)
 		if err != nil {
-			panic(err)
+			return err
 		}
 	}
+	return nil
 }
 
 // ___________________________________________________________________________________________________
@@ -61,11 +63,11 @@ func (k Keeper) Hooks() Hooks {
 }
 
 // BeforeEpochStart is the epoch start hook.
-func (h Hooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
-	h.k.BeforeEpochStart(ctx, epochIdentifier, epochNumber)
+func (h Hooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return h.k.BeforeEpochStart(ctx, epochIdentifier, epochNumber)
 }
 
 // AfterEpochEnd is the epoch end hook.
-func (h Hooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
-	h.k.AfterEpochEnd(ctx, epochIdentifier, epochNumber)
+func (h Hooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return h.k.AfterEpochEnd(ctx, epochIdentifier, epochNumber)
 }

x/mint/keeper/hooks.go

@@ -11,8 +11,9 @@ import (
 )
 
 // BeforeEpochStart is a hook which is executed before the start of an epoch. It is a no-op for mint module.
-func (k Keeper) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (k Keeper) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	// no-op
+	return nil
 }
 
 // AfterEpochEnd is a hook which is executed after the end of an epoch.
@@ -22,13 +23,13 @@ func (k Keeper) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochN
 // For an attempt to mint to occur:
 // - given epochIdentifier must be equal to the mint epoch identifier set via parameters.
 // - given epochNumber must be greater than or equal to the mint start epoch set via parameters.
-func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	params := k.GetParams(ctx)
 
 	if epochIdentifier == params.EpochIdentifier {
 		// not distribute rewards if it's not time yet for rewards distribution
 		if epochNumber < params.MintingRewardsDistributionStartEpoch {
-			return
+			return nil
 		} else if epochNumber == params.MintingRewardsDistributionStartEpoch {
 			k.setLastReductionEpochNum(ctx, epochNumber)
 		}
@@ -54,13 +55,13 @@ func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumb
 		// We over-allocate by the developer vesting portion, and burn this later
 		err := k.mintCoins(ctx, mintedCoins)
 		if err != nil {
-			panic(err)
+			return err
 		}
 
 		// send the minted coins to the fee collector account
 		err = k.DistributeMintedCoin(ctx, mintedCoin)
 		if err != nil {
-			panic(err)
+			return err
 		}
 
 		if mintedCoin.Amount.IsInt64() {
@@ -76,6 +77,7 @@ func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumb
 			),
 		)
 	}
+	return nil
 }
 
 // ___________________________________________________________________________________________________
@@ -93,10 +95,10 @@ func (k Keeper) Hooks() Hooks {
 }
 
 // epochs hooks.
-func (h Hooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
-	h.k.BeforeEpochStart(ctx, epochIdentifier, epochNumber)
+func (h Hooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return h.k.BeforeEpochStart(ctx, epochIdentifier, epochNumber)
 }
 
-func (h Hooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
-	h.k.AfterEpochEnd(ctx, epochIdentifier, epochNumber)
+func (h Hooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return h.k.AfterEpochEnd(ctx, epochIdentifier, epochNumber)
 }

x/mint/keeper/hooks_test.go

@@ -395,7 +395,10 @@ func (suite *KeeperTestSuite) TestAfterEpochEnd() {
 
 			osmoassert.ConditionalPanic(suite.T(), tc.expectedPanic, func() {
 				// System under test.
-				mintKeeper.AfterEpochEnd(ctx, defaultEpochIdentifier, tc.hookArgEpochNum)
+				err := mintKeeper.AfterEpochEnd(ctx, defaultEpochIdentifier, tc.hookArgEpochNum)
+				if err != nil {
+					panic(err)
+				}
 			})
 
 			// If panics, the behavior is undefined.

x/superfluid/keeper/epoch.go

@@ -14,7 +14,8 @@ import (
 	"github.com/osmosis-labs/osmosis/v11/x/superfluid/types"
 )
 
-func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, _ int64) {
+func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, _ int64) error {
+	return nil
 }
 
 func (k Keeper) AfterEpochStartBeginBlock(ctx sdk.Context) {

x/superfluid/keeper/hooks.go

@@ -24,11 +24,12 @@ func (k Keeper) Hooks() Hooks {
 
 // epochs hooks
 // Don't do anything pre epoch start.
-func (h Hooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (h Hooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return nil
 }
 
-func (h Hooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
-	h.k.AfterEpochEnd(ctx, epochIdentifier, epochNumber)
+func (h Hooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return h.k.AfterEpochEnd(ctx, epochIdentifier, epochNumber)
 }
 
 // lockup hooks

x/twap/listeners.go

@@ -18,15 +18,18 @@ func (k Keeper) EpochHooks() epochtypes.EpochHooks {
 	return &epochhook{k}
 }
 
-func (hook *epochhook) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) {
+func (hook *epochhook) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	if epochIdentifier == hook.k.PruneEpochIdentifier(ctx) {
 		if err := hook.k.pruneRecords(ctx); err != nil {
 			ctx.Logger().Error("Error pruning old twaps at the epoch end", err)
 		}
 	}
+	return nil
 }
 
-func (hook *epochhook) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {}
+func (hook *epochhook) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
+	return nil
+}
 
 type gammhook struct {
 	k Keeper