Skip to content
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

fix(gas-fees): use effective gas price in RefundGas #2076

Merged
merged 6 commits into from
Oct 13, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ txout.json
vote.json
**__pycache**
scratch-paper.md
logs

### TypeScript and Friends

Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [#2056](https://github.com/NibiruChain/nibiru/pull/2056) - feat(evm): add oracle precompile
- [#2065](https://github.com/NibiruChain/nibiru/pull/2065) - refactor(evm)!: Refactor out dead code from the evm.Params
- [#2073](https://github.com/NibiruChain/nibiru/pull/2073) - fix(evm-keeper): better utilize ERC20 metadata during FunToken creation
- [#2076](https://github.com/NibiruChain/nibiru/pull/2076) - fix(evm-gas-fees):
Use effective gas price in RefundGas and make sure that units are properly
reflected on all occurences of "base fee" in the codebase. This fixes [#2059](https://github.com/NibiruChain/nibiru/issues/2059)
and the [related comments from @Unique-Divine and @berndartmueller](https://github.com/NibiruChain/nibiru/issues/2059#issuecomment-2408625724).
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved


#### Dapp modules: perp, spot, oracle, etc
Expand Down
36 changes: 15 additions & 21 deletions app/evmante/evmante_can_transfer.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,16 @@ import (
// CanTransferDecorator checks if the sender is allowed to transfer funds according to the EVM block
// context rules.
type CanTransferDecorator struct {
evmKeeper EVMKeeper
}

// NewCanTransferDecorator creates a new CanTransferDecorator instance.
func NewCanTransferDecorator(k EVMKeeper) CanTransferDecorator {
return CanTransferDecorator{
evmKeeper: k,
}
*EVMKeeper
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Non-Pointer EVMKeeper Usages Found

The following instances of EVMKeeper are still using the non-pointer type:

  • app/evmante/interfaces.go:

    type EVMKeeper = evmkeeper.Keeper
  • app/evmante/evmante_verify_eth_acc.go:

    func NewAnteDecVerifyEthAcc(k *EVMKeeper, ak evm.AccountKeeper) AnteDecVerifyEthAcc {
        return AnteDecVerifyEthAcc{
            evmKeeper: k,
  • app/evmante/evmante_increment_sender_seq.go:

    func NewAnteDecEthIncrementSenderSequence(k *EVMKeeper, ak ante.AccountKeeper) AnteDecEthIncrementSenderSequence {
        return AnteDecEthIncrementSenderSequence{
            evmKeeper: k,
  • app/evmante/evmante_can_transfer.go:

    ethCfg := evm.EthereumConfig(ctd.EVMKeeper.EthChainID(ctx))
    baseFeeWeiPerGas := evm.NativeToWei(ctd.EVMKeeper.BaseFeeMicronibiPerGas(ctx))
    evmInstance := ctd.EVMKeeper.NewEVM(ctx, coreMsg, cfg, evm.NewNoOpTracer(), stateDB)

Please update these instances to use *EVMKeeper and ensure that appropriate nil checks are implemented where necessary.

🔗 Analysis chain

LGTM! Consider adding nil checks.

The change from EVMKeeper to *EVMKeeper is a good optimization. It allows for more efficient usage of the EVMKeeper and enables nil checks.

To ensure the change is implemented correctly throughout the codebase, run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of *EVMKeeper and potential nil checks

# Test 1: Check for any remaining non-pointer EVMKeeper usage
echo "Checking for non-pointer EVMKeeper usage:"
rg --type go 'type.*struct.*\bEVMKeeper\b[^*]'

# Test 2: Look for potential places where nil checks might be needed
echo "Potential places for nil checks:"
rg --type go '\bEVMKeeper\b.*\.' -C 2

Length of output: 2495

}

// AnteHandle creates an EVM from the message and calls the BlockContext CanTransfer function to
// see if the address can execute the transaction.
func (ctd CanTransferDecorator) AnteHandle(
ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler,
) (sdk.Context, error) {
params := ctd.evmKeeper.GetParams(ctx)
ethCfg := evm.EthereumConfig(ctd.evmKeeper.EthChainID(ctx))
params := ctd.GetParams(ctx)
ethCfg := evm.EthereumConfig(ctd.EVMKeeper.EthChainID(ctx))
signer := gethcore.MakeSigner(ethCfg, big.NewInt(ctx.BlockHeight()))

for _, msg := range tx.GetMsgs() {
Expand All @@ -44,44 +37,45 @@ func (ctd CanTransferDecorator) AnteHandle(
"invalid message type %T, expected %T", msg, (*evm.MsgEthereumTx)(nil),
)
}
baseFee := ctd.evmKeeper.GetBaseFee(ctx)
baseFeeMicronibiPerGas := ctd.EVMKeeper.BaseFeeMicronibiPerGas(ctx)
baseFeeWeiPerGas := evm.NativeToWei(baseFeeMicronibiPerGas)

coreMsg, err := msgEthTx.AsMessage(signer, baseFee)
coreMsg, err := msgEthTx.AsMessage(signer, baseFeeWeiPerGas)
if err != nil {
return ctx, errors.Wrapf(
err,
"failed to create an ethereum core.Message from signer %T", signer,
)
}

if baseFee == nil {
if baseFeeMicronibiPerGas == nil {
return ctx, errors.Wrap(
evm.ErrInvalidBaseFee,
"base fee is supported but evm block context value is nil",
)
}
if coreMsg.GasFeeCap().Cmp(baseFee) < 0 {
if coreMsg.GasFeeCap().Cmp(baseFeeMicronibiPerGas) < 0 {
return ctx, errors.Wrapf(
sdkerrors.ErrInsufficientFee,
"max fee per gas less than block base fee (%s < %s)",
coreMsg.GasFeeCap(), baseFee,
coreMsg.GasFeeCap(), baseFeeMicronibiPerGas,
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
)
}

// NOTE: pass in an empty coinbase address and nil tracer as we don't need them for the check below
cfg := &statedb.EVMConfig{
ChainConfig: ethCfg,
Params: params,
CoinBase: gethcommon.Address{},
BaseFee: baseFee,
ChainConfig: ethCfg,
Params: params,
BlockCoinbase: gethcommon.Address{},
BaseFeeWei: baseFeeMicronibiPerGas,
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
}

stateDB := statedb.New(
ctx,
ctd.evmKeeper,
ctd.EVMKeeper,
statedb.NewEmptyTxConfig(gethcommon.BytesToHash(ctx.HeaderHash().Bytes())),
)
evmInstance := ctd.evmKeeper.NewEVM(ctx, coreMsg, cfg, evm.NewNoOpTracer(), stateDB)
evmInstance := ctd.EVMKeeper.NewEVM(ctx, coreMsg, cfg, evm.NewNoOpTracer(), stateDB)

// check that caller has enough balance to cover asset transfer for **topmost** call
// NOTE: here the gas consumed is from the context with the infinite gas meter
Expand Down
2 changes: 1 addition & 1 deletion app/evmante/evmante_can_transfer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (s *TestSuite) TestCanTransferDecorator() {
s.Run(tc.name, func() {
deps := evmtest.NewTestDeps()
stateDB := deps.StateDB()
anteDec := evmante.NewCanTransferDecorator(&deps.App.AppKeepers.EvmKeeper)
anteDec := evmante.CanTransferDecorator{&deps.App.AppKeepers.EvmKeeper}
tx := tc.txSetup(&deps)

if tc.ctxSetup != nil {
Expand Down
4 changes: 2 additions & 2 deletions app/evmante/evmante_emit_event.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ import (

// EthEmitEventDecorator emit events in ante handler in case of tx execution failed (out of block gas limit).
type EthEmitEventDecorator struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
}

// NewEthEmitEventDecorator creates a new EthEmitEventDecorator
func NewEthEmitEventDecorator(k EVMKeeper) EthEmitEventDecorator {
func NewEthEmitEventDecorator(k *EVMKeeper) EthEmitEventDecorator {
return EthEmitEventDecorator{
evmKeeper: k,
}
Expand Down
6 changes: 3 additions & 3 deletions app/evmante/evmante_gas_consume.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ import (
// AnteDecEthGasConsume validates enough intrinsic gas for the transaction and
// gas consumption.
type AnteDecEthGasConsume struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
maxGasWanted uint64
}

// NewAnteDecEthGasConsume creates a new EthGasConsumeDecorator
func NewAnteDecEthGasConsume(
k EVMKeeper,
k *EVMKeeper,
maxGasWanted uint64,
) AnteDecEthGasConsume {
return AnteDecEthGasConsume{
Expand Down Expand Up @@ -68,7 +68,7 @@ func (anteDec AnteDecEthGasConsume) AnteHandle(

// Use the lowest priority of all the messages as the final one.
minPriority := int64(math.MaxInt64)
baseFeeMicronibiPerGas := anteDec.evmKeeper.GetBaseFee(ctx)
baseFeeMicronibiPerGas := anteDec.evmKeeper.BaseFeeMicronibiPerGas(ctx)

for _, msg := range tx.GetMsgs() {
msgEthTx, ok := msg.(*evm.MsgEthereumTx)
Expand Down
2 changes: 1 addition & 1 deletion app/evmante/evmante_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func NewAnteHandlerEVM(
NewEthValidateBasicDecorator(&options.EvmKeeper),
NewEthSigVerificationDecorator(&options.EvmKeeper),
NewAnteDecVerifyEthAcc(&options.EvmKeeper, options.AccountKeeper),
NewCanTransferDecorator(&options.EvmKeeper),
CanTransferDecorator{&options.EvmKeeper},
NewAnteDecEthGasConsume(&options.EvmKeeper, options.MaxTxGasWanted),
NewAnteDecEthIncrementSenderSequence(&options.EvmKeeper, options.AccountKeeper),
ante.AnteDecoratorGasWanted{},
Expand Down
4 changes: 2 additions & 2 deletions app/evmante/evmante_increment_sender_seq.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ import (

// AnteDecEthIncrementSenderSequence increments the sequence of the signers.
type AnteDecEthIncrementSenderSequence struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
accountKeeper ante.AccountKeeper
}

// NewAnteDecEthIncrementSenderSequence creates a new EthIncrementSenderSequenceDecorator.
func NewAnteDecEthIncrementSenderSequence(k EVMKeeper, ak ante.AccountKeeper) AnteDecEthIncrementSenderSequence {
func NewAnteDecEthIncrementSenderSequence(k *EVMKeeper, ak ante.AccountKeeper) AnteDecEthIncrementSenderSequence {
return AnteDecEthIncrementSenderSequence{
evmKeeper: k,
accountKeeper: ak,
Expand Down
14 changes: 7 additions & 7 deletions app/evmante/evmante_mempool_fees.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ var _ sdk.AnteDecorator = MempoolGasPriceDecorator{}
// is rejected. This applies to CheckTx only.
// If fee is high enough, then call next AnteHandler
type MempoolGasPriceDecorator struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
}

// NewMempoolGasPriceDecorator creates a new MinGasPriceDecorator instance used only for
// Ethereum transactions.
func NewMempoolGasPriceDecorator(k EVMKeeper) MempoolGasPriceDecorator {
func NewMempoolGasPriceDecorator(k *EVMKeeper) MempoolGasPriceDecorator {
return MempoolGasPriceDecorator{
evmKeeper: k,
}
Expand All @@ -39,14 +39,14 @@ func (d MempoolGasPriceDecorator) AnteHandle(
}

minGasPrice := ctx.MinGasPrices().AmountOf(evm.EVMBankDenom)
baseFeeMicronibi := d.evmKeeper.GetBaseFee(ctx)
baseFeeDec := math.LegacyNewDecFromBigInt(baseFeeMicronibi)
baseFeeMicronibi := d.evmKeeper.BaseFeeMicronibiPerGas(ctx)
baseFeeMicronibiDec := math.LegacyNewDecFromBigInt(baseFeeMicronibi)

// if MinGasPrices is not set, skip the check
if minGasPrice.IsZero() {
return next(ctx, tx, simulate)
} else if minGasPrice.LT(baseFeeDec) {
minGasPrice = baseFeeDec
} else if minGasPrice.LT(baseFeeMicronibiDec) {
minGasPrice = baseFeeMicronibiDec
}

for _, msg := range tx.GetMsgs() {
Expand All @@ -61,7 +61,7 @@ func (d MempoolGasPriceDecorator) AnteHandle(

baseFeeWei := evm.NativeToWei(baseFeeMicronibi)
effectiveGasPriceDec := math.LegacyNewDecFromBigInt(
evm.WeiToNative(ethTx.GetEffectiveGasPrice(baseFeeWei)),
evm.WeiToNative(ethTx.EffectiveGasPriceWeiPerGas(baseFeeWei)),
)
if effectiveGasPriceDec.LT(minGasPrice) {
// if sdk.NewDecFromBigInt(effectiveGasPrice).LT(minGasPrice) {
Expand Down
4 changes: 2 additions & 2 deletions app/evmante/evmante_setup_ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import (
// EthSetupContextDecorator is adapted from SetUpContextDecorator from cosmos-sdk, it ignores gas consumption
// by setting the gas meter to infinite
type EthSetupContextDecorator struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
}

func NewEthSetUpContextDecorator(k EVMKeeper) EthSetupContextDecorator {
func NewEthSetUpContextDecorator(k *EVMKeeper) EthSetupContextDecorator {
return EthSetupContextDecorator{
evmKeeper: k,
}
Expand Down
4 changes: 2 additions & 2 deletions app/evmante/evmante_sigverify.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ import (

// EthSigVerificationDecorator validates an ethereum signatures
type EthSigVerificationDecorator struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
}

// NewEthSigVerificationDecorator creates a new EthSigVerificationDecorator
func NewEthSigVerificationDecorator(k EVMKeeper) EthSigVerificationDecorator {
func NewEthSigVerificationDecorator(k *EVMKeeper) EthSigVerificationDecorator {
return EthSigVerificationDecorator{
evmKeeper: k,
}
Expand Down
8 changes: 4 additions & 4 deletions app/evmante/evmante_validate_basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ import (

// EthValidateBasicDecorator is adapted from ValidateBasicDecorator from cosmos-sdk, it ignores ErrNoSignatures
type EthValidateBasicDecorator struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
}

// NewEthValidateBasicDecorator creates a new EthValidateBasicDecorator
func NewEthValidateBasicDecorator(k EVMKeeper) EthValidateBasicDecorator {
func NewEthValidateBasicDecorator(k *EVMKeeper) EthValidateBasicDecorator {
return EthValidateBasicDecorator{
evmKeeper: k,
}
Expand Down Expand Up @@ -89,7 +89,7 @@ func (vbd EthValidateBasicDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simu
txFee := sdk.Coins{}
txGasLimit := uint64(0)

baseFee := vbd.evmKeeper.GetBaseFee(ctx)
baseFeeMicronibi := vbd.evmKeeper.BaseFeeMicronibiPerGas(ctx)

for _, msg := range protoTx.GetMsgs() {
msgEthTx, ok := msg.(*evm.MsgEthereumTx)
Expand All @@ -115,7 +115,7 @@ func (vbd EthValidateBasicDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simu
return ctx, errorsmod.Wrap(err, "failed to unpack MsgEthereumTx Data")
}

if baseFee == nil && txData.TxType() == gethcore.DynamicFeeTxType {
if baseFeeMicronibi == nil && txData.TxType() == gethcore.DynamicFeeTxType {
return ctx, errorsmod.Wrap(
gethcore.ErrTxTypeNotSupported,
"dynamic fee tx not supported",
Expand Down
4 changes: 2 additions & 2 deletions app/evmante/evmante_verify_eth_acc.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ import (

// AnteDecVerifyEthAcc validates an account balance checks
type AnteDecVerifyEthAcc struct {
evmKeeper EVMKeeper
evmKeeper *EVMKeeper
accountKeeper evm.AccountKeeper
}

// NewAnteDecVerifyEthAcc creates a new EthAccountVerificationDecorator
func NewAnteDecVerifyEthAcc(k EVMKeeper, ak evm.AccountKeeper) AnteDecVerifyEthAcc {
func NewAnteDecVerifyEthAcc(k *EVMKeeper, ak evm.AccountKeeper) AnteDecVerifyEthAcc {
return AnteDecVerifyEthAcc{
evmKeeper: k,
accountKeeper: ak,
Expand Down
26 changes: 2 additions & 24 deletions app/evmante/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,33 +2,11 @@
package evmante

import (
"math/big"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/vm"

"github.com/NibiruChain/nibiru/v2/x/evm"
evmkeeper "github.com/NibiruChain/nibiru/v2/x/evm/keeper"
"github.com/NibiruChain/nibiru/v2/x/evm/statedb"
"github.com/cosmos/cosmos-sdk/types/tx"
)

// EVMKeeper defines the expected keeper interface used on the AnteHandler
type EVMKeeper interface {
statedb.Keeper

NewEVM(ctx sdk.Context, msg core.Message, cfg *statedb.EVMConfig, tracer vm.EVMLogger, stateDB vm.StateDB) *vm.EVM
DeductTxCostsFromUserBalance(ctx sdk.Context, fees sdk.Coins, from common.Address) error
GetEvmGasBalance(ctx sdk.Context, addr common.Address) *big.Int
ResetTransientGasUsed(ctx sdk.Context)
GetParams(ctx sdk.Context) evm.Params

EVMState() evmkeeper.EvmState
EthChainID(ctx sdk.Context) *big.Int
GetBaseFee(ctx sdk.Context) *big.Int
}
type EVMKeeper = evmkeeper.Keeper

type protoTxProvider interface {
GetProtoTx() *tx.Tx
Expand Down
20 changes: 14 additions & 6 deletions e2e/evm/test/contract_send_nibi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* "e2e/evm/contracts/SendReceiveNibi.sol".
*/
import { describe, expect, it } from "@jest/globals"
import { toBigInt, Wallet } from "ethers"
import { parseEther, toBigInt, Wallet } from "ethers"
import { account, provider } from "./setup"
import { deployContractSendNibi } from "./utils"

Expand All @@ -33,16 +33,24 @@ async function testSendNibi(
const txCostWei = txCostMicronibi * tenPow12
const expectedOwnerWei = ownerBalanceBefore - txCostWei

const ownerBalanceAfter = await provider.getBalance(account)
const recipientBalanceAfter = await provider.getBalance(recipient)

console.debug(`DEBUG method ${method} %o:`, {
ownerBalanceBefore,
weiToSend,
gasUsed: receipt.gasUsed,
gasPrice: `${receipt.gasPrice.toString()} micronibi`,
expectedOwnerWei,
ownerBalanceAfter,
recipientBalanceBefore,
recipientBalanceAfter,
gasUsed: receipt.gasUsed,
gasPrice: `${receipt.gasPrice.toString()}`,
to: receipt.to,
from: receipt.from,
})

await expect(provider.getBalance(account)).resolves.toBe(expectedOwnerWei)
await expect(provider.getBalance(recipient)).resolves.toBe(weiToSend)
const deltaFromExpectation = ownerBalanceAfter - expectedOwnerWei
expect(deltaFromExpectation).toBeLessThan(parseEther("0.001"))
expect(recipientBalanceAfter).toBe(weiToSend)
}

describe("Send NIBI via smart contract", () => {
Expand Down
5 changes: 3 additions & 2 deletions e2e/evm/test/native_transfer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe("native transfer", () => {

// Assert balances with logging
const tenPow12 = toBigInt(1e12)
const gasUsed = 50000n // 50k gas for the transaction
const gasUsed = transaction.gasLimit
const txCostMicronibi = amountToSend / tenPow12 + gasUsed
const txCostWei = txCostMicronibi * tenPow12
const expectedSenderWei = senderBalanceBefore - txCostWei
Expand All @@ -35,8 +35,9 @@ describe("native transfer", () => {
amountToSend,
expectedSenderWei,
senderBalanceAfter,
txResponse,
})
expect(senderBalanceAfter).toEqual(expectedSenderWei)
expect(recipientBalanceAfter).toEqual(amountToSend)
expect(senderBalanceAfter).toEqual(expectedSenderWei)
}, 20e3)
})
6 changes: 3 additions & 3 deletions eth/rpc/backend/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func NewBackend(
// TODO: feat(eth): Implement the cosmos JSON-RPC defined by Wallet Connect V2:
// https://docs.walletconnect.com/2.0/json-rpc/cosmos.
type CosmosBackend interface {
// TODO: GetAccounts()
// TODO: SignDirect()
// TODO: SignAmino()
// TODO: for Wallet Connect V2: GetAccounts()
// TODO: for Wallet Connect V2: SignDirect()
// TODO: for Wallet Connect V2: SignAmino()
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
}
Loading
Loading