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

Alternative EIP-based HF file format in Common as preparation for Yolo v2 integration #876

Merged
merged 2 commits into from
Sep 21, 2020

Conversation

holgerd77
Copy link
Member

This PR adds an alternative HF file format in Common which further decouples the HF and EIP structure, the following is an example for the new format used in the berlin.json HF file:

{
  "name": "berlin",
  "comment": "HF targeted for July 2020 following the Muir Glacier HF",
  "url": "https://eips.ethereum.org/EIPS/eip-2070",
  "status": "Draft",
  "eips": [ 2315 ]
}

This generally allows for more flexibility when defining chain and HF structures and explicitly prepares for integrating ephemeral testnets like the upcoming Yolo v2 testnet which integrates the same EIPs as the berlin HF. In the current structure this would demand the duplication of all parameters affected.

I first wanted to go down the whole path and convert all HF files - we actually still can do this - but then I realized that this would lead to a lot of extra files created respectively file reads necessary on Common instantiation. Not sure if this is an issue but under the light of the current performance discussions I couldn't decide on a design here (an alternative would be to inline all EIP configs under one eip.json file, a bit on the cost of oversight). We can discuss this in the call later the day.

@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

Merging #876 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 77.08% <ø> (+0.49%) ⬆️
#blockchain 80.45% <ø> (+0.31%) ⬆️
#common 93.01% <100.00%> (+0.15%) ⬆️
#ethash 82.22% <ø> (-1.12%) ⬇️
#tx 92.94% <ø> (ø)
#vm 88.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

(note: the slight file structure changes on the HF files removing the additional "eip" parameter clustering is just cosmetics, these values are not read programatically anywhere)

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

lgtm!

@ryanio ryanio merged commit 38dc920 into master Sep 21, 2020
@holgerd77 holgerd77 deleted the common-separate-hfs-and-eips branch September 22, 2020 15:45
@holgerd77
Copy link
Member Author

@ryanio thanks for merging here, only just discovered. 😄

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