Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

Fail early on StateDB functions #566

Merged
merged 11 commits into from
Sep 17, 2021
Merged

Fail early on StateDB functions #566

merged 11 commits into from
Sep 17, 2021

Conversation

davcrypto
Copy link
Contributor

@davcrypto davcrypto commented Sep 16, 2021

Closes: #410

Description

Introduce stateErr fields into keeper so that stateDB functions enabled ability to detect previous error


For contributor use:

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

@orijbot
Copy link

orijbot commented Sep 16, 2021

@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #566 (205d167) into main (0463c8b) will decrease coverage by 0.67%.
The diff coverage is 19.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
- Coverage   53.39%   52.72%   -0.68%     
==========================================
  Files          64       64              
  Lines        5128     5233     +105     
==========================================
+ Hits         2738     2759      +21     
- Misses       2263     2320      +57     
- Partials      127      154      +27     
Impacted Files Coverage Δ
x/evm/keeper/statedb.go 73.35% <14.00%> (-12.87%) ⬇️
x/evm/keeper/keeper.go 74.56% <100.00%> (+0.14%) ⬆️
x/evm/keeper/state_transition.go 74.90% <100.00%> (+0.59%) ⬆️

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good in general, but the error is not cleared anywhere (I only see it on the test).

Can you also add a changelog entry under Improvements? 🙏

x/evm/keeper/statedb.go Outdated Show resolved Hide resolved
x/evm/keeper/statedb.go Show resolved Hide resolved
x/evm/keeper/statedb.go Outdated Show resolved Hide resolved
x/evm/keeper/statedb.go Show resolved Hide resolved
Co-authored-by: Federico Kunze Küllmer <[email protected]>
@davcrypto
Copy link
Contributor Author

Thanks @fedekunze for the review. Made some update and explanation according to the your comments. Lets discuss on it

x/evm/keeper/statedb.go Outdated Show resolved Hide resolved
@davcrypto davcrypto requested a review from fedekunze September 17, 2021 02:44
@fedekunze fedekunze enabled auto-merge (squash) September 17, 2021 06:13
@fedekunze fedekunze added the automerge Automatically merge PR once all prerequisites pass. label Sep 17, 2021
@fedekunze fedekunze merged commit 742b6d1 into evmos:main Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail early on StateDB functions
3 participants