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 1 commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ 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
- [#2xxx](https://github.com/NibiruChain/nibiru/pull/2xxx) - fix(evm-gas-fees): fix(gas-fees): use effective gas price in RefundGas
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

New gas fee refund functionality added

The addition of a fix for gas fee refunds using the effective gas price is a significant change that could impact transaction processing and user costs. This change should be thoroughly tested to ensure it behaves as expected across various transaction types and gas price scenarios.

Please provide more details on the implementation of this fix and its potential impact on users and validators. Consider adding a brief explanation of how the effective gas price is calculated and used in the refund process.



#### Dapp modules: perp, spot, oracle, etc
Expand Down
13 changes: 7 additions & 6 deletions app/evmante/evmante_can_transfer.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,27 +44,28 @@ 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)
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved

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
)
}

Expand All @@ -73,7 +74,7 @@ func (ctd CanTransferDecorator) AnteHandle(
ChainConfig: ethCfg,
Params: params,
CoinBase: gethcommon.Address{},
BaseFee: baseFee,
BaseFee: baseFeeMicronibiPerGas,
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved
}

stateDB := statedb.New(
Expand Down
2 changes: 1 addition & 1 deletion app/evmante/evmante_gas_consume.go
Original file line number Diff line number Diff line change
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
4 changes: 2 additions & 2 deletions app/evmante/evmante_mempool_fees.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (d MempoolGasPriceDecorator) AnteHandle(
}

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

// if MinGasPrices is not set, skip the check
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_validate_basic.go
Original file line number Diff line number Diff line change
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
2 changes: 1 addition & 1 deletion app/evmante/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type EVMKeeper interface {

EVMState() evmkeeper.EvmState
EthChainID(ctx sdk.Context) *big.Int
GetBaseFee(ctx sdk.Context) *big.Int
BaseFeeMicronibiPerGas(ctx sdk.Context) *big.Int
}

type protoTxProvider interface {
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
}
2 changes: 1 addition & 1 deletion eth/rpc/backend/tx_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func (b *Backend) GetTransactionReceipt(hash gethcommon.Hash) (*TransactionRecei
// tolerate the error for pruned node.
b.logger.Error("fetch basefee failed, node is pruned?", "height", res.Height, "error", err)
} else {
receipt.EffectiveGasPrice = dynamicTx.EffectiveGasPriceWei(baseFee)
receipt.EffectiveGasPrice = dynamicTx.EffectiveGasPriceWeiPerGas(baseFee)
}
}
return &receipt, nil
Expand Down
27 changes: 27 additions & 0 deletions justfile
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,33 @@ lint:
localnet *PASS_FLAGS:
make localnet FLAGS="{{PASS_FLAGS}}"

# Clears the logs directory
log-clear:
#!/usr/bin/env bash
rm logs/*
Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved

# Runs "just localnet" with logging (logs/localnet.txt)
log-localnet:
#!/usr/bin/env bash
mkdir -p logs
just localnet 2>&1 | tee -a logs/localnet.txt
Comment on lines +61 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling in the log-localnet recipe.

While the recipe implements the basic functionality, it could benefit from improved error handling as suggested in the previous review. Consider updating the recipe to handle potential errors when creating the logs directory:

 log-localnet:
   #!/usr/bin/env bash
-  mkdir -p logs
+  if ! mkdir -p logs; then
+    echo "Error: Failed to create logs directory." >&2
+    exit 1
+  fi
   just localnet 2>&1 | tee -a logs/localnet.txt

This change will ensure that the script exits with an error message if it fails to create the logs directory.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Runs "just localnet" with logging (logs/localnet.txt)
log-localnet:
#!/usr/bin/env bash
mkdir -p logs
just localnet 2>&1 | tee -a logs/localnet.txt
# Runs "just localnet" with logging (logs/localnet.txt)
log-localnet:
#!/usr/bin/env bash
if ! mkdir -p logs; then
echo "Error: Failed to create logs directory." >&2
exit 1
fi
just localnet 2>&1 | tee -a logs/localnet.txt


# Runs the EVM E2E test with logging (logs/e2e.txt)
log-e2e:
#!/usr/bin/env bash
just test-e2e 2>&1 | tee -a ../../logs/e2e.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve consistency and error handling in the log-e2e recipe.

The recipe implements the basic functionality, but there are opportunities for improvement:

  1. The log file path is inconsistent with the log-localnet recipe.
  2. It doesn't ensure the existence of the logs directory.

Consider updating the recipe as follows:

 log-e2e:
   #!/usr/bin/env bash
-  just test-e2e 2>&1 | tee -a ../../logs/e2e.txt
+  if ! mkdir -p logs; then
+    echo "Error: Failed to create logs directory." >&2
+    exit 1
+  fi
+  just test-e2e 2>&1 | tee -a logs/e2e.txt

This change will:

  1. Use a consistent log file path with other recipes.
  2. Ensure the logs directory exists before running the command.
  3. Provide error handling if directory creation fails.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Runs the EVM E2E test with logging (logs/e2e.txt)
log-e2e:
#!/usr/bin/env bash
just test-e2e 2>&1 | tee -a ../../logs/e2e.txt
# Runs the EVM E2E test with logging (logs/e2e.txt)
log-e2e:
#!/usr/bin/env bash
if ! mkdir -p logs; then
echo "Error: Failed to create logs directory." >&2
exit 1
fi
just test-e2e 2>&1 | tee -a logs/e2e.txt


# Runs the EVM E2E tests
test-e2e:
#!/usr/bin/env bash
source contrib/bashlib.sh
log_info "Make sure the localnet is running! (just localnet)"

cd e2e/evm
nvm use
just test

Unique-Divine marked this conversation as resolved.
Show resolved Hide resolved

# Test: "localnet.sh" script
test-localnet:
#!/usr/bin/env bash
Expand Down
5 changes: 4 additions & 1 deletion x/evm/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import (

// BASE_FEE_MICRONIBI is the global base fee value for the network. It has a
// constant value of 1 unibi (micronibi) == 10^12 wei.
var BASE_FEE_MICRONIBI = big.NewInt(1)
var (
BASE_FEE_MICRONIBI = big.NewInt(1)
BASE_FEE_WEI = NativeToWei(BASE_FEE_MICRONIBI)
)

const (
// ModuleName string name of module
Expand Down
40 changes: 23 additions & 17 deletions x/evm/keeper/gas_fees.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,36 +34,42 @@ func (k *Keeper) GetEthIntrinsicGas(
)
}

// RefundGas transfers the leftover gas to the sender of the message, caped to
// half of the total gas consumed in the transaction. Additionally, the function
// sets the total gas consumed to the value returned by the EVM execution, thus
// ignoring the previous intrinsic gas consumed during in the AnteHandler.
// RefundGas transfers the leftover gas to the sender of the message.
func (k *Keeper) RefundGas(
ctx sdk.Context,
msg core.Message,
msgFrom gethcommon.Address,
leftoverGas uint64,
denom string,
weiPerGas *big.Int,
Comment on lines +40 to +42
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

Incomplete Updates to RefundGas Function Signature

The RefundGas function has been updated in some parts of the codebase, but the following files still use the old signature:

  • eth/gas_limit.go
  • eth/gas_limit_test.go
  • app/ante/gas.go

Please update all remaining calls to RefundGas to match the new signature with msgFrom gethcommon.Address and weiPerGas *big.Int to prevent potential compile-time errors or unexpected behavior.

🔗 Analysis chain

Ensure all callers of RefundGas are updated to match the new signature

The RefundGas function's signature has changed to accept msgFrom gethcommon.Address and weiPerGas *big.Int instead of msg core.Message and denom string. Please verify that all invocations of this function have been updated accordingly throughout the codebase to prevent any compile-time errors or unexpected behavior.

To automate this verification, you can run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all calls to `RefundGas` and check for updated parameters.

# Search for calls to `RefundGas` and display the lines with context.
rg --type go 'RefundGas\(' -A 3

Length of output: 2800

) error {
// Return EVM tokens for remaining gas, exchanged at the original rate.
remaining := new(big.Int).Mul(new(big.Int).SetUint64(leftoverGas), msg.GasPrice())
leftoverWei := new(big.Int).Mul(
new(big.Int).SetUint64(leftoverGas),
weiPerGas,
)
leftoverMicronibi := evm.WeiToNative(leftoverWei)

switch remaining.Sign() {
switch leftoverMicronibi.Sign() {
case -1:
// negative refund errors
return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", remaining.Int64())
// Should be impossible since leftoverGas is a uint64. Reaching this case
// would imply a critical error in the effective gas calculation.
return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", leftoverMicronibi.Int64())
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use String() method for error message to avoid potential overflows

When logging leftoverMicronibi in the error message, using Int64() may lead to incorrect values if leftoverMicronibi exceeds the range of an int64. It's safer to use leftoverMicronibi.String() to represent the full big.Int value.

Apply this diff to fix the error message:

-return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", leftoverMicronibi.Int64())
+return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %s", leftoverMicronibi.String())
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", leftoverMicronibi.Int64())
return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %s", leftoverMicronibi.String())

case 1:
// positive amount refund
refundedCoins := sdk.Coins{sdk.NewCoin(denom, sdkmath.NewIntFromBigInt(remaining))}

// refund to sender from the fee collector module account, which is the escrow account in charge of collecting tx fees

err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, authtypes.FeeCollectorName, msg.From().Bytes(), refundedCoins)
refundedCoins := sdk.Coins{sdk.NewCoin(evm.EVMBankDenom, sdkmath.NewIntFromBigInt(leftoverMicronibi))}

// Refund to sender from the fee collector module account. This account
// manages the collection of gas fees.
err := k.bankKeeper.SendCoinsFromModuleToAccount(
ctx,
authtypes.FeeCollectorName, // sender
msgFrom.Bytes(), // recipient
refundedCoins,
)
if err != nil {
err = errors.Wrapf(errortypes.ErrInsufficientFunds, "fee collector account failed to refund fees: %s", err.Error())
return errors.Wrapf(err, "failed to refund %d leftover gas (%s)", leftoverGas, refundedCoins.String())
}
default:
// no refund, consume gas and update the tx gas meter
// no refund
}

return nil
Expand Down
Loading
Loading