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

Switch validator to eipv #2860

Merged
merged 47 commits into from
Aug 10, 2020
Merged

Conversation

lightclient
Copy link
Member

As discussed in the EIPIP meetings, I've been working on a new validator called eipv. It isn't feature complete yet, but it should be comparable to the existing validator.

@@ -1,7 +1,6 @@
---
eip: 196
title: Precompiled contracts for addition and scalar multiplication
on the elliptic curve alt_bn128
title: Precompiled contracts for addition and scalar multiplication on the elliptic curve alt_bn128
author: Christian Reitwiessner<[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

^^

@lightclient lightclient marked this pull request as ready for review August 6, 2020 23:11
@lightclient
Copy link
Member Author

lightclient commented Aug 6, 2020

@axic @MicahZoltu @gcolvin @Souptacular @nicksavers: thoughts on this PR?


FILES="$(ls EIPS/*.md | egrep "eip-[0-9]+.md")"
bundle exec eip_validator $FILES
eipv EIPS/ --ignore=title_max_length,missing_discussions_to --skip=eip-20-token-standard.md
Copy link
Member Author

Choose a reason for hiding this comment

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

Please note that the validator is currently not ensuring that titles are less than the max specified length and that each EIP has a discussions-to field. This is due to the number of non-conforming EIPs. I think formally increasing the max title length to support all current EIPs and then turning on the check is acceptable. With regards to the discussions-to field, there are some options:

  1. In a separate PR, go in and create discussions threads for every EIP missing one
  2. Do 1) in this PR
  3. Forget historic EIPs with no discussions-to and only check new ones

I prefer 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer 1 or 2. Anything that gets the system into a more standardized state so we can reliably use the bot going forward and don't need to maintain code to support malformed ancient EIPs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, prefer 1 or 2

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll follow the 1 path.

@eip-automerger
Copy link

eip-automerger commented Aug 6, 2020

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • File .travis-ci.sh is not an EIP
  • File .travis.yml is not an EIP
  • EIP 1 is in state Active, not Draft or Last Call
  • EIP 1057 is in state Accepted, not Draft or Last Call
  • Error checking file EIPS/eip-1337.md
  • EIP 1355 is in state Abandoned, not Draft or Last Call
  • Error checking file EIPS/eip-1485.md
  • Error checking file EIPS/eip-1571.md
  • Error checking file EIPS/eip-1613.md
  • Error checking file EIPS/eip-1620.md
  • EIP 1679 is in state Final, not Draft or Last Call
  • EIP 1884 is in state Final, not Draft or Last Call
  • Error checking file EIPS/eip-1895.md
  • EIP 196 is in state Final, not Draft or Last Call
  • EIP 197 is in state Final, not Draft or Last Call
  • Error checking file EIPS/eip-2009.md
  • EIP 214 is in state Final, not Draft or Last Call
  • Error checking file EIPS/eip-2477.md
  • EIP 601 is in state Final, not Draft or Last Call
  • Error checking file EIPS/eip-615.md
  • Error checking file EIPS/eip-698.md
  • Error checking file EIPS/eip-712.md
  • Error checking file EIPS/eip-884.md
  • Error checking file EIPS/eip-998.md
  • EIP 1078 requires approval from one of ([email protected])
  • EIP 1153 requires approval from one of (@AlexeyAkhunov)
  • EIP 1186 requires approval from one of ([email protected], [email protected])
  • EIP 1191 requires approval from one of (@juli)
  • EIP 1271 requires approval from one of (@izqui, @PhABC, @abandeali1, @catageek, @shrugs, @frangio)
  • EIP 1470 requires approval from one of (@thec00n)
  • EIP 1973 requires approval from one of (@lerajk, @qinjian)
  • EIP 2193 requires approval from one of (@ctzurcanu, @loredanacirstea)
  • EIP 2378 requires approval from one of (@MadeofTin)
  • EIP 2458 requires approval from one of (@edsonayllon)
  • EIP 2470 requires approval from one of (@3esmit)
  • EIP 2525 requires approval from one of (@Amxx)
  • EIP 2657 requires approval from one of (@MadeofTin)
  • EIP 2746 requires approval from one of (@juanfranblanco, @jaerith)
  • EIP 2767 requires approval from one of (@mudgen, @zemse)
  • EIP 634 requires approval from one of (@ricmoo)

@@ -4,7 +4,6 @@ title: EIP Purpose and Guidelines
status: Active
type: Meta
author: Martin Becze <[email protected]>, Hudson Jameson <[email protected]>, and others
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md
Copy link
Member Author

Choose a reason for hiding this comment

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

It's not clear what purpose this URL served here.

Copy link
Member

Choose a reason for hiding this comment

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

I have changed this file locally just never submitted a PR. My understanding it tries to signal "look at the history".

What I opted for was "Martin ..., Hudson ..., et al."

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, it was supposed to be a part of the authors field.

Copy link
Contributor

@MicahZoltu MicahZoltu Aug 7, 2020

Choose a reason for hiding this comment

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

Can we change the authors for EIP-1 to just "Contributors Like You". 🚎 /not-🚎

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong feeling towards this, but IMO it's outside the scope of this PR.

EIPS/eip-1485.md Outdated
@@ -1,7 +1,7 @@
---
eip: 1485
title: TEthashV1
author: trustfarm (KT Ahn - 안씨아저씨) <[email protected]>, trustfarm <[email protected]>
author: trustfarm KT Ahn - 안씨아저씨 <[email protected]>, trustfarm <[email protected]>
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this is the best way to handle this. I'm not sure what the author was hoping to convey using the parentheses.

Copy link
Contributor

@MicahZoltu MicahZoltu Aug 7, 2020

Choose a reason for hiding this comment

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

I would probably talk to the author but my gut says to just remove the stuff in parenthesis. It seems like that email address is a company owned address, and the person on the other end of that address is subject to change. EIP Authors should not be subject to change IMO, so I think just trustfarm is appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@3esmit
Copy link
Contributor

3esmit commented Aug 6, 2020

I approve the change in ERC-2470 date format.

author: Paweł Bylica (@chfast) <[email protected]>, Jean M. Cyr (@jean-m-cyr)
author: Paweł Bylica (@chfast), Jean M. Cyr (@jean-m-cyr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should include the person twice in this case, so that they are a valid author both by email and by GitHub handle?

Copy link
Contributor

Choose a reason for hiding this comment

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

(same comment elsewhere where this change was made)

@MicahZoltu
Copy link
Contributor

Can you give us a snippet of what the errors look like? One of my biggest complaints about the validator is how cryptic some of its errors are. Would be nice to get the error messaging improved...

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

If it is as functional as the previous one, and has someone to support it (you) then I say we move forward with it and see how things go. If they go poorly, we can always rollback.

I don't have any strong negative feelings towards any of the EIP updates you have made here, though I did leave some comments.

@MadeofTin
Copy link
Contributor

cc @Souptacular to merge. :)

@lightclient
Copy link
Member Author

Can you give us a snippet of what the errors look like? One of my biggest complaints about the validator is how cryptic some of its errors are. Would be nice to get the error messaging improved...

Here is what CI was spitting out at the beginning of the PR: https://travis-ci.org/github/ethereum/EIPs/jobs/715639437#L643-L686. The main goal was to minimize the number of runtime errors that the old validator was providing, which (by writing in Rust) I believe this validator has accomplished. The general form of errors is eip-N: error text. I plan to improve this in the near future to also point to the line where the error was encountered.

For a full list of supported errors, please see: https://github.com/lightclient/eipv/blob/0e84318896dfec2024206f8bab8a9625ba8a75e4/src/error.rs#L56-L102

@lightclient
Copy link
Member Author

Bumping, @axic, @Souptacular, @gcolvin -- any thoughts? Is this okay to merge? The longer we wait, the more conflicts that will pile up here.

@Souptacular Souptacular merged commit 3194278 into ethereum:master Aug 10, 2020
@Souptacular
Copy link
Contributor

Merged! Thank you for the hard work @lightclient.

@lightclient
Copy link
Member Author

Woohoo! Thank you @Souptacular.

@axic axic mentioned this pull request Sep 9, 2020
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
* switch to eipv

* fix

* fix

* 1153 remove trailing whitespace

* remove file name checks

* 615 remo whitespace before comma

* 884 remove extra single-quotes

* 1337 remove whitespace before comma

* 1057 remove extra spaces after comma

* 2470 update created date to Y/M/D format

* 1078 update required eips to be in ascending order

* 2477 update required eips to be in ascending order

* 1271 remove extra whitespace

* 2767 required eipupdated to be in ascending order

* 2525 update created date to Y/M/D format

* 2458 remove trailing whitespace

* 1884 remove trailing whitespace

* 712 authors should be on a single line

* 601 remove extra whitespace

* 1485 remove unneeded parentheses

* 634 remove trailing whitespace

* 2657 update discussions-to to correct spelling

* 2009 remove trailing whitespace

* 998 required eips updated to be in ascending order

* 1186 remove trailing whitespace

* 1470 remove extra whitespace

* 1895 update created date to Y/M/D format

* 2747 remove extra whitespace

* 1613 remove leading whitespace

* 1571 can'have both handle and email in author field

* 1191 remove trailing whitespace

* 1973 remove trailing whitespace

* 196 don't wrap title field

* 1679 required eips must be in ascending order

* 1620 author can't have both handle and email

* 197 don't line wrap title field

* 2378 remove extra newline

* 1355 author can't have both handle and email

* 698 update created date to Y/M/D format

* 2193 required eips must be in ascending order

* 214 remove extra info after author email

* use v0.0.3 of eipv

* 1 remove malformed field

* bump eipv to v0.0.4

* cache eipv build

* 1485 remove extra author info

* 2771 removing extra whitespaces
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
* switch to eipv

* fix

* fix

* 1153 remove trailing whitespace

* remove file name checks

* 615 remo whitespace before comma

* 884 remove extra single-quotes

* 1337 remove whitespace before comma

* 1057 remove extra spaces after comma

* 2470 update created date to Y/M/D format

* 1078 update required eips to be in ascending order

* 2477 update required eips to be in ascending order

* 1271 remove extra whitespace

* 2767 required eipupdated to be in ascending order

* 2525 update created date to Y/M/D format

* 2458 remove trailing whitespace

* 1884 remove trailing whitespace

* 712 authors should be on a single line

* 601 remove extra whitespace

* 1485 remove unneeded parentheses

* 634 remove trailing whitespace

* 2657 update discussions-to to correct spelling

* 2009 remove trailing whitespace

* 998 required eips updated to be in ascending order

* 1186 remove trailing whitespace

* 1470 remove extra whitespace

* 1895 update created date to Y/M/D format

* 2747 remove extra whitespace

* 1613 remove leading whitespace

* 1571 can'have both handle and email in author field

* 1191 remove trailing whitespace

* 1973 remove trailing whitespace

* 196 don't wrap title field

* 1679 required eips must be in ascending order

* 1620 author can't have both handle and email

* 197 don't line wrap title field

* 2378 remove extra newline

* 1355 author can't have both handle and email

* 698 update created date to Y/M/D format

* 2193 required eips must be in ascending order

* 214 remove extra info after author email

* use v0.0.3 of eipv

* 1 remove malformed field

* bump eipv to v0.0.4

* cache eipv build

* 1485 remove extra author info

* 2771 removing extra whitespaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants