fix(evm): proper tx gas refund outside of CallContract

CHANGELOG.md

@@ -54,6 +54,7 @@ documentation.
 - [#2127](https://github.com/NibiruChain/nibiru/pull/2127) - fix(vesting): disabled built in auth/vesting module functionality
 - [#2125](https://github.com/NibiruChain/nibiru/pull/2125) - feat(evm-precompile): Emit EVM events created to reflect the ABCI events that occur outside the EVM to make sure that block explorers and indexers can find indexed ABCI event information.
 - [#2129](https://github.com/NibiruChain/nibiru/pull/2129) - fix(evm): issue with infinite recursion in erc20 funtoken contracts
+- [#2132](https://github.com/NibiruChain/nibiru/pull/2132) - fix(evm): proper tx gas refund
 - [#2134](https://github.com/NibiruChain/nibiru/pull/2134) - fix(evm): query of NIBI should use bank state, not the StateDB
 - [#2140](https://github.com/NibiruChain/nibiru/pull/2140) - fix(bank): bank keeper extension now charges gas for the bank operations
 - [#2141](https://github.com/NibiruChain/nibiru/pull/2141) - refactor: simplify account retrieval operation in `nibid q evm account`.

eth/rpc/backend/backend_suite_test.go

@@ -10,6 +10,7 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	gethcommon "github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/common/hexutil"
 	gethcore "github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/crypto"
 	"github.com/ethereum/go-ethereum/params"
@@ -94,7 +95,7 @@ func (s *BackendSuite) SetupSuite() {
 	// Send Transfer TX and use the results in the tests
 	s.Require().NoError(err)
 	transferTxHash = s.SendNibiViaEthTransfer(recipient, amountToSend, true)
-	blockNumber, blockHash := WaitForReceipt(s, transferTxHash)
+	blockNumber, blockHash, _ := WaitForReceipt(s, transferTxHash)
 	s.Require().NotNil(blockNumber)
 	s.Require().NotNil(blockHash)
 	transferTxBlockNumber = rpc.NewBlockNumber(blockNumber)
@@ -103,7 +104,7 @@ func (s *BackendSuite) SetupSuite() {
 	// Deploy test erc20 contract
 	deployContractTxHash, contractAddress := s.DeployTestContract(true)
 	testContractAddress = contractAddress
-	blockNumber, blockHash = WaitForReceipt(s, deployContractTxHash)
+	blockNumber, blockHash, _ = WaitForReceipt(s, deployContractTxHash)
 	s.Require().NotNil(blockNumber)
 	s.Require().NotNil(blockHash)
 	deployContractBlockNumber = rpc.NewBlockNumber(blockNumber)
@@ -167,24 +168,33 @@ func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bo
 	return txHash
 }
 
-func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash) {
+func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
 	for {
 		receipt, err := s.backend.GetTransactionReceipt(txHash)
 		if err != nil {
-			return nil, nil
+			return nil, nil, nil
 		}
 		if receipt != nil {
-			return receipt.BlockNumber, &receipt.BlockHash
+			return receipt.BlockNumber, &receipt.BlockHash, receipt
 		}
 		select {
 		case <-ctx.Done():
 			fmt.Println("Timeout reached, transaction not included in a block yet.")
-			return nil, nil
+			return nil, nil, nil
 		default:
 			time.Sleep(1 * time.Second)
 		}
 	}
 }
+
+// broadcastSDKTx broadcasts the given SDK transaction and returns the response
+func (s *BackendSuite) getUnibiBalance(address gethcommon.Address) *hexutil.Big {
+	latestBlock := rpc.EthLatestBlockNumber
+	latestBlockOrHash := rpc.BlockNumberOrHash{BlockNumber: &latestBlock}
+	balance, err := s.backend.GetBalance(address, latestBlockOrHash)
+	s.Require().NoError(err)
+	return balance
+}

eth/rpc/backend/call_tx.go

@@ -9,6 +9,7 @@
 	"math/big"
 
 	errorsmod "cosmossdk.io/errors"
+	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/ethereum/go-ethereum/common"
@@ -338,3 +339,7 @@
 
 	return (*hexutil.Big)(result), nil
 }
+
+func (b *Backend) ClientCtx() client.Context {
+	return b.clientCtx
+}

eth/rpc/backend/gas_used_test.go

@@ -0,0 +1,180 @@
+package backend_test
+
+import (
+	"math/big"
+
+	"github.com/ethereum/go-ethereum/common/hexutil"
+	gethcore "github.com/ethereum/go-ethereum/core/types"
+
+	"github.com/NibiruChain/nibiru/v2/eth"
+	"github.com/NibiruChain/nibiru/v2/eth/rpc"
+	"github.com/NibiruChain/nibiru/v2/x/common/testutil"
+	"github.com/NibiruChain/nibiru/v2/x/evm"
+	"github.com/NibiruChain/nibiru/v2/x/evm/embeds"
+	"github.com/NibiruChain/nibiru/v2/x/evm/evmtest"
+	"github.com/NibiruChain/nibiru/v2/x/evm/precompile"
+)
+
+// TestGasUsedTransfers verifies that gas used is correctly calculated for simple transfers.
+// Test creates 2 eth transfer txs that are supposed to be included in the same block.
+// It checks that gas used is the same for both txs and the total block gas is greater than the sum of 2 gas used.
+func (s *BackendSuite) TestGasUsedTransfers() {
+	// Start with new block
+	s.Require().NoError(s.network.WaitForNextBlock())
+	balanceBefore := s.getUnibiBalance(s.fundedAccEthAddr)
+
+	// Send 2 similar transfers
+	randomEthAddr := evmtest.NewEthPrivAcc().EthAddr
+	txHash1 := s.SendNibiViaEthTransfer(randomEthAddr, amountToSend, false)
+	txHash2 := s.SendNibiViaEthTransfer(randomEthAddr, amountToSend, false)
+
+	blockNumber1, _, receipt1 := WaitForReceipt(s, txHash1)
+	blockNumber2, _, receipt2 := WaitForReceipt(s, txHash2)
+
+	s.Require().NotNil(receipt1)
+	s.Require().NotNil(receipt2)
+
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt1.Status)
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt2.Status)
+
+	// Expect txs are included into one block
+	s.Require().Equal(blockNumber1, blockNumber2)
+
+	// Ensure that gas used is the same for both transactions
+	s.Require().Equal(receipt1.GasUsed, receipt2.GasUsed)
+
+	// Get block receipt and check gas used
+	block, err := s.backend.GetBlockByNumber(rpc.NewBlockNumber(blockNumber1), false)
+	s.Require().NoError(err)
+	s.Require().NotNil(block)
+	s.Require().NotNil(block["gasUsed"])
+	s.Require().GreaterOrEqual(block["gasUsed"].(*hexutil.Big).ToInt().Uint64(), receipt1.GasUsed+receipt2.GasUsed)
+
+	// Balance after should be equal to balance before minus gas used and amount sent
+	balanceAfter := s.getUnibiBalance(s.fundedAccEthAddr)
+	s.Require().Equal(
+		receipt1.GasUsed+receipt2.GasUsed+2,
+		evm.WeiToNative(balanceBefore.ToInt()).Uint64()-evm.WeiToNative(balanceAfter.ToInt()).Uint64(),
+	)
+}
+
+// TestGasUsedFunTokens verifies that gas used is correctly calculated for precompile "sendToBank" txs.
+// Test creates 3 txs: 2 successful and one failing.
+// Successful txs gas should be refunded and failing tx should consume 100% of the gas limit.
+// It also checks that txs are included in the same block and block gas is greater or equals
+// to the total gas used by txs.
+func (s *BackendSuite) TestGasUsedFunTokens() {
+	// Create funtoken from erc20
+	erc20Addr, err := eth.NewEIP55AddrFromStr(testContractAddress.String())
+	s.Require().NoError(err)
+
+	_, err = s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
+	s.Require().NoError(err)
+
+	txResp, err := s.network.BroadcastMsgs(s.node.Address, &evm.MsgCreateFunToken{
+		Sender:    s.node.Address.String(),
+		FromErc20: &erc20Addr,
+	})
+	s.Require().NoError(err)
+	s.Require().NotNil(txResp)
+	s.Require().NoError(s.network.WaitForNextBlock())
+
+	randomNibiAddress := testutil.AccAddress()
+	packedArgsPass, err := embeds.SmartContract_FunToken.ABI.Pack(
+		"sendToBank",
+		erc20Addr.Address,
+		big.NewInt(1),
+		randomNibiAddress.String(),
+	)
+	s.Require().NoError(err)
+
+	nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
+	s.Require().NoError(err)
+
+	balanceBefore := s.getUnibiBalance(s.fundedAccEthAddr)
+
+	txHash1 := SendTransaction(
+		s,
+		&gethcore.LegacyTx{
+			Nonce:    uint64(*nonce),
+			To:       &precompile.PrecompileAddr_FunToken,
+			Data:     packedArgsPass,
+			Gas:      1_500_000,
+			GasPrice: big.NewInt(1),
+		},
+		false,
+	)
+
+	packedArgsFail, err := embeds.SmartContract_FunToken.ABI.Pack(
+		"sendToBank",
+		erc20Addr.Address,
+		big.NewInt(1),
+		"invalidAddress",
+	)
+	s.Require().NoError(err)
+	txHash2 := SendTransaction( // should fail due to invalid recipient address
+		s,
+		&gethcore.LegacyTx{
+			Nonce:    uint64(*nonce + 1),
+			To:       &precompile.PrecompileAddr_FunToken,
+			Data:     packedArgsFail,
+			Gas:      1_500_000,
+			GasPrice: big.NewInt(1),
+		},
+		false,
+	)
+	txHash3 := SendTransaction(
+		s,
+		&gethcore.LegacyTx{
+			Nonce:    uint64(*nonce + 2),
+			To:       &precompile.PrecompileAddr_FunToken,
+			Data:     packedArgsPass,
+			Gas:      1_500_000,
+			GasPrice: big.NewInt(1),
+		},
+		false,
+	)
+	blockNumber1, _, receipt1 := WaitForReceipt(s, txHash1)
+	blockNumber2, _, receipt2 := WaitForReceipt(s, txHash2)
+	blockNumber3, _, receipt3 := WaitForReceipt(s, txHash3)
+
+	s.Require().NotNil(receipt1)
+	s.Require().NotNil(receipt2)
+	s.Require().NotNil(receipt3)
+
+	s.Require().NotNil(blockNumber1)
+	s.Require().NotNil(blockNumber2)
+	s.Require().NotNil(blockNumber3)
+
+	// TXs should have been included in the same block
+	s.Require().Equal(blockNumber1, blockNumber2)
+	s.Require().Equal(blockNumber2, blockNumber3)
+
+	// 1 and 3 should pass and 2 should fail
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt1.Status)
+	s.Require().Equal(gethcore.ReceiptStatusFailed, receipt2.Status)
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt3.Status)
+
+	// TX 1 and 3 should have gas used lower than specified gas limit
+	s.Require().Less(receipt1.GasUsed, uint64(500_000))
+	s.Require().Less(receipt3.GasUsed, uint64(500_000))
+
+	// TX 2 should have gas used equal to specified gas limit as it failed
+	s.Require().Equal(uint64(1_500_000), receipt2.GasUsed)
+
+	block, err := s.backend.GetBlockByNumber(rpc.NewBlockNumber(blockNumber1), false)
+	s.Require().NoError(err)
+	s.Require().NotNil(block)
+	s.Require().NotNil(block["gasUsed"])
+	s.Require().GreaterOrEqual(
+		block["gasUsed"].(*hexutil.Big).ToInt().Uint64(),
+		receipt1.GasUsed+receipt2.GasUsed+receipt3.GasUsed,
+	)
+
+	// Balance after should be equal to balance before minus gas used
+	balanceAfter := s.getUnibiBalance(s.fundedAccEthAddr)
+	s.Require().Equal(
+		receipt1.GasUsed+receipt2.GasUsed+receipt3.GasUsed,
+		evm.WeiToNative(balanceBefore.ToInt()).Uint64()-evm.WeiToNative(balanceAfter.ToInt()).Uint64(),
+	)
+}

x/evm/keeper/call_contract.go

@@ -108,14 +108,11 @@ func (k Keeper) CallContractWithInput(
 		ctx, evmMsg, evm.NewNoOpTracer(), commit, evmCfg, txConfig, true,
 	)
 	if err != nil {
-		// We don't know the actual gas used, so consuming the gas limit
-		k.ResetGasMeterAndConsumeGas(ctx, gasLimit)
 		err = errors.Wrap(err, "failed to apply ethereum core message")
 		return
 	}
 
 	if evmResp.Failed() {
-		k.ResetGasMeterAndConsumeGas(ctx, evmResp.GasUsed)
 		if strings.Contains(evmResp.VmError, vm.ErrOutOfGas.Error()) {
 			err = fmt.Errorf("gas required exceeds allowance (%d)", gasLimit)
 			return
@@ -130,12 +127,6 @@ func (k Keeper) CallContractWithInput(
 
 	// Success, update block gas used and bloom filter
 	if commit {
-		blockGasUsed, err := k.AddToBlockGasUsed(ctx, evmResp.GasUsed)
-		if err != nil {
-			k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit())
-			return nil, nil, errors.Wrap(err, "error adding transient gas used to block")
-		}
-		k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed)
 		k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex))
 		// TODO: remove after migrating logs
 		//err = k.EmitLogEvents(ctx, evmResp)

x/evm/keeper/funtoken_from_coin.go

@@ -78,12 +78,18 @@
 	bytecodeForCall := append(embeds.SmartContract_ERC20Minter.Bytecode, packedArgs...)
 
 	// nil address for contract creation
-	_, _, err = k.CallContractWithInput(
+	evmResp, _, err := k.CallContractWithInput(
 		ctx, evm.EVM_MODULE_ADDRESS, nil, true, bytecodeForCall, Erc20GasLimitDeploy,
 	)
 	if err != nil {
+		k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit())
 		return gethcommon.Address{}, errors.Wrap(err, "failed to deploy ERC20 contract")
 	}
+	blockGasUsed, errBlockGasUsed := k.AddToBlockGasUsed(ctx, evmResp.GasUsed)
+	if errBlockGasUsed != nil {
+		return gethcommon.Address{}, errors.Wrap(errBlockGasUsed, "error adding transient gas used")
+	}
+	k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed)
 
 	return erc20Addr, nil
 }

x/evm/keeper/funtoken_from_coin_test.go

@@ -255,6 +255,8 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
 	s.Require().NoError(err)
 	s.Require().Equal("0", balance.String())
 
+	deps.ResetGasMeter()
+
 	s.T().Log("sad: Convert more erc-20 to back to bank coin, insufficient funds")
 	_, err = deps.EvmKeeper.CallContract(
 		deps.Ctx,

x/evm/keeper/keeper.go

@@ -104,7 +104,7 @@
 ) (blockGasUsed uint64, err error) {
 	// Either k.EvmState.BlockGasUsed.GetOr() or k.EvmState.BlockGasUsed.Set()
 	// also consume gas and could panic.
-	defer HandleOutOfGasPanic(&err, "")
+	defer HandleOutOfGasPanic(&err, "")()
 
 	blockGasUsed = k.EvmState.BlockGasUsed.GetOr(ctx, 0) + gasUsed
 	if blockGasUsed < gasUsed {
@@ -147,6 +147,7 @@
 		if r := recover(); r != nil {
 			switch r.(type) {
 			case sdk.ErrorOutOfGas:
+
 				*err = vm.ErrOutOfGas
 			default:
 				panic(r)

x/evm/keeper/msg_server.go

@@ -72,22 +72,21 @@
 
 	k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex))
 
-	blockGasUsed, err := k.AddToBlockGasUsed(ctx, evmResp.GasUsed)
-	if err != nil {
-		return nil, errors.Wrap(err, "error adding transient gas used to block")
-	}
-
 	// refund gas in order to match the Ethereum gas consumption instead of the
 	// default SDK one.
 	refundGas := uint64(0)
-	if evmMsg.Gas() > blockGasUsed {
-		refundGas = evmMsg.Gas() - blockGasUsed
+	if evmMsg.Gas() > evmResp.GasUsed {
+		refundGas = evmMsg.Gas() - evmResp.GasUsed
 	}
 	weiPerGas := txMsg.EffectiveGasPriceWeiPerGas(evmConfig.BaseFeeWei)
 	if err = k.RefundGas(ctx, evmMsg.From(), refundGas, weiPerGas); err != nil {
 		return nil, errors.Wrapf(err, "error refunding leftover gas to sender %s", evmMsg.From())
 	}
 
+	blockGasUsed, err := k.AddToBlockGasUsed(ctx, evmResp.GasUsed)
+	if err != nil {
+		return nil, errors.Wrap(err, "error adding transient gas used to block")
+	}
 	// reset the gas meter for current TxMsg (EthereumTx)
 	k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed)
 
@@ -563,8 +562,15 @@
 		coin.Amount.BigInt(),
 	)
 	if err != nil {
+		k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit())
 		return nil, err
 	}
+	blockGasUsed, errBlockGasUsed := k.AddToBlockGasUsed(ctx, evmResp.GasUsed)
+	if errBlockGasUsed != nil {
+		return nil, errors.Wrap(errBlockGasUsed, "error adding transient gas used")
+	}
+	k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed)
+
 	if evmResp.Failed() {
 		return nil,
 			fmt.Errorf("failed to mint erc-20 tokens of contract %s", erc20Addr.String())