Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add common boilerplate for Berlin hardfork #755

Merged
merged 2 commits into from
May 29, 2020

Conversation

cgewecke
Copy link
Contributor

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #755 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
- Coverage   92.02%   92.00%   -0.02%     
==========================================
  Files          46       43       -3     
  Lines        2998     2991       -7     
  Branches      469      469              
==========================================
- Hits         2759     2752       -7     
  Misses        144      144              
  Partials       95       95              
Flag Coverage Δ
#account 94.11% <ø> (ø)
#block 88.09% <ø> (ø)
#blockchain 88.74% <ø> (ø)
#common 94.11% <ø> (-0.26%) ⬇️
#tx 94.02% <ø> (ø)
#vm 93.09% <ø> (ø)
Impacted Files Coverage Δ
packages/common/src/genesisStates/index.ts
packages/common/src/chains/index.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56d02e7...e7847df. Read the comment docs.

"name": "berlin",
"block": null,
"consensus": "poa",
"finality": null
Copy link
Member

Choose a reason for hiding this comment

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

Just some side note, in case anyone is wondering: these consensus and finality fields are from the old Casper/sharding days when it was still planned to do the consensus/finality switches on-chain, modeled this this way to be able to do the respective switches cleanly along the Casper HFs.

So these can very well removed, for the moment this is I guess rather distracting and confusing than anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@holgerd77 Should we remove those fields in this PR or in a separate one?

Copy link
Member

Choose a reason for hiding this comment

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

Think I will merge here now since this is ready-for-merge here anyhow (thanks!). If you want you can open a separate PR on this, not too pressing though.

@holgerd77
Copy link
Member

Great! 😄 I would guess the Kovan network will do this HF as well so I think you can also add the boilerplate code to the kovan.json chain file? Otherwise this already looks good.

The base fees for the opcodes never made it to the Common library and are still collected in opcodes.ts. This should be changed I guess at some point and the base fees also moved over, with a start in the respective HF file on the "birth" of the opcode. For now it should be ok to add these to the VM as you've already done.

These are the only fees from the EIP to take into account I guess?

On the VM side you can already add berlin to the list of supportedHardforks (from my side also in this PR), then it get's allowed to use this HF switch in the VM and it should be easier to integrate on tests.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit df1ac84 into master May 29, 2020
@holgerd77 holgerd77 deleted the berlin-common-boilerplate branch May 29, 2020 07:13
@evertonfraga evertonfraga added this to the VM v5 milestone Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants