-
Notifications
You must be signed in to change notification settings - Fork 762
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
fix(forkdiff): update sub definitions to fix hydration #244
base: optimism
Are you sure you want to change the base?
Conversation
Here is the generated forkdiff report page, I had to |
|
||
Last updated: Tue Feb 13 19:58:26 UTC 2024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last updated: Tue Feb 13 19:58:26 UTC 2024 | |
Last updated: Tue Feb 13 19:58:26 UTC 2024 |
We're 100% going to forget updating this. The commit hash in the diff page is the best source of truth for last udpate.
@@ -1,6 +1,6 @@ | |||
title: "op-geth - go-ethereum fork diff overview" | |||
footer: | | |||
Fork-diff overview of [`op-geth`](https://github.com/ethereum-optimism/op-geth), a fork of [`go-ethereum`](https://github.com/ethereum/go-ethereum). | |||
Fork-diff overview of [`op-geth`](https://github.com/ethereum-optimism/op-geth), a fork of [`go-ethereum, v1.13.8`](https://github.com/ethereum/go-ethereum). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we won't maintain manual versions in multiple places. Can you revert this part of the change?
@@ -166,7 +168,6 @@ def: | |||
- "eth/ethconfig/config.go" | |||
- title: Tx gossip disable option | |||
globs: | |||
- "eth/handler.go" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it spit out the error because of the usage of this diff in two places. We should add a description of the eth/handler.go
changes in one place, to keep some documentation of the total set of changes.
Description
This updates your
fork.yaml
file so that forkdiff binary can successfully build the report page.Under the existing
fork.yaml
file, it fails to build and gives the following error:Tests
I ran forkdiff with the updated changes and was able to successfully build the report page.
Additional context
There is no validation of the file,
fork.yaml
. Additionally, there exists no timestamp or versioning information in the generated report page.I added two hard coded values:
1. the version of the corresponding git hash for go-ethereum, v1.13.8
2. a 'Last Updated' value, using TZ=UTC
Considerations
fork.yaml
should be validated at build, additionally I found no configuration specification for detailing diffing policy or setup.For reference, this is one configuration
This is beyond the scope of this PR, I just thought to mention it, as it makes maintainer job easier.