-
Notifications
You must be signed in to change notification settings - Fork 201
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(evm): StateDB multistore cache for precompile reversion and a safer Nibiru bank keeper that respects the EVM #2094
Changes from 8 commits
295a2d9
80dd2d7
c624912
7f904a0
08a73ee
717ce1c
04a6897
7c34423
4e3bf3c
576cf6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,8 @@ func NewTestDeps() TestDeps { | |
} | ||
|
||
func (deps TestDeps) StateDB() *statedb.StateDB { | ||
return statedb.New(deps.Ctx, &deps.App.EvmKeeper, | ||
return deps.EvmKeeper.NewStateDB( | ||
deps.Ctx, | ||
Comment on lines
+49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Direct Found inconsistent StateDB creation patterns that should be standardized:
The only legitimate direct usage is in 🔗 Analysis chainLGTM! Improved StateDB creation pattern. This change properly delegates StateDB creation to EvmKeeper, ensuring test scenarios use the same state management path as production code. This alignment strengthens test reliability and supports the PR's goal of safer state handling. Let's verify consistent StateDB creation patterns across the codebase: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any direct statedb.New calls that should use EvmKeeper.NewStateDB
# Expected: Only EvmKeeper should directly call statedb.New
# Search for direct statedb.New usage
echo "Checking for direct statedb.New usage..."
rg "statedb\.New\(" -A 3
# Search for EvmKeeper.NewStateDB usage
echo "Checking for EvmKeeper.NewStateDB usage..."
rg "\.NewStateDB\(" -A 3
Length of output: 1451 |
||
statedb.NewEmptyTxConfig( | ||
gethcommon.BytesToHash(deps.Ctx.HeaderHash().Bytes()), | ||
), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved these to the bottom since the workhorse functions in this file are below this |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,95 +26,6 @@ import ( | |
"github.com/NibiruChain/nibiru/v2/x/evm/embeds" | ||
) | ||
|
||
type GethTxType = uint8 | ||
|
||
func TxTemplateAccessListTx() *gethcore.AccessListTx { | ||
return &gethcore.AccessListTx{ | ||
GasPrice: big.NewInt(1), | ||
Gas: gethparams.TxGas, | ||
To: &gethcommon.Address{}, | ||
Value: big.NewInt(0), | ||
Data: []byte{}, | ||
} | ||
} | ||
|
||
func TxTemplateLegacyTx() *gethcore.LegacyTx { | ||
return &gethcore.LegacyTx{ | ||
GasPrice: big.NewInt(1), | ||
Gas: gethparams.TxGas, | ||
To: &gethcommon.Address{}, | ||
Value: big.NewInt(0), | ||
Data: []byte{}, | ||
} | ||
} | ||
|
||
func TxTemplateDynamicFeeTx() *gethcore.DynamicFeeTx { | ||
return &gethcore.DynamicFeeTx{ | ||
GasFeeCap: big.NewInt(10), | ||
GasTipCap: big.NewInt(2), | ||
Gas: gethparams.TxGas, | ||
To: &gethcommon.Address{}, | ||
Value: big.NewInt(0), | ||
Data: []byte{}, | ||
} | ||
} | ||
|
||
func NewEthTxMsgFromTxData( | ||
deps *TestDeps, | ||
txType GethTxType, | ||
innerTxData []byte, | ||
nonce uint64, | ||
to *gethcommon.Address, | ||
value *big.Int, | ||
gas uint64, | ||
accessList gethcore.AccessList, | ||
) (*evm.MsgEthereumTx, error) { | ||
if innerTxData == nil { | ||
innerTxData = []byte{} | ||
} | ||
|
||
var ethCoreTx *gethcore.Transaction | ||
switch txType { | ||
case gethcore.LegacyTxType: | ||
innerTx := TxTemplateLegacyTx() | ||
innerTx.Nonce = nonce | ||
innerTx.Data = innerTxData | ||
innerTx.To = to | ||
innerTx.Value = value | ||
innerTx.Gas = gas | ||
ethCoreTx = gethcore.NewTx(innerTx) | ||
case gethcore.AccessListTxType: | ||
innerTx := TxTemplateAccessListTx() | ||
innerTx.Nonce = nonce | ||
innerTx.Data = innerTxData | ||
innerTx.AccessList = accessList | ||
innerTx.To = to | ||
innerTx.Value = value | ||
innerTx.Gas = gas | ||
ethCoreTx = gethcore.NewTx(innerTx) | ||
case gethcore.DynamicFeeTxType: | ||
innerTx := TxTemplateDynamicFeeTx() | ||
innerTx.Nonce = nonce | ||
innerTx.Data = innerTxData | ||
innerTx.To = to | ||
innerTx.Value = value | ||
innerTx.Gas = gas | ||
innerTx.AccessList = accessList | ||
ethCoreTx = gethcore.NewTx(innerTx) | ||
default: | ||
return nil, fmt.Errorf( | ||
"received unknown tx type (%v) in NewEthTxMsgFromTxData", txType) | ||
} | ||
|
||
ethTxMsg := new(evm.MsgEthereumTx) | ||
if err := ethTxMsg.FromEthereumTx(ethCoreTx); err != nil { | ||
return ethTxMsg, err | ||
} | ||
|
||
ethTxMsg.From = deps.Sender.EthAddr.Hex() | ||
return ethTxMsg, ethTxMsg.Sign(deps.GethSigner(), deps.Sender.KeyringSigner) | ||
} | ||
|
||
// ExecuteNibiTransfer executes nibi transfer | ||
func ExecuteNibiTransfer(deps *TestDeps, t *testing.T) *evm.MsgEthereumTx { | ||
nonce := deps.StateDB().GetNonce(deps.Sender.EthAddr) | ||
|
@@ -326,6 +237,10 @@ func TransferWei( | |
return err | ||
} | ||
|
||
// -------------------------------------------------- | ||
// Templates | ||
// -------------------------------------------------- | ||
|
||
// ValidLegacyTx: Useful initial condition for tests | ||
// Exported only for use in tests. | ||
func ValidLegacyTx() *evm.LegacyTx { | ||
|
@@ -342,3 +257,92 @@ func ValidLegacyTx() *evm.LegacyTx { | |
S: []byte{}, | ||
} | ||
} | ||
|
||
type GethTxType = uint8 | ||
|
||
func TxTemplateAccessListTx() *gethcore.AccessListTx { | ||
return &gethcore.AccessListTx{ | ||
GasPrice: big.NewInt(1), | ||
Gas: gethparams.TxGas, | ||
To: &gethcommon.Address{}, | ||
Value: big.NewInt(0), | ||
Data: []byte{}, | ||
} | ||
} | ||
|
||
func TxTemplateLegacyTx() *gethcore.LegacyTx { | ||
return &gethcore.LegacyTx{ | ||
GasPrice: big.NewInt(1), | ||
Gas: gethparams.TxGas, | ||
To: &gethcommon.Address{}, | ||
Value: big.NewInt(0), | ||
Data: []byte{}, | ||
} | ||
} | ||
|
||
func TxTemplateDynamicFeeTx() *gethcore.DynamicFeeTx { | ||
return &gethcore.DynamicFeeTx{ | ||
GasFeeCap: big.NewInt(10), | ||
GasTipCap: big.NewInt(2), | ||
Gas: gethparams.TxGas, | ||
To: &gethcommon.Address{}, | ||
Value: big.NewInt(0), | ||
Data: []byte{}, | ||
} | ||
} | ||
|
||
func NewEthTxMsgFromTxData( | ||
deps *TestDeps, | ||
txType GethTxType, | ||
innerTxData []byte, | ||
nonce uint64, | ||
to *gethcommon.Address, | ||
value *big.Int, | ||
gas uint64, | ||
accessList gethcore.AccessList, | ||
) (*evm.MsgEthereumTx, error) { | ||
if innerTxData == nil { | ||
innerTxData = []byte{} | ||
} | ||
|
||
var ethCoreTx *gethcore.Transaction | ||
switch txType { | ||
case gethcore.LegacyTxType: | ||
innerTx := TxTemplateLegacyTx() | ||
innerTx.Nonce = nonce | ||
innerTx.Data = innerTxData | ||
innerTx.To = to | ||
innerTx.Value = value | ||
innerTx.Gas = gas | ||
ethCoreTx = gethcore.NewTx(innerTx) | ||
case gethcore.AccessListTxType: | ||
innerTx := TxTemplateAccessListTx() | ||
innerTx.Nonce = nonce | ||
innerTx.Data = innerTxData | ||
innerTx.AccessList = accessList | ||
innerTx.To = to | ||
innerTx.Value = value | ||
innerTx.Gas = gas | ||
ethCoreTx = gethcore.NewTx(innerTx) | ||
case gethcore.DynamicFeeTxType: | ||
innerTx := TxTemplateDynamicFeeTx() | ||
innerTx.Nonce = nonce | ||
innerTx.Data = innerTxData | ||
innerTx.To = to | ||
innerTx.Value = value | ||
innerTx.Gas = gas | ||
innerTx.AccessList = accessList | ||
ethCoreTx = gethcore.NewTx(innerTx) | ||
default: | ||
return nil, fmt.Errorf( | ||
"received unknown tx type (%v) in NewEthTxMsgFromTxData", txType) | ||
} | ||
|
||
ethTxMsg := new(evm.MsgEthereumTx) | ||
if err := ethTxMsg.FromEthereumTx(ethCoreTx); err != nil { | ||
return ethTxMsg, err | ||
} | ||
|
||
ethTxMsg.From = deps.Sender.EthAddr.Hex() | ||
return ethTxMsg, ethTxMsg.Sign(deps.GethSigner(), deps.Sender.KeyringSigner) | ||
} | ||
Comment on lines
+315
to
+369
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding input validation and refactoring common field assignments. The function is well-documented and handles different transaction types correctly. Consider these improvements:
Here's a suggested refactoring: func NewEthTxMsgFromTxData(
deps *TestDeps,
txType GethTxType,
innerTxData []byte,
nonce uint64,
to *gethcommon.Address,
value *big.Int,
gas uint64,
accessList gethcore.AccessList,
) (*evm.MsgEthereumTx, error) {
+ if value == nil {
+ return nil, fmt.Errorf("value cannot be nil")
+ }
+ if value.Sign() < 0 {
+ return nil, fmt.Errorf("negative value not allowed")
+ }
+
if innerTxData == nil {
innerTxData = []byte{}
}
+ // Common fields for all transaction types
+ commonFields := struct {
+ nonce uint64
+ data []byte
+ to *gethcommon.Address
+ value *big.Int
+ gas uint64
+ }{nonce, innerTxData, to, value, gas}
+
var ethCoreTx *gethcore.Transaction
switch txType {
case gethcore.LegacyTxType:
innerTx := TxTemplateLegacyTx()
- innerTx.Nonce = nonce
- innerTx.Data = innerTxData
- innerTx.To = to
- innerTx.Value = value
- innerTx.Gas = gas
+ innerTx.Nonce = commonFields.nonce
+ innerTx.Data = commonFields.data
+ innerTx.To = commonFields.to
+ innerTx.Value = commonFields.value
+ innerTx.Gas = commonFields.gas
ethCoreTx = gethcore.NewTx(innerTx)
// ... similar changes for other cases
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was here because of the repeated calls during gas estimation, not for EVM reversions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line's required to appease the module manager when it does a type assertion on an object of type
bankkeeper.Keeper
(an interface), asserting that it's actually abankkeeper.BaseKeeper
struct. I didn't see a simple way to force x/bank to use theNibiruBankKeeper
struct without editing the Bank module directly, so I opted for what's written here, which is quite simple.Edit: Moved the
NibiruBankKeeper
to #2095. The rationale behind these lines are in that PR