ethermint icon indicating copy to clipboard operation
ethermint copied to clipboard

fix: skip codehash check when the code has been deleted in the evm state

Open JayT106 opened this issue 2 years ago • 5 comments

Closes: #1319

just ignores the codehash check when the evm account code is empty.


For contributor use:

  • [x] Targeted PR against correct branch (see CONTRIBUTING.md)
  • [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • [x] Code follows the module structure standards.
  • [x] Wrote unit and integration tests
  • [x] Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • [x] Added relevant godoc comments.
  • [x] Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • [x] Re-reviewed Files changed in the Github PR explorer

For admin use:

  • [ ] Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • [ ] Reviewers assigned
  • [ ] Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

JayT106 avatar Aug 31 '22 21:08 JayT106

Codecov Report

Merging #1320 (6523d81) into main (236ca33) will increase coverage by 0.01%. The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1320      +/-   ##
==========================================
+ Coverage   55.86%   55.88%   +0.01%     
==========================================
  Files         108      108              
  Lines       10017    10021       +4     
==========================================
+ Hits         5596     5600       +4     
  Misses       4140     4140              
  Partials      281      281              
Impacted Files Coverage Δ
x/evm/genesis.go 56.66% <100.00%> (+3.09%) :arrow_up:

codecov[bot] avatar Aug 31 '22 21:08 codecov[bot]

there seems to be conflicts with main?

yihuang avatar Sep 27 '22 03:09 yihuang

LGTM, can we merge this now @fedekunze ?

yihuang avatar Sep 28 '22 07:09 yihuang

@yihuang isn't the test with empty code hash still missing?

emptyCodeHash := crypto.Keccak256(nil)
codeHash := common.BytesToHash(emptyCodeHash).String()

danburck avatar Sep 29 '22 20:09 danburck

@yihuang isn't the test with empty code hash still missing?

emptyCodeHash := crypto.Keccak256(nil)
codeHash := common.BytesToHash(emptyCodeHash).String()

Which specific case do you have in mind, the default account has the empty code hash, so the previous test cases tested that.

yihuang avatar Sep 30 '22 00:09 yihuang