-
Notifications
You must be signed in to change notification settings - Fork 193
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
Add Granite to network upgrades page. #890
Add Granite to network upgrades page. #890
Conversation
A separate commit will address broken anchor links to specs/superchain-upgrades.html
✅ Deploy Preview for docs-optimism ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
So questions (some are about the specs repo):
|
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Tip Announcements
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional context usedPath-based instructions (1)
LanguageTool
Additional comments not posted (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
I made a PR in specs to add the Granite page to the sidebar and changed all hard fork specs links to go to their details page: ethereum-optimism/specs#367 |
ff8eba4
to
cb0b670
Compare
cb0b670
to
42493fa
Compare
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pages/builders/node-operators/network-upgrades.mdx (1)
26-26
: Suggestion: Fix cell padding inconsistencies in the upgrade activation table.The lint tool has flagged two warnings regarding inconsistent cell padding in the upgrade activation table:
- Line 26: Cell should be padded with 1 space, not 12
- Line 28: Cell should be padded with 1 space, not 12
While these inconsistencies do not affect the functionality or clarity of the content, maintaining consistent formatting can improve the overall readability and maintainability of the documentation.
Consider updating the cell padding to ensure consistency throughout the table:
-| [Granite](https://specs.optimism.io/protocol/granite/overview.html) | (https://gov.optimism.io/t/upgrade-proposal-10-granite-network-upgrade/8733) | Wed Sep 11 16:00:01 UTC 2024 (`1726070401`) | Mon Aug 12 16:00:00 UTC 2024 (`1723478400`) | +| [Granite](https://specs.optimism.io/protocol/granite/overview.html) | (https://gov.optimism.io/t/upgrade-proposal-10-granite-network-upgrade/8733) | Wed Sep 11 16:00:01 UTC 2024 (`1726070401`) | Mon Aug 12 16:00:00 UTC 2024 (`1723478400`) | -| [Ecotone](https://specs.optimism.io/protocol/ecotone/overview.html) | (https://gov.optimism.io/t/upgrade-proposal-5-ecotone-network-upgrade/7669) | Thu Mar 14 00:00:01 UTC 2024 (`1710374401`) | Wed Feb 21 17:00:00 UTC 2024 (`1708534800`) | +| [Ecotone](https://specs.optimism.io/protocol/ecotone/overview.html) | (https://gov.optimism.io/t/upgrade-proposal-5-ecotone-network-upgrade/7669) | Thu Mar 14 00:00:01 UTC 2024 (`1710374401`) | Wed Feb 21 17:00:00 UTC 2024 (`1708534800`) |Also applies to: 28-28
Tools
GitHub Check: lint
[warning] 26-26:
Cell should be padded with 1 space, not 12
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pages/builders/node-operators/network-upgrades.mdx (1)
34-34
: Improve clarity by adding a comma and preposition.To enhance the clarity of the sentence, consider making the following changes:
- Add a comma after "upgrade changes" to separate the two clauses.
- Add the preposition "in" before "order" to specify the relationship between the changes and the activation order.
Here's the suggested revision:
-These are the summary of each network upgrade changes order by the most recent +These are the summary of each network upgrade changes, in order by the most recentMaking these small adjustments will improve the readability and grammatical correctness of the sentence.
Tools
LanguageTool
[uncategorized] ~34-~34: Possible missing comma found.
Context: ...are the summary of each network upgrade changes order by the most recent activation. Th...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~34-~34: Possible missing preposition found.
Context: ...summary of each network upgrade changes order by the most recent activation. These ar...(AI_HYDRA_LEO_MISSING_IN)
Hey @pauldowman thanks for opening this PR. Our team will take a look but we're a short staffed this week so it might take a couple days |
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pages/builders/node-operators/network-upgrades.mdx (1)
34-35
: Add a comma to improve readability.Consider adding a comma after "upgrade" to separate the two parts of the sentence:
-These are the summaries of each network upgrade changes ordered by the most +These are the summaries of each network upgrade, changes ordered by the mostTools
LanguageTool
[uncategorized] ~34-~34: Possible missing comma found.
Context: ...e the summaries of each network upgrade changes ordered by the most recent activation....(AI_HYDRA_LEO_MISSING_COMMA)
Granite goes live tomorrow, and the contracts will be upgraded today, there will be comms about it today and I think we really should have the docs updated if at all possible! The diff is pretty tiny. There are failing checks for broken links but most weren't touched in this PR. The new one is the Granite specs page which will exist after this PR is merged: ethereum-optimism/specs#367 |
Hey @pauldowman, went ahead and approved this! Looks great to me and it seems like most of your questions have been resolved with your PR to the specs repo. Let me know if you need anymore assistance. For future reference, GH can be a bit noisy and hard to parse through. If there's ever something time sensitive like this that we haven't gotten to, feel free to ping us in the #docs slack channel. Thanks! |
We can merge once the specs PR is merged |
Thanks @bradleycamacho! The specs PR is merged now. Should I merge this or is that usually for someone on the docs team to do? |
Adds Granite hard fork times to the network upgrades page.
This PR is not ready yet because I noticed that there are broken anchor links to specs/superchain-upgrades.html, e.g. https://specs.optimism.io/protocol/superchain-upgrades.html#fjord, and there are pages in the specs repo for each hard fork that should probably be linked to instead but the granite page isn't ready.