From 1ec83c30f7537719e0154b08a51cb6f2d66455b3 Mon Sep 17 00:00:00 2001 From: Puneet <59960662+puneet2019@users.noreply.github.com> Date: Tue, 30 Aug 2022 20:52:52 +0400 Subject: [PATCH] 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 | 1 + x/epochs/keeper/hooks.go | 6 +++-- x/epochs/types/hooks.go | 15 ++++++------ x/epochs/types/hooks_test.go | 45 ++++++++++++++++++++++++++++-------- x/incentives/keeper/hooks.go | 18 ++++++++------- x/mint/keeper/hooks.go | 20 ++++++++-------- x/mint/keeper/hooks_test.go | 19 +++++++-------- x/superfluid/keeper/epoch.go | 3 ++- x/superfluid/keeper/hooks.go | 7 +++--- x/twap/listeners.go | 7 ++++-- x/txfees/keeper/hooks.go | 16 +++++++++---- 11 files changed, 101 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index befd500bae9..2f7923eff40 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/x/epochs/keeper/hooks.go b/x/epochs/keeper/hooks.go index 46a57b0bf54..77cc6308d61 100644 --- a/x/epochs/keeper/hooks.go +++ b/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) } diff --git a/x/epochs/types/hooks.go b/x/epochs/types/hooks.go index e54367b14f5..a626ffa1a56 100644 --- a/x/epochs/types/hooks.go +++ b/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) diff --git a/x/epochs/types/hooks_test.go b/x/epochs/types/hooks_test.go index 1d15007fd12..407f0bf2c3d 100644 --- a/x/epochs/types/hooks_test.go +++ b/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) - } }) diff --git a/x/incentives/keeper/hooks.go b/x/incentives/keeper/hooks.go index fd314a0668a..8c95d029268 100644 --- a/x/incentives/keeper/hooks.go +++ b/x/incentives/keeper/hooks.go @@ -9,11 +9,12 @@ 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 @@ -21,7 +22,7 @@ func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumb 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) } diff --git a/x/mint/keeper/hooks.go b/x/mint/keeper/hooks.go index 7d768efe119..4faddebe254 100644 --- a/x/mint/keeper/hooks.go +++ b/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) } diff --git a/x/mint/keeper/hooks_test.go b/x/mint/keeper/hooks_test.go index 8ebc17b8b30..5765dceed4f 100644 --- a/x/mint/keeper/hooks_test.go +++ b/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. diff --git a/x/superfluid/keeper/epoch.go b/x/superfluid/keeper/epoch.go index a2d7c046d40..10519b7a819 100644 --- a/x/superfluid/keeper/epoch.go +++ b/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) { diff --git a/x/superfluid/keeper/hooks.go b/x/superfluid/keeper/hooks.go index e71d1e9be50..9557de91415 100644 --- a/x/superfluid/keeper/hooks.go +++ b/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 diff --git a/x/twap/listeners.go b/x/twap/listeners.go index 7ead436d387..c4453666761 100644 --- a/x/twap/listeners.go +++ b/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 diff --git a/x/txfees/keeper/hooks.go b/x/txfees/keeper/hooks.go index 51a06f266d1..604a8d4bb7e 100644 --- a/x/txfees/keeper/hooks.go +++ b/x/txfees/keeper/hooks.go @@ -8,10 +8,12 @@ import ( txfeestypes "github.com/osmosis-labs/osmosis/v11/x/txfees/types" ) -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 +} // at the end of each epoch, swap all non-OSMO fees into OSMO and transfer to fee module account -func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) { +func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error { nonNativeFeeAddr := k.accountKeeper.GetModuleAddress(txfeestypes.NonNativeFeeCollectorName) baseDenom, _ := k.GetBaseDenom(ctx) feeTokens := k.GetFeeTokens(ctx) @@ -44,6 +46,8 @@ func (k Keeper) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumb err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, txfeestypes.NonNativeFeeCollectorName, txfeestypes.FeeCollectorName, baseDenomCoins) return err }) + + return nil } // Hooks wrapper struct for incentives keeper @@ -58,8 +62,10 @@ func (k Keeper) Hooks() Hooks { return Hooks{k} } -func (h Hooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, epochNumber int64) {} +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) }