From f88a395e43de42c05a7be547ed0206f10295fa6f Mon Sep 17 00:00:00 2001 From: Unique Divine <51418232+Unique-Divine@users.noreply.github.com> Date: Fri, 7 Jun 2024 21:58:21 -0500 Subject: [PATCH] refactor(evm): Remove dead code and document non-EVM ante handler (#1914) * test(evm): grpc_query full coverage * chore: changelog update * chore: refactored eth util methods * fix: removed hardcoded gas value in grpc_query test * Squashed commit of the following: commit b5687130ff5f3d020a3b14d219fec3a816579c30 Author: Unique-Divine Date: Wed Jun 5 20:57:44 2024 -0500 chore: run tidy commit 1f1f9385952c4a170f744726bed8a3ee7c376028 Merge: 3e3cc837 bbcc6f8c Author: Unique-Divine Date: Wed Jun 5 19:16:30 2024 -0500 Merge branch 'main' into ud/fix-race-condition commit 3e3cc837b204971c58c775fe25d28fd01bce4021 Author: Unique-Divine Date: Wed Jun 5 19:15:40 2024 -0500 chore: changelog commit 3876ccb431aac5c9991a3540d764061cb52a0857 Author: Unique-Divine Date: Wed Jun 5 19:04:00 2024 -0500 refactor: more consistent test names commit aaa0a19f103a12c60f226a5057779a74d680e61c Author: Unique-Divine Date: Wed Jun 5 18:53:09 2024 -0500 test(oracle): Fix missing tear down step for oracle integration test commit 8c3c35eafc41d29becba1379d1f9ca5e984d8d9a Author: Unique-Divine Date: Wed Jun 5 17:55:56 2024 -0500 chore: add test comands to justfile commit 4916282353300b2dbf639e599cfbc3685cda01f6 Merge: 64ed0a29 e7e708d7 Author: Unique-Divine Date: Fri May 31 09:35:33 2024 -0500 Merge branch 'main' into ud/fix-race-condition commit 64ed0a29c918c4c1402eceddc13998ed4a156712 Author: Unique-Divine Date: Fri May 31 01:44:55 2024 -0500 fix(gosdk): tests parallel race condition * refactor: ante handler and evm cleanup * refactor: remove dead code * refactor(evm): Remove dead code and document non-EVM ante handler * chore: add issue number for TODO comment * chore: another issue ticket --- CHANGELOG.md | 1 + app/ante.go | 80 +++++---------- app/ante/authz_guard.go | 9 ++ app/ante/fixed_gas.go | 15 ++- app/ante/testutil_test.go | 2 +- app/evmante_fee_market.go | 20 +--- app/evmante_handler.go | 2 +- app/evmante_test.go | 3 + x/common/testutil/cli/network.go | 168 +++++++++++++++---------------- x/evm/keeper/grpc_query_test.go | 30 ++++-- 10 files changed, 152 insertions(+), 178 deletions(-) create mode 100644 app/ante/authz_guard.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e9c9591d..5f1aa8ec8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#1907](https://github.com/NibiruChain/nibiru/pull/1907) - test(evm): grpc_query full coverage - [#1909](https://github.com/NibiruChain/nibiru/pull/1909) - chore(evm): set is_london true by default and removed from config - [#1911](https://github.com/NibiruChain/nibiru/pull/1911) - chore(evm): simplified config by removing old eth forks +- [#1914](https://github.com/NibiruChain/nibiru/pull/1914) - refactor(evm): Remove dead code and document non-EVM ante handler #### Dapp modules: perp, spot, oracle, etc diff --git a/app/ante.go b/app/ante.go index 21435d335..003bec946 100644 --- a/app/ante.go +++ b/app/ante.go @@ -31,7 +31,6 @@ func NewAnteHandler( var anteHandler sdk.AnteHandler hasExt, typeUrl := TxHasExtensions(tx) - // TODO: handle ethereum txs if hasExt && typeUrl != "" { anteHandler = AnteHandlerExtendedTx(typeUrl, keepers, opts, ctx) return anteHandler(ctx, tx, sim) @@ -39,7 +38,7 @@ func NewAnteHandler( switch tx.(type) { case sdk.Tx: - anteHandler = AnteHandlerStandardTx(opts) + anteHandler = NewAnteHandlerNonEVM(keepers, opts) default: return ctx, fmt.Errorf("invalid tx type (%T) in AnteHandler", tx) } @@ -47,50 +46,6 @@ func NewAnteHandler( } } -func AnteHandlerStandardTx(opts ante.AnteHandlerOptions) sdk.AnteHandler { - anteDecorators := []sdk.AnteDecorator{ - AnteDecoratorPreventEtheruemTxMsgs{}, // reject MsgEthereumTxs - authante.NewSetUpContextDecorator(), - wasmkeeper.NewLimitSimulationGasDecorator(opts.WasmConfig.SimulationGasLimit), - wasmkeeper.NewCountTXDecorator(opts.TxCounterStoreKey), - authante.NewExtensionOptionsDecorator(opts.ExtensionOptionChecker), - authante.NewValidateBasicDecorator(), - authante.NewTxTimeoutHeightDecorator(), - authante.NewValidateMemoDecorator(opts.AccountKeeper), - ante.NewPostPriceFixedPriceDecorator(), - ante.AnteDecoratorStakingCommission{}, - authante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper), - // Replace fee ante from cosmos auth with a custom one. - authante.NewDeductFeeDecorator( - opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, opts.TxFeeChecker), - devgasante.NewDevGasPayoutDecorator( - opts.DevGasBankKeeper, opts.DevGasKeeper), - // NOTE: SetPubKeyDecorator must be called before all signature verification decorators - authante.NewSetPubKeyDecorator(opts.AccountKeeper), - authante.NewValidateSigCountDecorator(opts.AccountKeeper), - authante.NewSigGasConsumeDecorator(opts.AccountKeeper, opts.SigGasConsumer), - authante.NewSigVerificationDecorator(opts.AccountKeeper, opts.SignModeHandler), - authante.NewIncrementSequenceDecorator(opts.AccountKeeper), - ibcante.NewRedundantRelayDecorator(opts.IBCKeeper), - } - - return sdk.ChainAnteDecorators(anteDecorators...) -} - -func TxHasExtensions(tx sdk.Tx) (hasExt bool, typeUrl string) { - extensionTx, ok := tx.(authante.HasExtensionOptionsTx) - if !ok { - return false, "" - } - - extOpts := extensionTx.GetExtensionOptions() - if len(extOpts) == 0 { - return false, "" - } - - return true, extOpts[0].GetTypeUrl() -} - func AnteHandlerExtendedTx( typeUrl string, keepers AppKeepers, @@ -123,24 +78,25 @@ func NewAnteHandlerNonEVM( authante.NewSetUpContextDecorator(), wasmkeeper.NewLimitSimulationGasDecorator(opts.WasmConfig.SimulationGasLimit), wasmkeeper.NewCountTXDecorator(opts.TxCounterStoreKey), - // TODO: UD - // cosmosante.NewAuthzLimiterDecorator( // disable the Msg types that cannot be included on an authz.MsgExec msgs field - // sdk.MsgTypeURL(&evm.MsgEthereumTx{}), - // sdk.MsgTypeURL(&sdkvesting.MsgCreateVestingAccount{}), - // ), + // TODO: bug(security): Authz is unsafe. Let's include a guard to make + // things safer. + // ticket: https://github.com/NibiruChain/nibiru/issues/1915 authante.NewExtensionOptionsDecorator(opts.ExtensionOptionChecker), authante.NewValidateBasicDecorator(), authante.NewTxTimeoutHeightDecorator(), authante.NewValidateMemoDecorator(opts.AccountKeeper), - // TODO: UD - // cosmosante.NewMinGasPriceDecorator(options.FeeMarketKeeper, options.EvmKeeper), - ante.NewPostPriceFixedPriceDecorator(), + ante.AnteDecoratorEnsureSinglePostPriceMessage{}, ante.AnteDecoratorStakingCommission{}, + // ----------- Ante Handlers: Gas authante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper), + // TODO: spike(security): Does minimum gas price of 0 pose a risk? + // ticket: https://github.com/NibiruChain/nibiru/issues/1916 authante.NewDeductFeeDecorator( opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, opts.TxFeeChecker), + // ----------- Ante Handlers: devgas devgasante.NewDevGasPayoutDecorator( opts.DevGasBankKeeper, opts.DevGasKeeper), + // ----------- Ante Handlers: Keys and signatures // NOTE: SetPubKeyDecorator must be called before all signature verification decorators authante.NewSetPubKeyDecorator(opts.AccountKeeper), authante.NewValidateSigCountDecorator(opts.AccountKeeper), @@ -148,6 +104,20 @@ func NewAnteHandlerNonEVM( authante.NewSigVerificationDecorator(opts.AccountKeeper, opts.SignModeHandler), authante.NewIncrementSequenceDecorator(opts.AccountKeeper), ibcante.NewRedundantRelayDecorator(opts.IBCKeeper), - NewGasWantedDecorator(k), + AnteDecoratorGasWanted{}, ) } + +func TxHasExtensions(tx sdk.Tx) (hasExt bool, typeUrl string) { + extensionTx, ok := tx.(authante.HasExtensionOptionsTx) + if !ok { + return false, "" + } + + extOpts := extensionTx.GetExtensionOptions() + if len(extOpts) == 0 { + return false, "" + } + + return true, extOpts[0].GetTypeUrl() +} diff --git a/app/ante/authz_guard.go b/app/ante/authz_guard.go new file mode 100644 index 000000000..9ceb8b240 --- /dev/null +++ b/app/ante/authz_guard.go @@ -0,0 +1,9 @@ +package ante + +// TODO: https://github.com/NibiruChain/nibiru/issues/1915 +// feat(ante): Add an authz guard to disable authz Ethereum txs and provide +// additional security around the default functionality exposed by the module. +// +// Implemenetation Notes +// UD-NOTE - IsAuthzMessage fn. Use authz import with module name +// UD-NOTE - Define set of disabled txMsgs diff --git a/app/ante/fixed_gas.go b/app/ante/fixed_gas.go index d1f4ca7ad..5d31b30b8 100644 --- a/app/ante/fixed_gas.go +++ b/app/ante/fixed_gas.go @@ -9,17 +9,14 @@ import ( const OracleMessageGas = 500 -var _ sdk.AnteDecorator = EnsureSinglePostPriceMessageDecorator{} +var _ sdk.AnteDecorator = AnteDecoratorEnsureSinglePostPriceMessage{} -// EnsureSinglePostPriceMessageDecorator ensures that there is only one oracle vote message in the transaction -// and sets the gas meter to a fixed value. -type EnsureSinglePostPriceMessageDecorator struct{} +// AnteDecoratorEnsureSinglePostPriceMessage ensures that there is only one +// oracle vote message in the transaction and sets the gas meter to a fixed +// value. +type AnteDecoratorEnsureSinglePostPriceMessage struct{} -func NewPostPriceFixedPriceDecorator() EnsureSinglePostPriceMessageDecorator { - return EnsureSinglePostPriceMessageDecorator{} -} - -func (gd EnsureSinglePostPriceMessageDecorator) AnteHandle( +func (gd AnteDecoratorEnsureSinglePostPriceMessage) AnteHandle( ctx sdk.Context, tx sdk.Tx, simulate bool, diff --git a/app/ante/testutil_test.go b/app/ante/testutil_test.go index 071415826..72d0747c5 100644 --- a/app/ante/testutil_test.go +++ b/app/ante/testutil_test.go @@ -66,7 +66,7 @@ func (suite *AnteTestSuite) SetupTest() { ante.NewValidateBasicDecorator(), ante.NewTxTimeoutHeightDecorator(), ante.NewValidateMemoDecorator(suite.app.AccountKeeper), - nibiruante.NewPostPriceFixedPriceDecorator(), + nibiruante.AnteDecoratorEnsureSinglePostPriceMessage{}, nibiruante.AnteDecoratorStakingCommission{}, ante.NewConsumeGasForTxSizeDecorator(suite.app.AccountKeeper), ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, suite.app.FeeGrantKeeper, nil), // Replace fee ante from cosmos auth with a custom one. diff --git a/app/evmante_fee_market.go b/app/evmante_fee_market.go index 22cff38f9..b7cebbfa3 100644 --- a/app/evmante_fee_market.go +++ b/app/evmante_fee_market.go @@ -9,23 +9,11 @@ import ( "github.com/NibiruChain/nibiru/eth" ) -// GasWantedDecorator keeps track of the gasWanted amount on the current block in transient store -// for BaseFee calculation. -// NOTE: This decorator does not perform any validation -type GasWantedDecorator struct { - AppKeepers -} - -// NewGasWantedDecorator creates a new NewGasWantedDecorator -func NewGasWantedDecorator( - k AppKeepers, -) GasWantedDecorator { - return GasWantedDecorator{ - AppKeepers: k, - } -} +// AnteDecoratorGasWanted keeps track of the gasWanted amount on the current block in +// transient store for BaseFee calculation. +type AnteDecoratorGasWanted struct{} -func (gwd GasWantedDecorator) AnteHandle( +func (gwd AnteDecoratorGasWanted) AnteHandle( ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler, ) (newCtx sdk.Context, err error) { feeTx, ok := tx.(sdk.FeeTx) diff --git a/app/evmante_handler.go b/app/evmante_handler.go index 5d0ce2740..74cbea3ac 100644 --- a/app/evmante_handler.go +++ b/app/evmante_handler.go @@ -22,7 +22,7 @@ func NewAnteHandlerEVM( NewCanTransferDecorator(k), NewAnteDecEthGasConsume(k, options.MaxTxGasWanted), NewAnteDecEthIncrementSenderSequence(k), - NewGasWantedDecorator(k), + AnteDecoratorGasWanted{}, // emit eth tx hash and index at the very last ante handler. NewEthEmitEventDecorator(k), ) diff --git a/app/evmante_test.go b/app/evmante_test.go index ef5306116..7d2ef2140 100644 --- a/app/evmante_test.go +++ b/app/evmante_test.go @@ -189,3 +189,6 @@ func (s *TestSuite) TestAnteDecEthGasConsume() { }) } } + +// func (s *TestSuite) TestAnteDecoratorVerifyEthAcc_CheckTx() { +// } diff --git a/x/common/testutil/cli/network.go b/x/common/testutil/cli/network.go index 8dbe769a5..6a477e48f 100644 --- a/x/common/testutil/cli/network.go +++ b/x/common/testutil/cli/network.go @@ -59,92 +59,90 @@ var lock = new(sync.Mutex) // creates an ABCI Application to provide to Tendermint. type AppConstructor = func(val Validator) servertypes.Application -type ( - // Network defines a in-process testing network. It is primarily intended - // for client and integration testing. The Network struct can spawn any - // number of validators, each with its own RPC and API clients. - // - // ### Constraints - // - // 1. Only the first validator will have a functional RPC and API - // server/client. - // 2. Due to constraints in Tendermint's JSON-RPC implementation, only one - // test network can run at a time. For this reason, it's essential to - // invoke `Network.Cleanup` after testing to allow other tests to create - // networks. - Network struct { - BaseDir string - Config Config - Validators []*Validator - Logger Logger - } +// Network defines a in-process testing network. It is primarily intended +// for client and integration testing. The Network struct can spawn any +// number of validators, each with its own RPC and API clients. +// +// ### Constraints +// +// 1. Only the first validator will have a functional RPC and API +// server/client. +// 2. Due to constraints in Tendermint's JSON-RPC implementation, only one +// test network can run at a time. For this reason, it's essential to +// invoke `Network.Cleanup` after testing to allow other tests to create +// networks. +type Network struct { + BaseDir string + Config Config + Validators []*Validator + Logger Logger +} - // Validator defines an in-process Tendermint validator node. Through this - // object, a client can make RPC and API calls and interact with any client - // command or handler. - Validator struct { - AppConfig *serverconfig.Config - ClientCtx client.Context - Ctx *server.Context - // Dir is the root directory of the validator node data and config. Passed to the Tendermint config. - Dir string - - // NodeID is a unique ID for the validator generated when the - // 'cli.Network' is started. - NodeID string - PubKey cryptotypes.PubKey - - // Moniker is a human-readable name that identifies a validator. A - // moniker is optional and may be empty. - Moniker string - - // APIAddress is the endpoint that the validator API server binds to. - // Only the first validator of a 'cli.Network' exposes the full API. - APIAddress string - - // RPCAddress is the endpoint that the RPC server binds to. Only the - // first validator of a 'cli.Network' exposes the full API. - RPCAddress string - - // P2PAddress is the endpoint that the RPC server binds to. The P2P - // server handles Tendermint peer-to-peer (P2P) networking and is - // critical for blockchain replication and consensus. It allows nodes - // to gossip blocks, transactions, and consensus messages. Only the - // first validator of a 'cli.Network' exposes the full API. - P2PAddress string - - // Address - account address - Address sdk.AccAddress - - // EthAddress - Ethereum address - EthAddress common.Address - - // ValAddress - validator operator (valoper) address - ValAddress sdk.ValAddress - - // RPCClient wraps most important rpc calls a client would make to - // listen for events, test if it also implements events.EventSwitch. - // - // RPCClient implementations in "github.com/cometbft/cometbft/rpc" v0.37.2: - // - rpc.HTTP - // - rpc.Local - RPCClient tmclient.Client - - JSONRPCClient *ethclient.Client - - tmNode *node.Node - - // API exposes the app's REST and gRPC interfaces, allowing clients to - // read from state and broadcast txs. The API server connects to the - // underlying ABCI application. - api *serverapi.Server - grpc *grpc.Server - grpcWeb *http.Server - secretMnemonic string - jsonrpc *http.Server - jsonrpcDone chan struct{} - } -) +// Validator defines an in-process Tendermint validator node. Through this +// object, a client can make RPC and API calls and interact with any client +// command or handler. +type Validator struct { + AppConfig *serverconfig.Config + ClientCtx client.Context + Ctx *server.Context + // Dir is the root directory of the validator node data and config. Passed to the Tendermint config. + Dir string + + // NodeID is a unique ID for the validator generated when the + // 'cli.Network' is started. + NodeID string + PubKey cryptotypes.PubKey + + // Moniker is a human-readable name that identifies a validator. A + // moniker is optional and may be empty. + Moniker string + + // APIAddress is the endpoint that the validator API server binds to. + // Only the first validator of a 'cli.Network' exposes the full API. + APIAddress string + + // RPCAddress is the endpoint that the RPC server binds to. Only the + // first validator of a 'cli.Network' exposes the full API. + RPCAddress string + + // P2PAddress is the endpoint that the RPC server binds to. The P2P + // server handles Tendermint peer-to-peer (P2P) networking and is + // critical for blockchain replication and consensus. It allows nodes + // to gossip blocks, transactions, and consensus messages. Only the + // first validator of a 'cli.Network' exposes the full API. + P2PAddress string + + // Address - account address + Address sdk.AccAddress + + // EthAddress - Ethereum address + EthAddress common.Address + + // ValAddress - validator operator (valoper) address + ValAddress sdk.ValAddress + + // RPCClient wraps most important rpc calls a client would make to + // listen for events, test if it also implements events.EventSwitch. + // + // RPCClient implementations in "github.com/cometbft/cometbft/rpc" v0.37.2: + // - rpc.HTTP + // - rpc.Local + RPCClient tmclient.Client + + JSONRPCClient *ethclient.Client + + tmNode *node.Node + + // API exposes the app's REST and gRPC interfaces, allowing clients to + // read from state and broadcast txs. The API server connects to the + // underlying ABCI application. + api *serverapi.Server + grpc *grpc.Server + grpcWeb *http.Server + secretMnemonic string + jsonrpc *http.Server + jsonrpcDone chan struct{} +} // NewAppConstructor returns a new simapp AppConstructor func NewAppConstructor(encodingCfg app.EncodingConfig, chainID string) AppConstructor { diff --git a/x/evm/keeper/grpc_query_test.go b/x/evm/keeper/grpc_query_test.go index 97d87d0dc..49c13a34a 100644 --- a/x/evm/keeper/grpc_query_test.go +++ b/x/evm/keeper/grpc_query_test.go @@ -19,8 +19,24 @@ import ( "github.com/NibiruChain/nibiru/x/evm/evmtest" ) +type TestCase[In, Out any] struct { + name string + // setup: Optional setup function to create the scenario + setup func(deps *evmtest.TestDeps) + scenario func(deps *evmtest.TestDeps) ( + req In, + wantResp Out, + ) + wantErr string +} + func InvalidEthAddr() string { return "0x0000" } +// TraceNibiTransfer returns a hardcoded JSON string representing the expected +// trace output of a successful "ether" (unibi) token transfer. +// Used to test the correctness of "TraceTx" and "TraceBlock". +// - Note that the struct logs are empty. That is because a simple token +// transfer does not involve contract operations. func TraceNibiTransfer() string { return fmt.Sprintf(`{ "gas": %d, @@ -30,6 +46,9 @@ func TraceNibiTransfer() string { }`, gethparams.TxGas) } +// TraceERC20Transfer returns a hardcoded JSON string representing the expected +// trace output of a successful ERC-20 token transfer (an EVM tx). +// Used to test the correctness of "TraceTx" and "TraceBlock". func TraceERC20Transfer() string { return `{ "gas": 35062, @@ -46,17 +65,6 @@ func TraceERC20Transfer() string { }` } -type TestCase[In, Out any] struct { - name string - // setup: Optional setup function to create the scenario - setup func(deps *evmtest.TestDeps) - scenario func(deps *evmtest.TestDeps) ( - req In, - wantResp Out, - ) - wantErr string -} - func (s *KeeperSuite) TestQueryNibiruAccount() { type In = *evm.QueryNibiruAccountRequest type Out = *evm.QueryNibiruAccountResponse