Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

evm: various fixes #319

Merged
merged 12 commits into from
Jun 4, 2020
Merged

evm: various fixes #319

merged 12 commits into from
Jun 4, 2020

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented May 29, 2020

Description

  • remove multiple KVStores and switch to use prefix stores
  • move SetBlockBloom to EndBlock
  • bug fixes

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)

@fedekunze fedekunze self-assigned this May 29, 2020
x/evm/abci.go Show resolved Hide resolved
x/evm/keeper/querier.go Outdated Show resolved Hide resolved
@@ -75,6 +75,7 @@ func NewGenesisStorage(key, value ethcmn.Hash) GenesisStorage {
func DefaultGenesisState() GenesisState {
return GenesisState{
Accounts: []GenesisAccount{},
TxsLogs: []TransactionLogs{},
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, this will be handy

logs, err := s.GetLogs(ch.txhash)
if err != nil {
// panic on unmarshal error
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

hm... panic seems intense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an internal error that is only present is there's an unmarshal error. We want to panic here bc otherwise we could persist invalid state and have to manually stop the chain (which requires coordination of +1/3 validatiors) and then roll-back to the height before the first error instance of this error occurred.

Having a panic prevents this overhead and allows us to detect these errors during development 🙂

@fedekunze fedekunze marked this pull request as ready for review May 29, 2020 21:41
@fedekunze
Copy link
Contributor Author

@noot this is ready for review again. I'll write tests for the logs validations after I finish the WebSocket PR

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

code looks good, but please fix tests + lint!

@fedekunze fedekunze requested a review from noot June 1, 2020 12:26
@noot
Copy link
Contributor

noot commented Jun 1, 2020

@fedekunze I think this causes some issue with transaction receipts, I ran the rpc tests and the TestEth_GetTransactionReceipt fails now because eth_getTransactionReceipt returns nil

ETHERMINT_NODE_HOST=http://localhost:8545 ETHERMINT_INTEGRATION_TEST_MODE=stable go test ./tests/... -test.v -test.run ^TestEth_GetTransactionReceipt

or make it-tests

Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

running the rpc tests separately works, so I'll approve, but we should look into why it doesn't work when you run them all together

@fedekunze fedekunze merged commit 427e96c into development Jun 4, 2020
@fedekunze fedekunze deleted the fedekunze/block branch June 4, 2020 10:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants