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

Pending tasks for ERC migration #1

Open
2 of 10 tasks
lightclient opened this issue Oct 25, 2023 · 16 comments
Open
2 of 10 tasks

Pending tasks for ERC migration #1

lightclient opened this issue Oct 25, 2023 · 16 comments
Labels
bug Something isn't working w-stale

Comments

@lightclient
Copy link
Member

lightclient commented Oct 25, 2023

TODO

  • Repos split
  • Website unified on EIPs repo
  • Activate eip-bot on ERC repo (waiting for token)
  • Replace eip: xxxx in ERC template with ERC
  • Sort out eip-1 vs. erc-1
  • More thorough rewrite of the readme
  • Figure out cross-linking
  • Figure out if any of the other non-core EIPs should be moved over here
  • Remove placeholders in EIPs repo
  • Ensure relative links work as intended
@lightclient lightclient added the bug Something isn't working label Oct 25, 2023
@Pandapip1 Pandapip1 pinned this issue Oct 25, 2023
@cfries
Copy link
Contributor

cfries commented Oct 26, 2023

Note: Currently all pull request fail due to the HTML Validator reporting broken links.
See https://github.com/ethereum/ERCs/actions/runs/6657294483/job/18091699250?pr=25

A pull request of an ERC fails even if that ERC does not have broken links, because others still have them. However, correcting them will make it harder to migrate pull request, as this will generate conflicts in the git merge.
Not sure if these is a practical solution: but one could

  • disable the HTML validator for a while, or,
  • create a dummy EIP directory (as part of the validation process maybe) (maybe with files containing rHTML redirects)
  • fix all the links and live with merge conflicts.

@lightclient
Copy link
Member Author

@cfries thanks for noting this. We're looking into a solution. I would rather not disable the HTML validator because it is a good forcing function to fix this ASAP. The main interim solution I am considering is to add more logic to the jekyll build script in ethereum/eips to convert ERC -> EIP where necessary. Unfortunately that isn't a great solution because then on the compiled site in many places where the ERC should be referred to as ERC it is referred to as EIP.

The longer term solution is to either i) break the website into a separate repo and make it work correctly for both EIP and ERC or ii) to simply deploy an ERC only jekyll instance. Obviously I prefer ii) because it is much simpler.

@cfries
Copy link
Contributor

cfries commented Oct 26, 2023

@lightclient thanks for the reply. I believe there is kind of "deadlock" because an ERC-X is getting a fail because the links in ERC-Y are wrong. Would it be possible to modify the validation process in such a way that it only checks the consistency of links within a pull request's ERC. This would allow to get the PR through.

@lightclient
Copy link
Member Author

I'm hesitant to make any changes to the validation checks until we have our next EIPIP call on wednesday. I have removed HTMLProofer as a required check to merge PRs though.

@tbergmueller
Copy link
Contributor

tbergmueller commented Oct 27, 2023

Since Website is already marked as complete;
On the example of our ERC-6956:
Currently, the EIPs and ERCs are only available via https://eips.ethereum.org/EIPS/eip-6956
Maybe you could add https://eips.ethereum.org/ERCS/erc-6956 to match the new naming scheme? To avoid confusion, also https://eips.ethereum.org/EIPS/eip-6956 should point to the erc-6956.md file from this ERCs repo [not sure if it does already].

Edit, missed the webpage, thx @Edoumou (https://ercs.ethereum.org/ERCS/erc-6956)

CodeSpell
Discovered in https://github.com/ethereum/ERCs/actions/runs/6664676695/job/18112761827?pr=43 ;
A lot of ERCs have spelling issues. I manually corrected mine, the suggestions were all correct. Maybe following all unambiguous suggestions of CodeSpell is an option to resolve this fast for all ERCs?

EIP Walidator
Is running now for 19m 21sec ... Runner fails to pick up the job respectively takes a very long time. This could be related to several people working and pushing simultanously a.t.m.

2023-10-27T08:17:03.6049222Z Requested labels: ubuntu-latest
2023-10-27T08:17:03.6049488Z Job defined at: ethereum/ERCs/.github/workflows/ci.yml@refs/pull/43/merge
2023-10-27T08:17:03.6049574Z Waiting for a runner to pick up this job...
2023-10-27T08:17:03.9988102Z Job is waiting for a hosted runner to come online.
2023-10-27T08:17:08.3452129Z Job is about to start running on the hosted runner: GitHub Actions 9 (hosted)

EIP Walidator Bug 1
After ~29min a runner picked up the job, failed due to quota limit; See #45 ... This happened on the first run of this PR as well (can't find logs anymore)

EIP-Walidator Bug 2
When Walidator runs, it fails in checking the requires in the ERC header, See #46

@cfries
Copy link
Contributor

cfries commented Oct 27, 2023

I try to find out if there are some easy regexp to fix the broken links. One can easily bring the 1800 broken links down to 90.
The remaining issues require a bit more work maybe (crosslinking). See here for an example: #42

@Edoumou
Copy link
Contributor

Edoumou commented Oct 27, 2023

https://eips.ethereum.org/ERCS/erc-6956

Hey @tbergmueller. There is already a web page for ERCs. Here is the link that points to your proposal:

https://ercs.ethereum.org/ERCS/erc-5521

@tbergmueller
Copy link
Contributor

Thx @Edoumou , missed that; And thx for crediting me with another proposal :D mine is https://ercs.ethereum.org/ERCS/erc-6956

@Edoumou
Copy link
Contributor

Edoumou commented Oct 27, 2023

Thx @Edoumou , missed that; And thx for crediting me with another proposal :D mine is https://ercs.ethereum.org/ERCS/erc-6956

My bad @tbergmueller,

Sure, yours is https://ercs.ethereum.org/ERCS/erc-6956

@lightclient
Copy link
Member Author

@tbergmueller the issue with the codespell also appears to be a quota limit that causes codespell to run on the entire repo instead of only changed files. I'm not sure what we can really do to reduce the likelihood of hitting the quota limits. It seems like a somewhat rare issue, but we should monitor to see if it is more common than I am thinking.

@tbergmueller
Copy link
Contributor

@lightclient Thanks, I saw you retried the CodeSpell - Could you please trigger Walidator as well, assuming that quota-limit is not an issue any more?
There is no way for me to retry runs, right? I'd need to push again some changes to the PR?

Copy link

github-actions bot commented Nov 4, 2023

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

Copy link

github-actions bot commented Dec 1, 2023

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

@github-actions github-actions bot added w-stale and removed w-stale labels Dec 1, 2023
Copy link

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

Copy link

github-actions bot commented Feb 3, 2024

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2024
SamWilsn added a commit that referenced this issue Mar 5, 2024
* Update erc-7007.md to add opML compatibility (#1)

* Update erc-7007.md to add opML compatibility

* Update erc-7007.md param name

* Create IOpmlLib.sol

* Update and rename ERC7007.sol to ERC7007_zkml.sol

* Update IOpmlLib.sol

* Update IOpmlLib.sol

* Update IOpmlLib.sol

* Create ERC7007_opml.sol

* Update ERC7007_opml.sol

* Update IOpmlLib.sol

* feat: update implementation to include owner address

* feat: update authorship, added Updatable extension, update metadata with `proof_type`

* fix: full term before abbreviation

* feat: Update ERC7007 contracts and add IERC7007Updatable interface

* feat: Add rationale and security considerations

* feat: `update` function to use `verify` in `require` statement

* misc: capitalization

* xhyu patch 1 (#2)

* misc: improve abstract & motivation

* fix: Spec intro & Model Publication wording

* fix: Rewrite part of motivation

* corrected some small typos
* rewrote the last two paragraphs in Motivation

---------

Co-authored-by: drCathieSo.eth <socathie@users.noreply.github.com>

* fix: proposal reference and header

* fix: file references

* Update erc-7007.md: Fix wordings (#4)

* Update erc-7007.md: Fix wordings

* update license

* Update description and motivation -> rationale

---------

Co-authored-by: drCathieSo.eth <socathie@users.noreply.github.com>

* Add workflow (#5)

* Add workflow diagram

* Add workflow description

* Apply suggestions from code review

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>

* Update erc-7007.md

* Fix uppercase keywords

* Update erc-7007.md

---------

Co-authored-by: 李婷婷 Lee Ting Ting <tina1998612@users.noreply.github.com>
Co-authored-by: xhyumiracle <xhyumiracle@gmail.com>
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
@Pandapip1 Pandapip1 reopened this Apr 29, 2024
@github-actions github-actions bot removed the w-stale label Apr 30, 2024
Copy link

github-actions bot commented May 7, 2024

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working w-stale
Projects
None yet
Development

No branches or pull requests

6 participants
@tbergmueller @cfries @lightclient @Pandapip1 @Edoumou and others