Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 2m 44s —— View job Claude finished @tac0turtle's task in 2m 30s —— View job Documentation Review CompleteI've conducted a comprehensive review of this documentation rewrite PR. This is an ambitious effort to restructure and expand the documentation, but there are several critical issues that need to be addressed before merging. Executive SummaryMajor Issues:
Recommendation: Substantial revision needed before merge. The issues below should be addressed systematically. Critical Issues1. Severe Content DuplicationBlock Lifecycle Documentation (CRITICAL)
Recommendation:
Getting Started vs. ev-abci Integration (HIGH)Multiple reviewers (pthmas, gemini-code-assist) identified duplication: Duplicated:
The ev-abci folder should be the single source of truth for ev-abci integration, with getting-started providing only minimal quickstart pointers. 2. Technical InaccuraciesBased Sequencing Description (MEDIUM - docs/guides/advanced/based-sequencing.md:63)Issue: Documentation states "No malicious actor concern - invalid blocks are automatically rejected by validation rules" Reality: The code shows based sequencing uses Location: docs/guides/advanced/based-sequencing.md:55-62 Retry Strategy Documentation (MEDIUM - docs/concepts/block-lifecycle.md:296,340)Inconsistency: Line 296 states exponential backoff is "capped at Code Check: I searched for Location: docs/concepts/block-lifecycle.md:296, 340 Fix backoff cap inconsistency → Deprecated Namespace Flag (MEDIUM - docs/guides/da-layers/celestia.md)Issue: Documentation describes Location: docs/guides/da-layers/celestia.md (description section vs. command example) Gas Price Detection Claims (LOW - docs/concepts/block-lifecycle.md:165)Issue: Reviewer pthmas notes: "Maybe a bit misleading? automatic gas price detection is set by default unless someone specifies with the Location: docs/concepts/block-lifecycle.md:165 (GasPrice configuration description) 3. Structural and Organizational IssuesUnnecessary Navigation Layers (MEDIUM)
Location: docs/getting-started/choose-your-path.md Reviewer comment (pthmas): "Not sure we need this page" Overview vs. Concepts Duplication (MEDIUM)
Reviewer comment (pthmas): "this feels also a bit like a duplicate of the docs/concepts folder. imo the overview folder should be one page" Placeholder Files (HIGH)Several files were flagged by gemini-code-assist review as containing only TODO comments:
Action Required: Either complete these files or remove them from this PR. 4. Documentation Quality IssuesInconsistent Migration Guidance (HIGH)docs/ev-abci/migration-from-cometbft.md:191 Gemini-code-assist review identified incorrect start command pattern: // Documented (INCORRECT):
RunE: func(cmd *cobra.Command, _ []string) error {
return server.Start(cmd, evabci_server.StartHandler(cmd, app.New))
},
// Should be:
RunE: func(cmd *cobra.Command, _ []string) error {
return evabci_server.StartHandler(cmd, app.New)
},This error could prevent successful migration for users. Attester Implementation Status (MEDIUM)docs/overview/architecture.md:89 - Reviewer alpe notes: "Should specify that this is not yet available" The architecture table lists "Attester" as a node type, but this may not be implemented yet. Location: docs/overview/architecture.md:89 Forced Inclusion / Based Sequencing Confusion (MEDIUM)docs/guides/advanced/forced-inclusion.md:101-115,128 Reviewer pthmas comments:
These sections conflate forced inclusion (censorship resistance mechanism) with based sequencing (fully decentralized mode). 5. Minor IssuesFormatting Issues
Unnecessary Summary Sections (LOW)Reviewer pthmas notes: "I think it's useless to have a summary at the end of the page"
Maintenance Concerns (MEDIUM)docs/reference/interfaces/da.md - Reviewer pthmas: "It's probably hard to maintain all the interfaces changes up to date and documented. Could we just point to the correct files in the repo?" Documenting full interfaces in markdown will quickly become stale. Consider linking to GoDoc or code instead. docs/guides/da-layers/celestia.md - Reviewer pthmas: "hard to maintain, should we just link the celestia docs?" External dependencies (like Celestia setup) should reference official docs rather than duplicating them. Log Messages Without Context (LOW)docs/guides/operations/troubleshooting.md - Reviewer pthmas notes the error examples provide messages without actionable context or troubleshooting steps beyond what users would already know. Positive AspectsDespite the issues, this PR has significant value:
RecommendationsImmediate Actions (Before Merge)
Medium-Term Improvements
Testing RecommendationsSince I cannot run markdown linters without permissions, the team should: # Run markdown linting
make lint
# Check for broken internal links
# (consider adding a link checker to CI)
# Verify code examples compile/run
# (test the integration examples)SummaryThis is valuable work that significantly expands the documentation, but it needs substantial revision before merge. The most critical issue is content duplication which will cause maintenance problems immediately. Technical inaccuracies could mislead users and cause failed deployments. Suggested Approach:
The documentation framework and structure are sound - the content just needs consolidation and accuracy review. |
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's documentation, providing a more organized and in-depth resource for developers and operators. It covers fundamental concepts, practical implementation steps for different execution environments, operational best practices, and detailed technical specifications, making it easier for users to understand, integrate, and manage Evolve-based chains. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3026 +/- ##
=======================================
Coverage 61.64% 61.64%
=======================================
Files 111 111
Lines 11120 11120
=======================================
Hits 6855 6855
Misses 3526 3526
Partials 739 739
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant rewrite and restructuring of the documentation, adding a large volume of new content across concepts, guides, and reference sections. The effort to improve the documentation is commendable. However, there are several areas that need attention before this can be merged.
Firstly, there's a notable amount of content duplication, particularly between docs/concepts/block-lifecycle.md and docs/reference/specs/block-manager.md. This could lead to maintenance issues and should be resolved, perhaps by using includes or linking between documents.
Secondly, a large number of placeholder files have been added, containing only TODO comments. These should be either completed or removed.
Finally, there are various inconsistencies in technical details, formatting issues (like the use of :::: instead of ::: for VitePress admonitions), minor typos, and some broken links. Addressing these points will greatly improve the quality and usability of the new documentation.
- Updated Celestia guide to clarify prerequisites, installation, and configuration for connecting Evolve chains to Celestia. - Enhanced Local DA documentation with installation instructions, configuration options, and use cases for development and testing. - Expanded troubleshooting guide with detailed diagnostic commands, common issues, and solutions for node operations. - Created comprehensive upgrades guide covering minor and major upgrades, version compatibility, and rollback procedures. - Added aggregator node documentation detailing configuration, block production settings, and monitoring options. - Introduced attester node overview with configuration and use cases for low-latency applications. - Removed outdated light node documentation. - Improved formatting and clarity in ev-reth chainspec reference for better readability.
tac0turtle
requested review from
alpe,
chatton,
julienrbrt,
pthmas and
randygrok
pthmas
left a comment
pthmas
left a comment
There was a problem hiding this comment.
A lot of duplicate that we need to remove but overall looks good. I can't guarantee that the evm part is correct i don't have enough knowledge about it.
docs/concepts/block-lifecycle.md
Outdated
There was a problem hiding this comment.
Maybe a bit misleading? automatic gas price detection is set by default unless someone specifies with the da.submit_options {gas_price: xx}
docs/concepts/block-lifecycle.md
Outdated
There was a problem hiding this comment.
Is this actually implemented? I couldn't find anything in the codebase.
docs/concepts/block-lifecycle.md
Outdated
There was a problem hiding this comment.
This feels like a duplicate of the previous chapters of this same file.
| - The component architecture supports the separation of header and data structures in Evolve. This allows for expanding the sequencing scheme beyond single sequencing and enables the use of a decentralized sequencer mode. For detailed information on this architecture, see the [Header and Data Separation ADR](../../adr/adr-014-header-and-data-separation.md) | ||
| - Components process blocks with a minimal header format, which is designed to eliminate dependency on CometBFT's header format and can be used to produce an execution layer tailored header if needed. For details on this header structure, see the [Evolve Minimal Header](../../adr/adr-015-rollkit-minimal-header.md) specification | ||
|
|
||
| ## Metrics |
There was a problem hiding this comment.
Should the metric get a dedicated page?
| - **Guarantee**: Data availability sampling (DAS) | ||
| - **Latency**: ~12 seconds to finality | ||
|
|
||
| ### Custom DA |
|
|
||
| We are now ready to run our full node. If we are running the full node on the same machine as the sequencer, we need to make sure we update the ports to avoid conflicts. | ||
|
|
||
| Make sure to include these flags with your start command: |
There was a problem hiding this comment.
may we could also add that the config/ev-node file can be updated
| | **Aggregator** | Yes | Yes | Yes | Block producer (sequencer) | | ||
| | **Full Node** | No | Yes | No | RPC provider, validator | | ||
| | **Light Node** | No | Headers only | No | Mobile, embedded clients | | ||
| | **Attester** | No | Yes | No | Soft consensus participant | |
There was a problem hiding this comment.
Should specify that this is not yet available
| @@ -0,0 +1,185 @@ | |||
| # Architecture | |||
There was a problem hiding this comment.
this feels also a bit like a duplicate of the docs/concepts folder
| @@ -0,0 +1,95 @@ | |||
| # Introduction | |||
There was a problem hiding this comment.
imo the overview folder should be one page, for more information the reader should go to the concepts folder
| @@ -0,0 +1,193 @@ | |||
| # DA Interface | |||
There was a problem hiding this comment.
It's probably hard to maintain all the interfaces changes up to date and documented. Could we just point to the correct files in the repo?
3 participants
Overview
This pr restructures the docs and writes new sections in order to be better suited for users