evm: code comments and cleanup (#404)

* evm: code comments and cleanup

* context comment

* abci, tx type event and metrics

* statedb comments

x/evm/keeper/abci.go

@@ -12,17 +12,15 @@ import (
 	"github.com/tharsis/ethermint/x/evm/types"
 )
 
-// BeginBlock sets the block hash -> block height map for the previous block height
+// BeginBlock sets the sdk Context and EIP155 chain id to the Keeper.
-// and resets the Bloom filter and the transaction count to 0.
 func (k *Keeper) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {
 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)
 	k.WithContext(ctx)
 	k.WithChainID(ctx)
 }
 
-// EndBlock updates the accounts and commits state objects to the KV Store, while
+// EndBlock also retrieves the bloom filter value from the transient store and commits it to the
-// deleting the empty ones. It also sets the bloom filers for the request block to
+// KVStore. The EVM end block logic doesn't update the validator set, thus it returns
-// the store. The EVM end block logic doesn't update the validator set, thus it returns
 // an empty slice.
 func (k *Keeper) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) []abci.ValidatorUpdate {
 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker)

x/evm/keeper/keeper.go

@@ -17,33 +17,42 @@ import (
 	"github.com/tharsis/ethermint/x/evm/types"
 )
 
-// Keeper wraps the CommitStateDB, allowing us to pass in SDK context while adhering
+// Keeper grants access to the EVM module state and implements the go-ethereum StateDB interface.
-// to the StateDB interface.
 type Keeper struct {
 	// Protobuf codec
 	cdc codec.BinaryCodec
 	// Store key required for the EVM Prefix KVStore. It is required by:
-	// - storing Account's Storage State
+	// - storing account's Storage State
-	// - storing Account's Code
+	// - storing account's Code
 	// - storing transaction Logs
-	// - storing block height -> bloom filter map. Needed for the Web3 API.
+	// - storing Bloom filters by block height. Needed for the Web3 API.
 	storeKey sdk.StoreKey
 
 	// key to access the transient store, which is reset on every block during Commit
 	transientKey sdk.StoreKey
 
-	paramSpace    paramtypes.Subspace
+	// module specific parameter space that can be configured through governance
+	paramSpace paramtypes.Subspace
+	// access to account state
 	accountKeeper types.AccountKeeper
-	bankKeeper    types.BankKeeper
+	// update balance and accounting operations with coins
+	bankKeeper types.BankKeeper
+	// access historical headers for EVM state transition execution
 	stakingKeeper types.StakingKeeper
 
+	// Context for accessing the store, emit events and log info.
+	// It is kept as a field to make is accessible by the StateDb
+	// functions. Resets on every transaction/block.
 	ctx sdk.Context
-	// set in `BeginCachedContext`, used by `GetCommittedState` api.
+	// Context of the committed state (before transaction execution).
+	// Required for StateDB.CommitedState. Set in `BeginCachedContext`.
 	committedCtx sdk.Context
 
 	// chain ID number obtained from the context's chain id
 	eip155ChainID *big.Int
-	debug         bool
+	// trace EVM state transition execution. This value is obtained from the `--trace` flag.
+	// For more info check https://geth.ethereum.org/docs/dapp/tracing
+	debug bool
 }
 
 // NewKeeper generates new evm module keeper

x/evm/keeper/msg_server.go

@@ -2,6 +2,7 @@ package keeper
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/armon/go-metrics"
@@ -18,6 +19,10 @@ import (
 
 var _ types.MsgServer = &Keeper{}
 
+// EthereumTx implements the gRPC MsgServer interface. It receives a transaction which is then
+// executed (i.e applied) against the go-ethereum EVM. The provided SDK Context is set to the Keeper
+// so that it can implements and call the StateDB methods without receiving it as a function
+// parameter.
 func (k *Keeper) EthereumTx(goCtx context.Context, msg *types.MsgEthereumTx) (*types.MsgEthereumTxResponse, error) {
 	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), types.TypeMsgEthereumTx)
 
@@ -27,7 +32,7 @@ func (k *Keeper) EthereumTx(goCtx context.Context, msg *types.MsgEthereumTx) (*t
 	sender := msg.From
 	tx := msg.AsTransaction()
 
-	var labels []metrics.Label
+	labels := []metrics.Label{telemetry.NewLabel("tx_type", fmt.Sprintf("%d", tx.Type()))}
 	if tx.To() == nil {
 		labels = []metrics.Label{
 			telemetry.NewLabel("execution", "create"),
@@ -89,6 +94,7 @@ func (k *Keeper) EthereumTx(goCtx context.Context, msg *types.MsgEthereumTx) (*t
 			sdk.EventTypeMessage,
 			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
 			sdk.NewAttribute(sdk.AttributeKeySender, sender),
+			sdk.NewAttribute(types.AttributeKeyTxType, fmt.Sprintf("%d", tx.Type())),
 		),
 	})
 

x/evm/keeper/state_transition.go

@@ -24,9 +24,10 @@ import (
 	"github.com/ethereum/go-ethereum/params"
 )
 
-// NewEVM generates an ethereum VM from the provided Message fields and the chain parameters
+// NewEVM generates a go-ethereum VM from the provided Message fields and the chain parameters
-// (config). It sets the validator operator address as the coinbase address to make it available for
+// (ChainConfig and module Params). It additionally sets the validator operator address as the
-// the COINBASE opcode, even though there is no beneficiary (since we're not mining).
+// coinbase address to make it available for the COINBASE opcode, even though there is no
+// beneficiary of the coinbase transaction (since we're not mining).
 func (k *Keeper) NewEVM(msg core.Message, config *params.ChainConfig, params types.Params, coinbase common.Address) *vm.EVM {
 	blockCtx := vm.BlockContext{
 		CanTransfer: core.CanTransfer,
@@ -45,8 +46,8 @@ func (k *Keeper) NewEVM(msg core.Message, config *params.ChainConfig, params typ
 	return vm.NewEVM(blockCtx, txCtx, k, config, vmConfig)
 }
 
-// VMConfig creates an EVM configuration from the module parameters and the debug setting.
+// VMConfig creates an EVM configuration from the debug setting and the extra EIPs enabled on the
-// The config generated uses the default JumpTable from the EVM.
+// module parameters. The config generated uses the default JumpTable from the EVM.
 func (k Keeper) VMConfig(params types.Params) vm.Config {
 	return vm.Config{
 		Debug:       k.debug,
@@ -73,13 +74,14 @@ func (k Keeper) GetHashFn() vm.GetHashFunc {
 				return common.BytesToHash(headerHash)
 			}
 
-			// only recompute the hash if not set
+			// only recompute the hash if not set (eg: checkTxState)
 			contextBlockHeader := k.ctx.BlockHeader()
 			header, err := tmtypes.HeaderFromProto(&contextBlockHeader)
 			if err != nil {
 				k.Logger(k.ctx).Error("failed to cast tendermint header from proto", "error", err)
 				return common.Hash{}
 			}
+
 			headerHash = header.Hash()
 			return common.BytesToHash(headerHash)
 
@@ -107,7 +109,7 @@ func (k Keeper) GetHashFn() vm.GetHashFunc {
 }
 
 // ApplyTransaction runs and attempts to perform a state transition with the given transaction (i.e Message), that will
-// only be persisted to the underlying KVStore if the transaction does not error.
+// only be persisted (committed) to the underlying KVStore if the transaction does not fail.
 //
 // Gas tracking
 //
@@ -149,15 +151,19 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT
 	// create an ethereum EVM instance and run the message
 	evm := k.NewEVM(msg, ethCfg, params, coinbase)
 
-	k.SetTxHashTransient(tx.Hash())
+	txHash := tx.Hash()
+
+	// set the transaction hash and index to the impermanent (transient) block state so that it's also
+	// available on the StateDB functions (eg: AddLog)
+	k.SetTxHashTransient(txHash)
 	k.IncreaseTxIndexTransient()
+
 	// pass false to execute in real mode, which do actual gas refunding
 	res, err := k.ApplyMessage(evm, msg, ethCfg, false)
 	if err != nil {
 		return nil, stacktrace.Propagate(err, "failed to apply ethereum core message")
 	}
 
-	txHash := tx.Hash()
 	res.Hash = txHash.Hex()
 	logs := k.GetTxLogs(txHash)
 
@@ -226,7 +232,7 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT
 //
 // Query mode
 //
-// The grpc query endpoint EthCall calls this in query mode, and since the query handler don't call AnteHandler,
+// The gRPC query endpoint from 'eth_call' calls this method in query mode, and since the query handler don't call AnteHandler,
 // so we don't do real gas refund in that case.
 func (k *Keeper) ApplyMessage(evm *vm.EVM, msg core.Message, cfg *params.ChainConfig, query bool) (*types.MsgEthereumTxResponse, error) {
 	var (
@@ -256,17 +262,19 @@ func (k *Keeper) ApplyMessage(evm *vm.EVM, msg core.Message, cfg *params.ChainCo
 	}
 
 	if query {
-		// query handlers don't call ante handler to deduct gas fee, so don't do actual refund here, because the
+		// gRPC query handlers don't go through the AnteHandler to deduct the gas fee from the sender or have access historical state.
-		// module account balance might not be enough
+		// We don't refund gas to the sender.
+		// For more info, see: https://github.com/tharsis/ethermint/issues/229 and https://github.com/cosmos/cosmos-sdk/issues/9636
 		leftoverGas += k.GasToRefund(msg.Gas() - leftoverGas)
 	} else {
-		// refund gas prior to handling the vm error in order to set the updated gas meter
+		// refund gas prior to handling the vm error in order to match the Ethereum gas consumption instead of the default SDK one.
 		leftoverGas, err = k.RefundGas(msg, leftoverGas)
 		if err != nil {
 			return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From())
 		}
 	}
 
+	// EVM execution error needs to be available for the JSON-RPC client
 	var vmError string
 	if vmErr != nil {
 		vmError = vmErr.Error()
@@ -280,7 +288,7 @@ func (k *Keeper) ApplyMessage(evm *vm.EVM, msg core.Message, cfg *params.ChainCo
 	}, nil
 }
 
-// GetEthIntrinsicGas get the transaction intrinsic gas cost
+// GetEthIntrinsicGas returns the intrinsic gas cost for the transaction
 func (k *Keeper) GetEthIntrinsicGas(msg core.Message, cfg *params.ChainConfig, isContractCreation bool) (uint64, error) {
 	height := big.NewInt(k.ctx.BlockHeight())
 	homestead := cfg.IsHomestead(height)
@@ -289,7 +297,8 @@ func (k *Keeper) GetEthIntrinsicGas(msg core.Message, cfg *params.ChainConfig, i
 	return core.IntrinsicGas(msg.Data(), msg.AccessList(), isContractCreation, homestead, istanbul)
 }
 
-// GasToRefund calculate the amount of gas should refund to sender
+// GasToRefund calculates the amount of gas the state machine should refund to the sender. It is
+// capped by half of the gas consumed.
 func (k *Keeper) GasToRefund(gasConsumed uint64) uint64 {
 	// Apply refund counter, capped to half of the used gas.
 	refund := gasConsumed / 2
@@ -305,6 +314,7 @@ func (k *Keeper) GasToRefund(gasConsumed uint64) uint64 {
 // returned by the EVM execution, thus ignoring the previous intrinsic gas consumed during in the
 // AnteHandler.
 func (k *Keeper) RefundGas(msg core.Message, leftoverGas uint64) (uint64, error) {
+	// safety check: leftover gas after execution should never exceed the gas limit defined on the message
 	if leftoverGas > msg.Gas() {
 		return leftoverGas, stacktrace.Propagate(
 			sdkerrors.Wrapf(types.ErrInconsistentGas, "leftover gas cannot be greater than gas limit (%d > %d)", leftoverGas, msg.Gas()),
@@ -313,10 +323,12 @@ func (k *Keeper) RefundGas(msg core.Message, leftoverGas uint64) (uint64, error)
 	}
 
 	gasConsumed := msg.Gas() - leftoverGas
-	refund := k.GasToRefund(gasConsumed)
 
+	// calculate available gas to refund and add it to the leftover gas amount
+	refund := k.GasToRefund(gasConsumed)
 	leftoverGas += refund
 
+	// safety check: leftover gas after refund should never exceed the gas limit defined on the message
 	if leftoverGas > msg.Gas() {
 		return leftoverGas, stacktrace.Propagate(
 			sdkerrors.Wrapf(types.ErrInconsistentGas, "leftover gas cannot be greater than gas limit (%d > %d)", leftoverGas, msg.Gas()),

x/evm/keeper/statedb.go

@@ -24,7 +24,9 @@ var _ vm.StateDB = &Keeper{}
 // ----------------------------------------------------------------------------
 
 // CreateAccount creates a new EthAccount instance from the provided address and
-// sets the value to store.
+// sets the value to store. If an account with the given address already exists,
+// this function also resets any preexisting code and storage associated with that
+// address.
 func (k *Keeper) CreateAccount(addr common.Address) {
 	cosmosAddr := sdk.AccAddress(addr.Bytes())
 
@@ -51,7 +53,9 @@ func (k *Keeper) CreateAccount(addr common.Address) {
 // Balance
 // ----------------------------------------------------------------------------
 
-// AddBalance calls CommitStateDB.AddBalance using the passed in context
+// AddBalance adds the given amount to the address balance coin by minting new
+// coins and transferring them to the address. The coin denomination is obtained
+// from the module parameters.
 func (k *Keeper) AddBalance(addr common.Address, amount *big.Int) {
 	if amount.Sign() != 1 {
 		k.Logger(k.ctx).Debug(
@@ -94,7 +98,10 @@ func (k *Keeper) AddBalance(addr common.Address, amount *big.Int) {
 	)
 }
 
-// SubBalance calls CommitStateDB.SubBalance using the passed in context
+// SubBalance subtracts the given amount from the address balance by transferring the
+// coins to an escrow account and then burning them. The coin denomination is obtained
+// from the module parameters. This function performs a no-op if the amount is negative
+// or the user doesn't have enough funds for the transfer.
 func (k *Keeper) SubBalance(addr common.Address, amount *big.Int) {
 	if amount.Sign() != 1 {
 		k.Logger(k.ctx).Debug(
@@ -111,7 +118,7 @@ func (k *Keeper) SubBalance(addr common.Address, amount *big.Int) {
 	coins := sdk.Coins{sdk.NewCoin(params.EvmDenom, sdk.NewIntFromBigInt(amount))}
 
 	if err := k.bankKeeper.SendCoinsFromAccountToModule(k.ctx, cosmosAddr, types.ModuleName, coins); err != nil {
-		k.Logger(k.ctx).Error(
+		k.Logger(k.ctx).Debug(
 			"failed to send from account to module when subtracting balance",
 			"ethereum-address", addr.Hex(),
 			"cosmos-address", cosmosAddr.String(),
@@ -138,7 +145,8 @@ func (k *Keeper) SubBalance(addr common.Address, amount *big.Int) {
 	)
 }
 
-// GetBalance calls CommitStateDB.GetBalance using the passed in context
+// GetBalance returns the EVM denomination balance of the provided address. The
+// denomination is obtained from the module parameters.
 func (k *Keeper) GetBalance(addr common.Address) *big.Int {
 	cosmosAddr := sdk.AccAddress(addr.Bytes())
 	params := k.GetParams(k.ctx)
@@ -151,7 +159,8 @@ func (k *Keeper) GetBalance(addr common.Address) *big.Int {
 // Nonce
 // ----------------------------------------------------------------------------
 
-// GetNonce calls CommitStateDB.GetNonce using the passed in context
+// GetNonce retrieves the account with the given address and returns the tx
+// sequence (i.e nonce). The function performs a no-op if the account is not found.
 func (k *Keeper) GetNonce(addr common.Address) uint64 {
 	cosmosAddr := sdk.AccAddress(addr.Bytes())
 	nonce, err := k.accountKeeper.GetSequence(k.ctx, cosmosAddr)
@@ -209,7 +218,8 @@ func (k *Keeper) SetNonce(addr common.Address, nonce uint64) {
 // Code
 // ----------------------------------------------------------------------------
 
-// GetCodeHash calls CommitStateDB.GetCodeHash using the passed in context
+// GetCodeHash fetches the account from the store and returns its code hash. If the account doesn't
+// exist or is not an EthAccount type, GetCodeHash returns the empty code hash value.
 func (k *Keeper) GetCodeHash(addr common.Address) common.Hash {
 	cosmosAddr := sdk.AccAddress(addr.Bytes())
 	account := k.accountKeeper.GetAccount(k.ctx, cosmosAddr)
@@ -225,7 +235,8 @@ func (k *Keeper) GetCodeHash(addr common.Address) common.Hash {
 	return common.HexToHash(ethAccount.CodeHash)
 }
 
-// GetCode calls CommitStateDB.GetCode using the passed in context
+// GetCode returns the code byte array associated with the given address.
+// If the code hash from the account is empty, this function returns nil.
 func (k *Keeper) GetCode(addr common.Address) []byte {
 	hash := k.GetCodeHash(addr)
 
@@ -247,7 +258,8 @@ func (k *Keeper) GetCode(addr common.Address) []byte {
 	return code
 }
 
-// SetCode calls CommitStateDB.SetCode using the passed in context
+// SetCode stores the code byte array to the application KVStore and sets the
+// code hash to the given account. The code is deleted from the store if it is empty.
 func (k *Keeper) SetCode(addr common.Address, code []byte) {
 	if bytes.Equal(code, types.EmptyCodeHash) {
 		k.Logger(k.ctx).Debug("passed in EmptyCodeHash, but expected empty code")
@@ -309,7 +321,7 @@ func (k *Keeper) GetCodeSize(addr common.Address) int {
 // execution has finalized. The refund value is cleared on every transaction and
 // at the end of every block.
 
-// AddRefund adds the given amount of gas to the refund cached value.
+// AddRefund adds the given amount of gas to the refund transient value.
 func (k *Keeper) AddRefund(gas uint64) {
 	refund := k.GetRefund()
 
@@ -319,7 +331,7 @@ func (k *Keeper) AddRefund(gas uint64) {
 	store.Set(types.KeyPrefixTransientRefund, sdk.Uint64ToBigEndian(refund))
 }
 
-// SubRefund subtracts the given amount of gas from the refund value. This function
+// SubRefund subtracts the given amount of gas from the transient refund value. This function
 // will panic if gas amount is greater than the stored refund.
 func (k *Keeper) SubRefund(gas uint64) {
 	refund := k.GetRefund()
@@ -336,7 +348,7 @@ func (k *Keeper) SubRefund(gas uint64) {
 }
 
 // GetRefund returns the amount of gas available for return after the tx execution
-// finalises. This value is reset to 0 on every transaction.
+// finalizes. This value is reset to 0 on every transaction.
 func (k *Keeper) GetRefund() uint64 {
 	store := k.ctx.TransientStore(k.transientKey)
 
@@ -622,7 +634,8 @@ func (k *Keeper) AddPreimage(_ common.Hash, _ []byte) {}
 // Iterator
 // ----------------------------------------------------------------------------
 
-// ForEachStorage calls CommitStateDB.ForEachStorage using passed in context
+// ForEachStorage uses the store iterator to iterate over all the state keys and perform a callback
+// function on each of them.
 func (k *Keeper) ForEachStorage(addr common.Address, cb func(key, value common.Hash) bool) error {
 	store := k.ctx.KVStore(k.storeKey)
 	prefix := types.AddressStoragePrefix(addr)

x/evm/types/events.go

@@ -8,6 +8,7 @@ const (
 	AttributeKeyRecipient       = "recipient"
 	AttributeKeyTxHash          = "txHash"
 	AttributeKeyEthereumTxHash  = "ethereumTxHash"
+	AttributeKeyTxType          = "txType"
 	// tx failed in eth vm execution
 	AttributeKeyEthereumTxFailed = "ethereumTxFailed"
 	AttributeValueCategory       = ModuleName

x/evm/types/key.go

@@ -30,9 +30,6 @@ const (
 	prefixLogs
 	prefixCode
 	prefixStorage
-	prefixChainConfig
-	prefixHashTxReceipt
-	prefixBlockHeightTxs
 )
 
 const (
@@ -53,9 +50,6 @@ var (
 	KeyPrefixLogs               = []byte{prefixLogs}
 	KeyPrefixCode               = []byte{prefixCode}
 	KeyPrefixStorage            = []byte{prefixStorage}
-	KeyPrefixChainConfig        = []byte{prefixChainConfig}
-	KeyPrefixHashTxReceipt      = []byte{prefixHashTxReceipt}
-	KeyPrefixBlockHeightTxs     = []byte{prefixBlockHeightTxs}
 )
 
 var (
@@ -85,17 +79,6 @@ func StateKey(address ethcmn.Address, key []byte) []byte {
 	return append(AddressStoragePrefix(address), key...)
 }
 
-// KeyHashTxReceipt returns a key for accessing tx receipt data by hash.
-func KeyHashTxReceipt(hash ethcmn.Hash) []byte {
-	return append(KeyPrefixHashTxReceipt, hash.Bytes()...)
-}
-
-// KeyBlockHeightTxs returns a key for accessing tx hash list by block height.
-func KeyBlockHeightTxs(height uint64) []byte {
-	heightBytes := sdk.Uint64ToBigEndian(height)
-	return append(KeyPrefixBlockHeightTxs, heightBytes...)
-}
-
 // KeyAddressStorage returns the key hash to access a given account state. The composite key
 // (address + hash) is hashed using Keccak256.
 func KeyAddressStorage(address ethcmn.Address, hash ethcmn.Hash) ethcmn.Hash {