-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(baseapp): create checktx handler #21979
Changes from 10 commits
136c9da
94f6f3d
c71365c
a789bbe
38b7154
1ade678
31c6047
b2c856a
b09a063
20e9fc0
228cd9b
0a50812
76c5587
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -37,7 +37,7 @@ import ( | |||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
type ( | ||||||||||||||||
execMode uint8 | ||||||||||||||||
execMode = uint8 | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using type aliases; prefer type definitions According to the Uber Go Style Guide, type aliases should be avoided. Consider defining -type (
- execMode = uint8
+type (
+ execMode uint8 📝 Committable suggestion
Suggested change
tac0turtle marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
|
||||||||||||||||
// StoreLoader defines a customizable function to control how we load the | ||||||||||||||||
// CommitMultiStore from disk. This is useful for state migration, when | ||||||||||||||||
|
@@ -91,6 +91,7 @@ type BaseApp struct { | |||||||||||||||
prepareCheckStater sdk.PrepareCheckStater // logic to run during commit using the checkState | ||||||||||||||||
precommiter sdk.Precommiter // logic to run during commit using the deliverState | ||||||||||||||||
versionModifier server.VersionModifier // interface to get and set the app version | ||||||||||||||||
checkTxHandler sdk.CheckTxHandler | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Group Per the Uber Go Style Guide, related struct fields should be grouped together. Consider moving postHandler sdk.PostHandler // post handler, optional
+ checkTxHandler sdk.CheckTxHandler // ABCI CheckTx handler
initChainer sdk.InitChainer // ABCI InitChain handler
preBlocker sdk.PreBlocker // logic to run before BeginBlocker
beginBlocker sdk.BeginBlocker // (legacy ABCI) BeginBlock handler
endBlocker sdk.EndBlocker // (legacy ABCI) EndBlock handler
- checkTxHandler sdk.CheckTxHandler 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for adding a comment and moving it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @julienrbrt, understood! Adding a comment to checkTxHandler sdk.CheckTxHandler // ABCI CheckTx handler 😊🐰 |
||||||||||||||||
|
||||||||||||||||
addrPeerFilter sdk.PeerFilter // filter peers by address and port | ||||||||||||||||
idPeerFilter sdk.PeerFilter // filter peers by node ID | ||||||||||||||||
|
@@ -688,7 +689,6 @@ func (app *BaseApp) getContextForTx(mode execMode, txBytes []byte) sdk.Context { | |||||||||||||||
// a branched multi-store. | ||||||||||||||||
func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context, storetypes.CacheMultiStore) { | ||||||||||||||||
ms := ctx.MultiStore() | ||||||||||||||||
// TODO: https://github.com/cosmos/cosmos-sdk/issues/2824 | ||||||||||||||||
msCache := ms.CacheMultiStore() | ||||||||||||||||
if msCache.TracingEnabled() { | ||||||||||||||||
msCache = msCache.SetTracingContext( | ||||||||||||||||
|
@@ -761,7 +761,7 @@ func (app *BaseApp) deliverTx(tx []byte) *abci.ExecTxResult { | |||||||||||||||
telemetry.SetGauge(float32(gInfo.GasWanted), "tx", "gas", "wanted") | ||||||||||||||||
}() | ||||||||||||||||
|
||||||||||||||||
gInfo, result, anteEvents, err := app.runTx(execModeFinalize, tx) | ||||||||||||||||
gInfo, result, anteEvents, err := app.runTx(execModeFinalize, tx, nil) | ||||||||||||||||
if err != nil { | ||||||||||||||||
resultStr = "failed" | ||||||||||||||||
resp = responseExecTxResultWithEvents( | ||||||||||||||||
|
@@ -822,7 +822,7 @@ type HasNestedMsgs interface { | |||||||||||||||
// Note, gas execution info is always returned. A reference to a Result is | ||||||||||||||||
// returned if the tx does not run out of gas and if all the messages are valid | ||||||||||||||||
// and execute successfully. An error is returned otherwise. | ||||||||||||||||
func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) { | ||||||||||||||||
func (app *BaseApp) runTx(mode execMode, txBytes []byte, tx sdk.Tx) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) { | ||||||||||||||||
julienrbrt marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
// NOTE: GasWanted should be returned by the AnteHandler. GasUsed is | ||||||||||||||||
// determined by the GasMeter. We need access to the context to get the gas | ||||||||||||||||
// meter, so we initialize upfront. | ||||||||||||||||
|
@@ -870,9 +870,12 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res | |||||||||||||||
defer consumeBlockGas() | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
tx, err := app.txDecoder(txBytes) | ||||||||||||||||
if err != nil { | ||||||||||||||||
return sdk.GasInfo{GasUsed: 0, GasWanted: 0}, nil, nil, sdkerrors.ErrTxDecode.Wrap(err.Error()) | ||||||||||||||||
// if the transaction is not decoded, decode it here | ||||||||||||||||
if tx == nil { | ||||||||||||||||
tx, err = app.txDecoder(txBytes) | ||||||||||||||||
if err != nil { | ||||||||||||||||
return sdk.GasInfo{GasUsed: 0, GasWanted: 0}, nil, nil, sdkerrors.ErrTxDecode.Wrap(err.Error()) | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
msgs := tx.GetMsgs() | ||||||||||||||||
|
@@ -1160,7 +1163,7 @@ func (app *BaseApp) PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) { | |||||||||||||||
return nil, err | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
_, _, _, err = app.runTx(execModePrepareProposal, bz) | ||||||||||||||||
_, _, _, err = app.runTx(execModePrepareProposal, bz, tx) | ||||||||||||||||
if err != nil { | ||||||||||||||||
return nil, err | ||||||||||||||||
Comment on lines
+1168
to
1170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider refactoring to eliminate code duplication Both Also applies to: 1185-1187 |
||||||||||||||||
} | ||||||||||||||||
|
@@ -1179,7 +1182,7 @@ func (app *BaseApp) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) { | |||||||||||||||
return nil, err | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
_, _, _, err = app.runTx(execModeProcessProposal, txBz) | ||||||||||||||||
_, _, _, err = app.runTx(execModeProcessProposal, txBz, tx) | ||||||||||||||||
if err != nil { | ||||||||||||||||
return nil, err | ||||||||||||||||
} | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
# CheckTx | ||
|
||
CheckTx is called by the `BaseApp` when comet receives a transaction from a client, over the p2p network or RPC. The CheckTx method is responsible for validating the transaction and returning an error if the transaction is invalid. | ||
|
||
```mermaid | ||
graph TD | ||
subgraph SDK[Cosmos SDK] | ||
B[Baseapp] | ||
A[AnteHandlers] | ||
B <-->|Validate TX| A | ||
end | ||
C[CometBFT] <-->|CheckTx|SDK | ||
U((User)) -->|Submit TX| C | ||
N[P2P] -->|Receive TX| C | ||
``` | ||
|
||
```go reference | ||
https://github.com/cosmos/cosmos-sdk/blob/31c604762a434c7b676b6a89897ecbd7c4653a23/baseapp/abci.go#L350-L390 | ||
``` | ||
|
||
## CheckTx Handler | ||
|
||
`CheckTxHandler` allows users to extend the logic of `CheckTx`. `CheckTxHandler` is called by pasding context and the transaction bytes received through ABCI. It is required that the handler returns deterministic results given the same transaction bytes. | ||
|
||
:::note | ||
we return the raw decoded transaction here to avoid decoding it twice. | ||
::: | ||
|
||
```go | ||
type CheckTxHandler func(ctx sdk.Context, tx []byte) (Tx, error) | ||
``` | ||
|
||
Setting a custom `CheckTxHandler` is optional. It can be done from your app.go file: | ||
|
||
```go | ||
func NewSimApp( | ||
logger log.Logger, | ||
db corestore.KVStoreWithBatch, | ||
traceStore io.Writer, | ||
loadLatest bool, | ||
appOpts servertypes.AppOptions, | ||
baseAppOptions ...func(*baseapp.BaseApp), | ||
) *SimApp { | ||
... | ||
// Create ChecktxHandler | ||
checktxHandler := abci.NewCustomCheckTxHandler(...) | ||
app.SetCheckTxHandler(checktxHandler) | ||
... | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve TODO comments regarding gas value types
The code includes TODO comments questioning whether
GasWanted
andGasUsed
should accept unsigned integers. Since gas values are inherently non-negative, consider usinguint64
instead ofint64
to accurately represent these values and prevent potential issues with negative values.