error returning epoch hooks (#2526)

* add error to epoch hooks.

* twap module implementing epoch hooks return error

* fix tests.

* fix imports.

* add entry to CHANGELOG.md

* ignore the error in epoch hooks.go, instead of in abci.go

* rename expectedPanic to expectedError and refactor.

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/hooks.go

@@ -6,10 +6,12 @@ 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)
+	// Error is not handled as AfterEpochEnd Hooks use osmoutils.ApplyFuncIfNoError()
+	_ = 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)
+	// Error is not handled as BeforeEpochStart Hooks use osmoutils.ApplyFuncIfNoError()
+	_ = 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

@@ -93,7 +93,7 @@ func (suite *KeeperTestSuite) TestAfterEpochEnd() {
 		// Expected results.
 		expectedLastReductionEpochNum int64
 		expectedDistribution          sdk.Dec
-		expectedPanic                 bool
+		expectedError                 bool
 	}{
 		"before start epoch - no distributions": {
 			hookArgEpochNum: defaultMintingRewardsDistributionStartEpoch - 1,
@@ -346,7 +346,7 @@ func (suite *KeeperTestSuite) TestAfterEpochEnd() {
 
 			expectedDistribution:          sdk.ZeroDec(),
 			expectedLastReductionEpochNum: defaultMintingRewardsDistributionStartEpoch,
-			expectedPanic:                 true,
+			expectedError:                 true,
 		},
 	}
 
@@ -382,7 +382,7 @@ func (suite *KeeperTestSuite) TestAfterEpochEnd() {
 
 			developerAccountBalanceBeforeHook := app.BankKeeper.GetBalance(ctx, accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName), sdk.DefaultBondDenom)
 
-			if tc.expectedPanic {
+			if tc.expectedError {
 				// If panic is expected, burn developer module account balance so that it causes an error that leads to a
 				// panic in the hook.
 				suite.Require().NoError(distrKeeper.FundCommunityPool(ctx, sdk.NewCoins(developerAccountBalanceBeforeHook), accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName)))
@@ -393,13 +393,14 @@ func (suite *KeeperTestSuite) TestAfterEpochEnd() {
 			oldSupply := app.BankKeeper.GetSupply(ctx, sdk.DefaultBondDenom).Amount
 			suite.Require().Equal(sdk.NewInt(keeper.DeveloperVestingAmount), oldSupply)
 
-			osmoassert.ConditionalPanic(suite.T(), tc.expectedPanic, func() {
-				// System under test.
-				mintKeeper.AfterEpochEnd(ctx, defaultEpochIdentifier, tc.hookArgEpochNum)
-			})
+			if tc.expectedError {
+				suite.Require().Error(mintKeeper.AfterEpochEnd(ctx, defaultEpochIdentifier, tc.hookArgEpochNum))
+			} else {
+				suite.Require().NoError(mintKeeper.AfterEpochEnd(ctx, defaultEpochIdentifier, tc.hookArgEpochNum))
+			}
 
 			// If panics, the behavior is undefined.
-			if tc.expectedPanic {
+			if tc.expectedError {
 				return
 			}
 
@@ -541,7 +542,7 @@ func (suite *KeeperTestSuite) TestAfterEpochEnd_FirstYearThirdening_RealParamete
 	for i := int64(1); i <= defaultReductionPeriodInEpochs; i++ {
 		developerAccountBalanceBeforeHook := app.BankKeeper.GetBalance(ctx, accountKeeper.GetModuleAddress(types.DeveloperVestingModuleAcctName), sdk.DefaultBondDenom)
 
-		// System undert test.
+		// System under test.
 		mintKeeper.AfterEpochEnd(ctx, defaultEpochIdentifier, i)
 
 		// System truncates EpochProvisions because bank takes an Int.

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