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

common: update status fields for EIPs and hardforks #1777

Merged
merged 2 commits into from
Mar 11, 2022

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Mar 10, 2022

Closes #1773

Have only updated the "status" fields, the rest are prettier formats.

(I wonder if we should open a separate PR to format all of our JSON and markdown files in the monorepo at once, the command from root would be: npx prettier "**/*.{json,md}" --write)

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #1777 (6288c49) into master (ba916e5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 85.57% <ø> (ø)
blockchain 83.28% <ø> (ø)
client 75.82% <ø> (ø)
common 93.90% <ø> (ø)
devp2p 82.56% <ø> (ø)
ethash 90.76% <ø> (ø)
trie 80.02% <ø> (ø)
tx 88.20% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.17% <ø> (ø)

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

@@ -3,7 +3,7 @@
"number": 2315,
"comment": "Simple subroutines for the EVM",
"url": "https://eips.ethereum.org/EIPS/eip-2315",
"status": "Draft",
"status": "EXPERIMENTAL",
Copy link
Member

Choose a reason for hiding this comment

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

Ah, no, this status here is really the EIP status. This EXPERIMENTAL is only used in the VM to then indicate the status of the implementation.

I guess if we update here to EXPERIMENTAL and mix this with the EIP states it get's chaotic. That said, this status field is also only used internally, I added this when setting up the Common class with the above semantics in mind. Not sure if it's very helpful and we generally want to keep or not.

We might want to discuss separately. In doubt we just repeat redundant information here, which will likely always get outdated again and again. I guess if we decide we could even "just remove" without the fear of any breaking-change consequences.

Nevertheless: for now will update here back to Draft to keep things consistent.

Copy link
Member

Choose a reason for hiding this comment

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

But, to add some own opinion on that: I am currently also undecided (to remove or not), this might also be some useful helper on the status of EIPs we have implemented (otherwise one would likely not always look this up manually).

@holgerd77 holgerd77 force-pushed the update-common-status-fields branch from 9adfb1f to 6288c49 Compare March 11, 2022 10:39
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 b5d9fb0 into master Mar 11, 2022
@holgerd77 holgerd77 deleted the update-common-status-fields branch March 11, 2022 11:06
@jochem-brouwer
Copy link
Member

Great, thanks! But moving 2315 to experimental in VM has not been addressed here (have reopened the issue).

@ethereumjs ethereumjs deleted a comment Jul 11, 2022
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.

Common: Update EIP status fields, VM: move EIP-2315 to EXPERIMENTAL state
3 participants