Skip to content

Commit

Permalink
refactor: Revert middlewares to antehandlers (part 1/2: baseapp) (#11979
Browse files Browse the repository at this point in the history
)

## Description

We decided to remove middlewares, and revert to antehandlers (exactly like in v045) for this release. A better middleware solution will be implemented after v046.

ref: #11955 

Because this refactor is big, so I decided to cut it into two. This PR is part 1 of 2:
- part 1: Revert baseapp and middlewares to v0.45.4
- part 2: Add posthandler, tips, priority

---
 Suggestion for reviewers:

This PR might still be hard to review though. I think it's easier to actually review the diff between v0.45.4 and this PR:
- `git difftool -d v0.45.4..am/revert-045-baseapp baseapp`
  - most important parts to review: runTx, runMsgs
- `git difftool -d v0.45.4..am/revert-045-baseapp x/auth/ante`
  - only cosmetic changes
- `git difftool -d v0.45.4..am/revert-045-baseapp simapp`



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
  • Loading branch information
amaury1093 authored May 20, 2022
1 parent cf750b8 commit 01832e6
Show file tree
Hide file tree
Showing 76 changed files with 3,077 additions and 4,611 deletions.
7 changes: 1 addition & 6 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#10248](https://github.com/cosmos/cosmos-sdk/pull/10248) Remove unused `KeyPowerReduction` variable from x/staking types.
* (x/bank) [\#9832](https://github.com/cosmos/cosmos-sdk/pull/9832) `AddressFromBalancesStore` renamed to `AddressAndDenomFromBalancesStore`.
* (tests) [\#9938](https://github.com/cosmos/cosmos-sdk/pull/9938) `simapp.Setup` accepts additional `testing.T` argument.
* (baseapp) [\#9920](https://github.com/cosmos/cosmos-sdk/pull/9920) BaseApp `{Check,Deliver,Simulate}Tx` methods are now replaced by a middleware stack.
* Replace the Antehandler interface with the `tx.Handler` and `tx.Middleware` interfaces.
* Replace `baseapp.SetAnteHandler` with `baseapp.SetTxHandler`.
* Move Msg routers from BaseApp to middlewares.
* Move Baseapp panic recovery into a middleware.
* Rename simulation helper methods `baseapp.{Check,Deliver}` to `baseapp.Sim{Check,Deliver}**`.
* (baseapp) [\#11979](https://github.com/cosmos/cosmos-sdk/pull/11979) Rename baseapp simulation helper methods `baseapp.{Check,Deliver}` to `baseapp.Sim{Check,Deliver}`.
* (x/gov) [\#10373](https://github.com/cosmos/cosmos-sdk/pull/10373) Removed gov `keeper.{MustMarshal, MustUnmarshal}`.
* [\#10348](https://github.com/cosmos/cosmos-sdk/pull/10348) StdSignBytes takes a new argument of type `*tx.Tip` for signing over tips using LEGACY_AMINO_JSON.
* [\#10208](https://github.com/cosmos/cosmos-sdk/pull/10208) The `x/auth/signing.Tx` interface now also includes a new `GetTip() *tx.Tip` method for verifying tipped transactions. The `x/auth/types` expected BankKeeper interface now expects the `SendCoins` method too.
Expand Down
87 changes: 27 additions & 60 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import (

"github.com/cosmos/cosmos-sdk/codec"
snapshottypes "github.com/cosmos/cosmos-sdk/snapshots/types"
"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/tx"
)

// Supported ABCI Query prefixes
Expand Down Expand Up @@ -233,8 +233,8 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc

// CheckTx implements the ABCI interface and executes a tx in CheckTx mode. In
// CheckTx mode, messages are not executed. This means messages are only validated
// and only the wired middlewares are executed. State is persisted to the BaseApp's
// internal CheckTx state if the middlewares' CheckTx pass. Otherwise, the ResponseCheckTx
// and only the AnteHandler is executed. State is persisted to the BaseApp's
// internal CheckTx state if the AnteHandler passes. Otherwise, the ResponseCheckTx
// will contain releveant error information. Regardless of tx execution outcome,
// the ResponseCheckTx will contain relevant gas execution context.
func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
Expand All @@ -251,18 +251,18 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
panic(fmt.Sprintf("unknown RequestCheckTx type: %s", req.Type))
}

ctx := app.getContextForTx(mode, req.Tx)
res, checkRes, err := app.txHandler.CheckTx(ctx, tx.Request{TxBytes: req.Tx}, tx.RequestCheckTx{Type: req.Type})
gInfo, result, anteEvents, err := app.runTx(mode, req.Tx)
if err != nil {
return sdkerrors.ResponseCheckTx(err, uint64(res.GasUsed), uint64(res.GasWanted), app.trace)
return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace)
}

abciRes, err := convertTxResponseToCheckTx(res, checkRes)
if err != nil {
return sdkerrors.ResponseCheckTx(err, uint64(res.GasUsed), uint64(res.GasWanted), app.trace)
return abci.ResponseCheckTx{
GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints?
GasUsed: int64(gInfo.GasUsed), // TODO: Should type accept unsigned ints?
Log: result.Log,
Data: result.Data,
Events: sdk.MarkEventsToIndex(result.Events, app.indexEvents),
}

return abciRes
}

// DeliverTx implements the ABCI interface and executes a tx in DeliverTx mode.
Expand All @@ -271,28 +271,29 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
// Regardless of tx execution outcome, the ResponseDeliverTx will contain relevant
// gas execution context.
func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx {
var abciRes abci.ResponseDeliverTx
gInfo := sdk.GasInfo{}
resultStr := "successful"

defer func() {
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenDeliverTx(app.deliverState.ctx, req, abciRes); err != nil {
app.logger.Error("DeliverTx listening hook failed", "err", err)
}
}
telemetry.IncrCounter(1, "tx", "count")
telemetry.IncrCounter(1, "tx", resultStr)
telemetry.SetGauge(float32(gInfo.GasUsed), "tx", "gas", "used")
telemetry.SetGauge(float32(gInfo.GasWanted), "tx", "gas", "wanted")
}()

ctx := app.getContextForTx(runTxModeDeliver, req.Tx)
res, err := app.txHandler.DeliverTx(ctx, tx.Request{TxBytes: req.Tx})
gInfo, result, anteEvents, err := app.runTx(runTxModeDeliver, req.Tx)
if err != nil {
abciRes = sdkerrors.ResponseDeliverTx(err, uint64(res.GasUsed), uint64(res.GasWanted), app.trace)
return abciRes
resultStr = "failed"
return sdkerrors.ResponseDeliverTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace)
}

abciRes, err = convertTxResponseToDeliverTx(res)
if err != nil {
return sdkerrors.ResponseDeliverTx(err, uint64(res.GasUsed), uint64(res.GasWanted), app.trace)
return abci.ResponseDeliverTx{
GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints?
GasUsed: int64(gInfo.GasUsed), // TODO: Should type accept unsigned ints?
Log: result.Log,
Data: result.Data,
Events: sdk.MarkEventsToIndex(result.Events, app.indexEvents),
}

return abciRes
}

// Commit implements the ABCI interface. It will commit all state that exists in
Expand Down Expand Up @@ -853,37 +854,3 @@ func SplitABCIQueryPath(requestPath string) (path []string) {

return path
}

// makeABCIData generates the Data field to be sent to ABCI Check/DeliverTx.
func makeABCIData(txRes tx.Response) ([]byte, error) {
return proto.Marshal(&sdk.TxMsgData{MsgResponses: txRes.MsgResponses})
}

// convertTxResponseToCheckTx converts a tx.Response into a abci.ResponseCheckTx.
func convertTxResponseToCheckTx(txRes tx.Response, checkRes tx.ResponseCheckTx) (abci.ResponseCheckTx, error) {
data, err := makeABCIData(txRes)
if err != nil {
return abci.ResponseCheckTx{}, nil
}

return abci.ResponseCheckTx{
Data: data,
Log: txRes.Log,
Events: txRes.Events,
Priority: checkRes.Priority,
}, nil
}

// convertTxResponseToDeliverTx converts a tx.Response into a abci.ResponseDeliverTx.
func convertTxResponseToDeliverTx(txRes tx.Response) (abci.ResponseDeliverTx, error) {
data, err := makeABCIData(txRes)
if err != nil {
return abci.ResponseDeliverTx{}, nil
}

return abci.ResponseDeliverTx{
Data: data,
Log: txRes.Log,
Events: txRes.Events,
}, nil
}
72 changes: 36 additions & 36 deletions baseapp/abci_test.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
package baseapp_test
package baseapp

import (
"testing"

"github.com/stretchr/testify/require"
abci "github.com/tendermint/tendermint/abci/types"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
tmprototypes "github.com/tendermint/tendermint/proto/tendermint/types"
dbm "github.com/tendermint/tm-db"

"github.com/cosmos/cosmos-sdk/baseapp"
pruningtypes "github.com/cosmos/cosmos-sdk/pruning/types"
"github.com/cosmos/cosmos-sdk/snapshots"
snapshottypes "github.com/cosmos/cosmos-sdk/snapshots/types"
Expand All @@ -24,81 +24,81 @@ func TestGetBlockRentionHeight(t *testing.T) {
require.NoError(t, err)

testCases := map[string]struct {
bapp *baseapp.BaseApp
bapp *BaseApp
maxAgeBlocks int64
commitHeight int64
expected int64
}{
"defaults": {
bapp: baseapp.NewBaseApp(name, logger, db),
bapp: NewBaseApp(name, logger, db, nil),
maxAgeBlocks: 0,
commitHeight: 499000,
expected: 0,
},
"pruning unbonding time only": {
bapp: baseapp.NewBaseApp(name, logger, db, baseapp.SetMinRetainBlocks(1)),
bapp: NewBaseApp(name, logger, db, nil, SetMinRetainBlocks(1)),
maxAgeBlocks: 362880,
commitHeight: 499000,
expected: 136120,
},
"pruning iavl snapshot only": {
bapp: baseapp.NewBaseApp(
name, logger, db,
baseapp.SetPruning(pruningtypes.NewPruningOptions(pruningtypes.PruningNothing)),
baseapp.SetMinRetainBlocks(1),
baseapp.SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(10000, 1)),
bapp: NewBaseApp(
name, logger, db, nil,
SetPruning(pruningtypes.NewPruningOptions(pruningtypes.PruningNothing)),
SetMinRetainBlocks(1),
SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(10000, 1)),
),
maxAgeBlocks: 0,
commitHeight: 499000,
expected: 489000,
},
"pruning state sync snapshot only": {
bapp: baseapp.NewBaseApp(
name, logger, db,
baseapp.SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
baseapp.SetMinRetainBlocks(1),
bapp: NewBaseApp(
name, logger, db, nil,
SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
SetMinRetainBlocks(1),
),
maxAgeBlocks: 0,
commitHeight: 499000,
expected: 349000,
},
"pruning min retention only": {
bapp: baseapp.NewBaseApp(
name, logger, db,
baseapp.SetMinRetainBlocks(400000),
bapp: NewBaseApp(
name, logger, db, nil,
SetMinRetainBlocks(400000),
),
maxAgeBlocks: 0,
commitHeight: 499000,
expected: 99000,
},
"pruning all conditions": {
bapp: baseapp.NewBaseApp(
name, logger, db,
baseapp.SetPruning(pruningtypes.NewCustomPruningOptions(0, 0)),
baseapp.SetMinRetainBlocks(400000),
baseapp.SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
bapp: NewBaseApp(
name, logger, db, nil,
SetPruning(pruningtypes.NewCustomPruningOptions(0, 0)),
SetMinRetainBlocks(400000),
SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
),
maxAgeBlocks: 362880,
commitHeight: 499000,
expected: 99000,
},
"no pruning due to no persisted state": {
bapp: baseapp.NewBaseApp(
name, logger, db,
baseapp.SetPruning(pruningtypes.NewCustomPruningOptions(0, 0)),
baseapp.SetMinRetainBlocks(400000),
baseapp.SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
bapp: NewBaseApp(
name, logger, db, nil,
SetPruning(pruningtypes.NewCustomPruningOptions(0, 0)),
SetMinRetainBlocks(400000),
SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
),
maxAgeBlocks: 362880,
commitHeight: 10000,
expected: 0,
},
"disable pruning": {
bapp: baseapp.NewBaseApp(
name, logger, db,
baseapp.SetPruning(pruningtypes.NewCustomPruningOptions(0, 0)),
baseapp.SetMinRetainBlocks(0),
baseapp.SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
bapp: NewBaseApp(
name, logger, db, nil,
SetPruning(pruningtypes.NewCustomPruningOptions(0, 0)),
SetMinRetainBlocks(0),
SetSnapshot(snapshotStore, snapshottypes.NewSnapshotOptions(50000, 3)),
),
maxAgeBlocks: 362880,
commitHeight: 499000,
Expand Down Expand Up @@ -134,12 +134,12 @@ func TestBaseAppCreateQueryContext(t *testing.T) {
logger := defaultLogger()
db := dbm.NewMemDB()
name := t.Name()
app := baseapp.NewBaseApp(name, logger, db)
app := NewBaseApp(name, logger, db, nil)

app.BeginBlock(abci.RequestBeginBlock{Header: tmprototypes.Header{Height: 1}})
app.BeginBlock(abci.RequestBeginBlock{Header: tmproto.Header{Height: 1}})
app.Commit()

app.BeginBlock(abci.RequestBeginBlock{Header: tmprototypes.Header{Height: 2}})
app.BeginBlock(abci.RequestBeginBlock{Header: tmproto.Header{Height: 2}})
app.Commit()

testCases := []struct {
Expand All @@ -156,7 +156,7 @@ func TestBaseAppCreateQueryContext(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
_, err := app.CreateQueryContext(tc.height, tc.prove)
_, err := app.createQueryContext(tc.height, tc.prove)
if tc.expErr {
require.Error(t, err)
} else {
Expand Down
Loading

0 comments on commit 01832e6

Please sign in to comment.