From bc9cb03565510a05bcc234afc319417feb7375f1 Mon Sep 17 00:00:00 2001 From: Eduard Voiculescu Date: Thu, 17 Oct 2024 12:13:57 -0400 Subject: [PATCH] - address pr comments - move tracer calls to storage changes higher up --- x/evm/handler_test.go | 1 - x/evm/keeper/keeper.go | 4 ++-- x/evm/statedb/statedb.go | 20 +++++++++++++------- x/evm/tracers/firehose.go | 2 +- x/evm/tracers/firehose_test.go | 9 ++++----- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index af038088a1..e721f6c593 100644 --- a/x/evm/handler_test.go +++ b/x/evm/handler_test.go @@ -136,7 +136,6 @@ func (suite *HandlerTestSuite) TestHandleMsgEthereumTx() { for _, tc := range testCases { suite.Run(tc.msg, func() { - // todo: it seems we are missing a hook on a balance change? or something like that? suite.SetupTest() // reset //nolint tc.malleate() diff --git a/x/evm/keeper/keeper.go b/x/evm/keeper/keeper.go index bae0832897..018038ca4a 100644 --- a/x/evm/keeper/keeper.go +++ b/x/evm/keeper/keeper.go @@ -33,7 +33,6 @@ import ( ethermint "github.com/evmos/ethermint/types" "github.com/evmos/ethermint/x/evm/statedb" "github.com/evmos/ethermint/x/evm/types" - evmtypes "github.com/evmos/ethermint/x/evm/types" ) // CustomContractFn defines a custom precompiled contract generator with ctx, rules and returns a precompiled contract. @@ -141,7 +140,7 @@ func (k Keeper) ChainID() *big.Int { func (k *Keeper) InitChainer(ctx sdk.Context) { if tracer := cosmostracing.GetCtxBlockchainTracer(ctx); tracer != nil && tracer.OnBlockchainInit != nil { - tracer.OnBlockchainInit(evmtypes.DefaultChainConfig().EthereumConfig(k.ChainID())) + tracer.OnBlockchainInit(types.DefaultChainConfig().EthereumConfig(k.ChainID())) } } @@ -204,6 +203,7 @@ func (k *Keeper) PostTxProcessing(ctx sdk.Context, msg *core.Message, receipt *e return k.hooks.PostTxProcessing(ctx, msg, receipt) } +// SetTracer should only be called during initialization func (k *Keeper) SetTracer(tracer *cosmostracing.Hooks) { k.evmTracer = tracer } diff --git a/x/evm/statedb/statedb.go b/x/evm/statedb/statedb.go index 2458ff3ce5..b3cc167185 100644 --- a/x/evm/statedb/statedb.go +++ b/x/evm/statedb/statedb.go @@ -552,34 +552,40 @@ func (s *StateDB) SetCode(addr common.Address, code []byte) { oldCode := s.GetCode(addr) stateObject.SetCode(crypto.Keccak256Hash(code), code) + var oldCodeHash common.Hash + if oldCode != nil { + oldCodeHash = crypto.Keccak256Hash(oldCode) + } + if s.evmTracer != nil && s.evmTracer.OnCodeChange != nil { - s.evmTracer.OnCodeChange(addr, crypto.Keccak256Hash(oldCode), oldCode, crypto.Keccak256Hash(code), code) + s.evmTracer.OnCodeChange(addr, oldCodeHash, oldCode, crypto.Keccak256Hash(code), code) } } } // SetState sets the contract state. func (s *StateDB) SetState(addr common.Address, key, value common.Hash) { - stateObject := s.getOrNewStateObject(addr) - stateObject.SetState(key, value) - if s.evmTracer != nil && s.evmTracer.OnStorageChange != nil { s.evmTracer.OnStorageChange(addr, key, s.GetState(addr, key), value) } + + stateObject := s.getOrNewStateObject(addr) + stateObject.SetState(key, value) } // SetStorage replaces the entire storage for the specified account with given // storage. This function should only be used for debugging and the mutations // must be discarded afterward. func (s *StateDB) SetStorage(addr common.Address, storage Storage) { - stateObject := s.getOrNewStateObject(addr) - stateObject.SetStorage(storage) - if s.evmTracer != nil && s.evmTracer.OnStorageChange != nil { for key, value := range storage { s.evmTracer.OnStorageChange(addr, key, s.GetState(addr, key), value) } } + + stateObject := s.getOrNewStateObject(addr) + stateObject.SetStorage(storage) + } // Suicide marks the given account as suicided. diff --git a/x/evm/tracers/firehose.go b/x/evm/tracers/firehose.go index cf2493fe70..38054385a2 100644 --- a/x/evm/tracers/firehose.go +++ b/x/evm/tracers/firehose.go @@ -2430,7 +2430,7 @@ func (m Memory) GetPtr(offset, size int64) []byte { // work because the memory is going to be expanded before the operation is actually // executed so the memory will be of the correct size. // - // In this situtation, we must pad with zeroes when the memory is not big enough. + // In this situation, we must pad with zeroes when the memory is not big enough. reminder := m[offset:] return append(reminder, make([]byte, int(size)-len(reminder))...) } diff --git a/x/evm/tracers/firehose_test.go b/x/evm/tracers/firehose_test.go index 6556518352..d4f64f8883 100644 --- a/x/evm/tracers/firehose_test.go +++ b/x/evm/tracers/firehose_test.go @@ -393,12 +393,11 @@ func TestFirehose_reorderIsolatedTransactionsAndOrdinals(t *testing.T) { ordinals := maps.Keys(seenOrdinals) slices.Sort(ordinals) - // All ordinals should be in stricly increasing order - prev := -1 + // All ordinals should be in strictly increasing order + prev := 0 for _, ordinal := range ordinals { - if prev != -1 { - assert.Equal(t, prev+1, int(ordinal), "Ordinal %d is not in sequence", ordinal) - } + assert.Equal(t, prev+1, int(ordinal), "Ordinal %d is not in sequence", ordinal) + prev = int(ordinal) } }) }