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

fix evm parameters and block gas limit in getBlockByHeight and getBlockByHash #312

Merged
merged 5 commits into from
Jul 19, 2021

Conversation

thomas-nguy
Copy link
Contributor

Closes: #302

Description

  • Set correct block gas limit in Block response (GetBlockByNB and GetBlockByHash)/
  • Set correct block gas limit in evm parameters
  • Fix hash function in evm parameters

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)

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #312 (e0a3e4c) into main (0a8f02e) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #312      +/-   ##
==========================================
- Coverage   47.60%   47.46%   -0.14%     
==========================================
  Files          46       46              
  Lines        3151     3160       +9     
==========================================
  Hits         1500     1500              
- Misses       1576     1585       +9     
  Partials       75       75              
Impacted Files Coverage Δ
types/block.go 0.00% <0.00%> (ø)
x/evm/keeper/state_transition.go 5.92% <0.00%> (-0.38%) ⬇️

@thomas-nguy thomas-nguy force-pushed the thomas/302-FixMultiCall branch from cdd1322 to 73c0f3a Compare July 16, 2021 15:47
@thomas-nguy thomas-nguy changed the title fix evm parameters and get limit fix evm parameters and get block gas limit in getBlockByHeight and getBlockByHash Jul 16, 2021
@thomas-nguy thomas-nguy changed the title fix evm parameters and get block gas limit in getBlockByHeight and getBlockByHash fix evm parameters and block gas limit in getBlockByHeight and getBlockByHash Jul 16, 2021
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.

one suggestion, otherwise LGTM. Please open an issue on SDK

Comment on lines 70 to 71
// Note: we cannot use k.ctx.HeaderHash() because the headerHash it set only at begin block
// so it won't be set in case of simulated context
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it set for checkTxState too? can you open an issue on the SDK to fix this and include it on v0.43?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is also set for checkContext.
I realised my comment might be confusing. What I want to mean is that in case of a "query" context, such as simulating a transaction through "EthCall", then the header hash won't be set.
I have rewrote the comment, let me know if its clear now

x/evm/keeper/state_transition.go Show resolved Hide resolved
@thomas-nguy thomas-nguy force-pushed the thomas/302-FixMultiCall branch from fcb671c to 9dea9e2 Compare July 17, 2021 00:19
@thomas-nguy thomas-nguy force-pushed the thomas/302-FixMultiCall branch from 9dea9e2 to f2b707e Compare July 17, 2021 00:21
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
@fedekunze fedekunze enabled auto-merge (squash) July 17, 2021 08:49
@fedekunze fedekunze merged commit c8b88a3 into evmos:main Jul 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chain-related params returning zero value
2 participants