diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e993e712165..f293493ddfe8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#18531](https://github.com/cosmos/cosmos-sdk/pull/18531) Baseapp's `GetConsensusParams` returns an empty struct instead of panicking if no params are found. * (client/tx) [#18472](https://github.com/cosmos/cosmos-sdk/pull/18472) Utilizes the correct Pubkey when simulating a transaction. * (baseapp) [#18486](https://github.com/cosmos/cosmos-sdk/pull/18486) Fixed FinalizeBlock calls not being passed to ABCIListeners. +* (baseapp) [#18627](https://github.com/cosmos/cosmos-sdk/pull/18627) Post handlers are run on non successful transaction executions too. +* (baseapp) [#18654](https://github.com/cosmos/cosmos-sdk/pull/18654) Fixes an issue in which `gogoproto.Merge` does not work with gogoproto messages with custom types. ## [v0.50.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1) - 2023-11-07 diff --git a/UPGRADING.md b/UPGRADING.md index 18aee913c893..1f610f2a3e87 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -218,7 +218,7 @@ User manually wiring their chain need to add the logger argument when creating t #### Module Basics -Previously, the `ModuleBasics` was a global variable that was used to register all modules's `AppModuleBasic` implementation. +Previously, the `ModuleBasics` was a global variable that was used to register all modules' `AppModuleBasic` implementation. The global variable has been removed and the basic module manager can be now created from the module manager. This is automatically done for `depinject` / app v2 users, however for supplying different app module implementation, pass them via `depinject.Supply` in the main `AppConfig` (`app_config.go`): @@ -237,7 +237,7 @@ depinject.Supply( ) ``` -Users manually wiring their chain need to use the new `module.NewBasicManagerFromManager` function, after the module manager creation, and pass a `map[string]module.AppModuleBasic` as argument for optionally overridding some module's `AppModuleBasic`. +Users manually wiring their chain need to use the new `module.NewBasicManagerFromManager` function, after the module manager creation, and pass a `map[string]module.AppModuleBasic` as argument for optionally overriding some module's `AppModuleBasic`. #### AutoCLI @@ -365,7 +365,7 @@ To learn more see the [docs](https://docs.cosmos.network/main/learn/advanced/tra * Messages no longer need to implement the `LegacyMsg` interface and implementations of `GetSignBytes` can be deleted. Because of this change, global legacy Amino codec definitions and their registration in `init()` can safely be removed as well. -* The `AppModuleBasic` interface has been simplifed. Defining `GetTxCmd() *cobra.Command` and `GetQueryCmd() *cobra.Command` is no longer required. The module manager detects when module commands are defined. If AutoCLI is enabled, `EnhanceRootCommand()` will add the auto-generated commands to the root command, unless a custom module command is defined and register that one instead. +* The `AppModuleBasic` interface has been simplified. Defining `GetTxCmd() *cobra.Command` and `GetQueryCmd() *cobra.Command` is no longer required. The module manager detects when module commands are defined. If AutoCLI is enabled, `EnhanceRootCommand()` will add the auto-generated commands to the root command, unless a custom module command is defined and register that one instead. * The following modules' `Keeper` methods now take in a `context.Context` instead of `sdk.Context`. Any module that has an interfaces for them (like "expected keepers") will need to update and re-generate mocks if needed: diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 513e76b742a5..8de14c5f058b 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -925,24 +925,25 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res if err == nil { result, err = app.runMsgs(runMsgCtx, msgs, msgsV2, mode) } - if err == nil { - // Run optional postHandlers. - // - // Note: If the postHandler fails, we also revert the runMsgs state. - if app.postHandler != nil { - // The runMsgCtx context currently contains events emitted by the ante handler. - // We clear this to correctly order events without duplicates. - // Note that the state is still preserved. - postCtx := runMsgCtx.WithEventManager(sdk.NewEventManager()) - - newCtx, err := app.postHandler(postCtx, tx, mode == execModeSimulate, err == nil) - if err != nil { - return gInfo, nil, anteEvents, err - } - result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...) + // Run optional postHandlers (should run regardless of the execution result). + // + // Note: If the postHandler fails, we also revert the runMsgs state. + if app.postHandler != nil { + // The runMsgCtx context currently contains events emitted by the ante handler. + // We clear this to correctly order events without duplicates. + // Note that the state is still preserved. + postCtx := runMsgCtx.WithEventManager(sdk.NewEventManager()) + + newCtx, err := app.postHandler(postCtx, tx, mode == execModeSimulate, err == nil) + if err != nil { + return gInfo, nil, anteEvents, err } + result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...) + } + + if err == nil { if mode == execModeFinalize { // When block gas exceeds, it'll panic and won't commit the cached store. consumeBlockGas() diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index ea082b8b0464..3fe1b3463f0f 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -587,6 +587,53 @@ func TestBaseAppAnteHandler(t *testing.T) { suite.baseApp.Commit() } +func TestBaseAppPostHandler(t *testing.T) { + postHandlerRun := false + anteOpt := func(bapp *baseapp.BaseApp) { + bapp.SetPostHandler(func(ctx sdk.Context, tx sdk.Tx, simulate, success bool) (newCtx sdk.Context, err error) { + postHandlerRun = true + return ctx, nil + }) + } + + suite := NewBaseAppSuite(t, anteOpt) + + baseapptestutil.RegisterCounterServer(suite.baseApp.MsgServiceRouter(), CounterServerImpl{t, capKey1, []byte("foo")}) + + _, err := suite.baseApp.InitChain(&abci.RequestInitChain{ + ConsensusParams: &cmtproto.ConsensusParams{}, + }) + require.NoError(t, err) + + // execute a tx that will fail ante handler execution + // + // NOTE: State should not be mutated here. This will be implicitly checked by + // the next txs ante handler execution (anteHandlerTxTest). + tx := newTxCounter(t, suite.txConfig, 0, 0) + txBytes, err := suite.txConfig.TxEncoder()(tx) + require.NoError(t, err) + + res, err := suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 1, Txs: [][]byte{txBytes}}) + require.NoError(t, err) + require.Empty(t, res.Events) + require.True(t, res.TxResults[0].IsOK(), fmt.Sprintf("%v", res)) + + // PostHandler runs on successful message execution + require.True(t, postHandlerRun) + + // It should also run on failed message execution + postHandlerRun = false + tx = setFailOnHandler(suite.txConfig, tx, true) + txBytes, err = suite.txConfig.TxEncoder()(tx) + require.NoError(t, err) + res, err = suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 1, Txs: [][]byte{txBytes}}) + require.NoError(t, err) + require.Empty(t, res.Events) + require.False(t, res.TxResults[0].IsOK(), fmt.Sprintf("%v", res)) + + require.True(t, postHandlerRun) +} + // Test and ensure that invalid block heights always cause errors. // See issues: // - https://github.com/cosmos/cosmos-sdk/issues/11220 diff --git a/baseapp/internal/protocompat/protocompat.go b/baseapp/internal/protocompat/protocompat.go index b2a97f8890fc..6b8cdc081896 100644 --- a/baseapp/internal/protocompat/protocompat.go +++ b/baseapp/internal/protocompat/protocompat.go @@ -6,6 +6,7 @@ import ( "reflect" gogoproto "github.com/cosmos/gogoproto/proto" + "github.com/golang/protobuf/proto" // nolint: staticcheck // needed because gogoproto.Merge does not work consistently. See NOTE: comments. "google.golang.org/grpc" proto2 "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" @@ -122,14 +123,18 @@ func makeGogoHybridHandler(prefMethod protoreflect.MethodDescriptor, cdc codec.B } resp, err := method.Handler(handler, ctx, func(msg any) error { // merge! ref: https://github.com/cosmos/cosmos-sdk/issues/18003 - gogoproto.Merge(msg.(gogoproto.Message), inReq) + // NOTE: using gogoproto.Merge will fail for some reason unknown to me, but + // using proto.Merge with gogo messages seems to work fine. + proto.Merge(msg.(gogoproto.Message), inReq) return nil }, nil) if err != nil { return err } // merge resp, ref: https://github.com/cosmos/cosmos-sdk/issues/18003 - gogoproto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message)) + // NOTE: using gogoproto.Merge will fail for some reason unknown to me, but + // using proto.Merge with gogo messages seems to work fine. + proto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message)) return nil }, nil } @@ -162,14 +167,19 @@ func makeGogoHybridHandler(prefMethod protoreflect.MethodDescriptor, cdc codec.B // we can just call the handler after making a copy of the message, for safety reasons. resp, err := method.Handler(handler, ctx, func(msg any) error { // ref: https://github.com/cosmos/cosmos-sdk/issues/18003 - gogoproto.Merge(msg.(gogoproto.Message), m) + asGogoProto := msg.(gogoproto.Message) + // NOTE: using gogoproto.Merge will fail for some reason unknown to me, but + // using proto.Merge with gogo messages seems to work fine. + proto.Merge(asGogoProto, m) return nil }, nil) if err != nil { return err } // merge on the resp, ref: https://github.com/cosmos/cosmos-sdk/issues/18003 - gogoproto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message)) + // NOTE: using gogoproto.Merge will fail for some reason unknown to me, but + // using proto.Merge with gogo messages seems to work fine. + proto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message)) return nil default: panic("unreachable")