Conversation
RealiCZ
added
spec:unchanged
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Good structure overall — the separation into normative specs, behavior details, implementation references, and public book pages is clean and maintainable. Three issues to fix before merging: 1. Spec progression diagram mixes hardfork and spec names (blocking)
Showing them in a diagram labeled "Spec Progression" immediately followed by "each newer spec includes all previous behaviors" is wrong — 2.
3. Hardfork→spec example omits MiniRex1
|
ReviewOverall this is a solid refactor: the new style guide is well-defined, the spec restructuring into Abstract / Changes / Invariants / Inheritance / References is a clear improvement over the old numbered-section format, and the companion Four issues worth fixing before merge: Factual error
Missing unstable caveat
Semantic ambiguity in specs
|
|
Overall this PR is well-structured and a significant improvement to the documentation. The three-tier hierarchy (normative spec → behavior details → implementation references) is clear, the STYLE-GUIDE.md is useful, and the book pages are accurate and developer-friendly. A few issues to address: |
|
This is a well-structured docs PR. The spec refactoring (normative/behavior-details/implementation-references split) is clean and the style guide are a valuable addition. A few issues worth addressing. 1. Per-frame limits section scoped only to State Growth ( The "Per-Frame Limits (Rex4+)" subsection appears only under State Growth. But Rex4 adds per-frame budgets for all four dimensions (compute gas, data size, KV updates, state growth). A developer reading only the Data Size or KV Updates sections will miss this entirely. The subsection should be elevated to a top-level section or at minimum cross-referenced from each dimension. 2. "How It Works" steps 4-5 are alternatives, not sequential steps ( The numbered list presents a process flow, but steps 4 and 5 are mutually exclusive spec variants (pre-Rex4 absolute cap vs Rex4 relative cap). Numbering them as consecutive steps implies both execute for the same transaction, which is wrong. 3. Error name mismatch ( Step 4 says "execution halts with 4. The successor (Rex2) is listed before the predecessor (Rex), which is inconsistent with other spec files. Minor: |
ReviewThis is a solid documentation PR — the three-file spec structure (normative / behavior-details / impl-references), the GitBook setup, and the upgrade pages are all well-organized and accurate overall. A few issues worth addressing before merge: 1. CLAUDE.md spec progression is now inconsistent with the book (should be fixed here) CLAUDE.md line 55 still lists MINI_REX1 and MINI_REX2 in the spec progression, but those are hardforks, not spec IDs — they map back to EQUIVALENCE and MINI_REX respectively and do not appear in the MegaSpecId enum. The new book/src/evm/spec-system.md correctly explains this distinction. CLAUDE.md progression should be updated to: EQUIVALENCE → MINI_REX → REX → REX1 → REX2 → REX3 → REX4 (with a note that MiniRex1/MiniRex2 are hardforks that reuse earlier spec IDs). Since this PR introduces the hardfork-vs-spec distinction into the docs, this is the right place to fix the agent guidance file too. 2. resource-limits.md — opening count is confusing The opening sentence says MegaETH enforces seven types of resource limits but only four are listed inline. The other three (gas limit, tx size, DA size) only appear later under Two-Phase Checking. Either list all seven upfront or drop the count. 3. gas-detention.md — step 4 wording is imprecise Step 4 says: Pre-Rex4 (absolute cap): if compute_gas_used already exceeds the cap, execution halts with VolatileDataAccessOutOfGas. The halt occurs at the moment of volatile access, not during post-access enforcement as the current phrasing implies. Suggested: At the moment of the volatile access, if compute_gas_used already exceeds the cap, execution halts immediately with VolatileDataAccessOutOfGas. Otherwise, remaining compute gas is capped to cap minus compute_gas_used. 4. Minor: evm/README.md concept-dives table headers are swapped The table columns are labeled Topic and Page but the data has the link in Topic and the description in Page. Should be Topic and Description. |
ReviewOverall the structure is solid — the normative/informative separation, STYLE-GUIDE, and impl reference files are a clear improvement over the old format. A few issues worth addressing before merge. Rex4 unstable warning missing from the main EVM reference page
Rex4 implementation references: test file links removed The previous Minor: MiniRex1/MiniRex2 rows in upgrades/README.md The upgrade history table lists MiniRex1 and MiniRex2 as plain-text rows without links or explanation of why they have no dedicated pages. A note like "(no behavior change; see Spec System for hardfork-to-spec mapping)" would prevent readers from wondering if the pages are simply missing. |
|
Overall this is a solid docs restructuring — the new spec format is cleaner, the book pages are well-organized, and the AGENTS.md correction (removing MINI_REX1/MINI_REX2 from the spec progression) is accurate. Three issues to fix below. |
|
LGTM. Spot-checked the latest commits (cbbbc51, c5e9720): MIN_BUCKET_SIZE=256 is correct (verified against |
Session 2 — Dapp builder readability (issues 1-20): - Clarify compute gas = standard Ethereum gas with opcode examples - Explain gas_limit/compute gas/storage gas relationship - Fix LOG revert behavior, EIP-7623 floor cost explanations - Restructure Oracle interface into Public/Internal/Sequencer-Only - Add SSTORE no-refund warning with transient storage recommendation - Full system contract addresses, EIP-7883 ModExp note - MiniRex1/2 expanded, timestamp lower bound, detained limit cleanup Session 3 — Cross-reference docs/ and specs/ vs book/ (gaps G1-G12): - Add all 7 resource limits in unified table with pre-execution limits - Add block building workflow with skip/reject/fail/succeed semantics - KeylessDeploy: Nick's Method background, execution flow, security considerations, tx format table, value transfer, nonce rationale - Oracle: real-time data feeding lifecycle, source links - System tx: explicit nonce handling - Dual gas model rationale hint box Session 4-5 — Final pre-deletion sweep (F1-F9): - Gas detention: full beneficiary trigger list, DELEGATECALL exception - 98/100 rule: note MiniRex limitation - KeylessDeploy: error parameters, depth-0 rationale, gas price semantics - Oracle forced-cold SLOAD rationale - Contract creation storage gas charged regardless of initcode outcome - Style rules: hint boxes for developer tips and design rationale
|
LGTM. The 085c91b commit ("comprehensive book improvements") was not covered by prior reviews — here's what's new. What changed: Four new Three minor issues in this commit:
All factual content verified correct (no new numeric errors, SELFDESTRUCT correctly says "target address", initcode size is 548,864 in the new files). |
| **Format for each entry:** | ||
|
|
||
| ```md | ||
| **Term** — Definition in 1–4 sentences. |
There was a problem hiding this comment.
The glossary template here uses bold-text format, but the actual glossary.md and the "Anchor targets must be headings" rule (added in the same file, line ~100) both require ## headings for glossary entries so that #fragment links resolve.
| **Term** — Definition in 1–4 sentences. | |
| ## Term |
Update the template to match the actual convention and the anchor rule.
| Your transaction's `gas_limit` field works the same way as on Ethereum — it sets the maximum total gas you are willing to spend. | ||
| Both compute gas and storage gas are deducted from this single `gas_limit` budget. | ||
| If total gas consumed (compute + storage) exceeds `gas_limit`, the transaction runs out of gas just like on Ethereum. | ||
|
|
There was a problem hiding this comment.
Broken link — oracle-services/README.md does not exist in this PR; the file is oracle-services/overview.md.
| **Why forced cold?** During live execution, oracle data may come from the [oracle service's external environment](../oracle-services/overview.md) rather than on-chain storage. |
|
Two items still unresolved from prior review rounds: 1. Broken link (blocking) — 2. Forward reference in Everything else from prior review rounds has been thoroughly addressed. The three-tier doc hierarchy, STYLE-GUIDE, book pages, glossary, hardfork-vs-spec distinction, Rex4 unstable markers, and all Troublor feedback items look good. |
ReviewThe PR is in good shape after many iterations. The three-tier doc hierarchy (normative spec / behavior details / impl references), GitBook setup, and book pages are consistent and accurate. All major issues from the previous human review (Rex4 unstable warning, SALT bucket explanation, glossary, gas estimation guidance, LOG revert behavior) are addressed in the current state. One minor issue remaining (inline comment posted): book/SITE_DOC_FORMAT.md — the Glossary template shows bold-text format for terms, but the actual glossary.md uses heading format, which is also required by the 'Anchor targets must be headings' rule defined in the same file. The template and the rule contradict each other. Update the template to match both the rule and the implementation. Non-doc change (bin/mega-evme/src/replay/hardforks.rs): Rex3 timestamps for testnet (1771380000) and mainnet (1771639200) match the dates documented in upgrades/overview.md. Ordering constraints are respected. Looks correct. |
|
LGTM. All issues from prior review rounds have been addressed. One minor note: |
|
LGTM. All previously raised issues have been resolved:
The |
|
|
||
| {% hint style="info" %} | ||
| **Why forced cold?** During live execution, oracle data may come from the [oracle service's external environment](../oracle-services/README.md) rather than on-chain storage. | ||
| Replayers cannot determine which source was used, and external environment reads are inherently cold. |
There was a problem hiding this comment.
Broken link: ../oracle-services/README.md does not exist in this PR — the file added is oracle-services/overview.md.
| Replayers cannot determine which source was used, and external environment reads are inherently cold. | |
| **Why forced cold?** During live execution, oracle data may come from the [oracle service's external environment](../oracle-services/overview.md) rather than on-chain storage. |
|
|
||
| ```md | ||
| **Term** — Definition in 1–4 sentences. | ||
| ``` |
There was a problem hiding this comment.
The template uses bold-text format (**Term** — ...) but the actual glossary.md and the "Anchor targets must be headings" rule (line 236) both require heading format. These are inconsistent. The template should be:
| ``` | |
| ### Term | |
| Definition in 1–4 sentences. |
Or, if the template is intentionally showing a different format, clarify that glossary terms specifically use ##/### headings.
|
Nearly ready to merge — two remaining issues. Blocking: broken link in The Oracle Forced-Cold SLOAD hint block links to Minor: The "Format for each entry" template on line 265 shows bold-text format ( Everything else from prior review rounds has been thoroughly addressed — Rex4 correctly placed in its own "Upcoming (Unstable)" section, stable upgrade history in the |
|
All previously raised issues have been addressed — spec progression diagram, alloy-evm link, Rex4 unstable caveat, SELFBALANCE qualification, net state growth, per-frame limits scope, gas detention steps, and error name consistency all look correct now. Two new items: 1. Runtime code change not mentioned in PR description ( The PR description says "Keep this PR documentation/spec-text focused, without runtime code changes" but this PR changes Rex3 from 2. The PR changes |
| /// @param slots Array of storage slots to read from | ||
| /// @return values Array of bytes32 values at corresponding slots | ||
| function getSlots(uint256[] calldata slots) | ||
| function getSlots(uint256[] memory slots) |
There was a problem hiding this comment.
These parameter locations were changed from calldata to memory, but Oracle.sol uses calldata and so does the new book/src/system-contracts/oracle.md (also in this PR). Should stay consistent:
| function getSlots(uint256[] memory slots) | |
| function getSlots(uint256[] calldata slots) |
- Rename book/ to docs/, remove src/ nesting for idiomatic GitBook structure - Delete old docs/ (internal docs) and specs/ directories - Rename Overview.md -> overview.md, SUMMARY.md -> toc.md, SITE_DOC_FORMAT.md -> CONTRIBUTING.md - Add GitBook skill.md reference to CONTRIBUTING.md - Update all cross-references in AGENTS.md, README.md, ARCH.md, upgrade pages, and source comments
| - [Project Architecture](../../../../ARCH.md) | ||
| - [Development Guide](../../../../DEV.md) | ||
| - [Main README](../../../../README.md) |
There was a problem hiding this comment.
Duplicate entry: the replacement for [Development Guide] landed on top of the existing [Main README] line, leaving two identical entries. Remove one.
| - [Project Architecture](../../../../ARCH.md) | |
| - [Development Guide](../../../../DEV.md) | |
| - [Main README](../../../../README.md) | |
| - [Main README](../../../../README.md) |
|
|
||
| This file defines conventions and rules for writing and editing the public-facing book (`docs/`), hosted via GitBook. | ||
|
|
||
| For GitBook markdown syntax, custom blocks, configuration files, and best practices, refer to the [GitBook skill.md](https://gitbook.com/docs/skill.md). |
There was a problem hiding this comment.
https://gitbook.com/docs/skill.md is not a real URL — the actual GitBook documentation is at https://docs.gitbook.com. Either fix the link or just omit the reference; the sentence adds little value since CONTRIBUTING.md already is the conventions document.
|
Two small issues in the latest commits; everything else from prior reviews looks addressed.
|
2 participants
Summary
book/, including EVM, system contracts, and network upgrade pages.specs/STYLE-GUIDE.mdto define normative/informative doc conventions and writing rules.specs/impl/(Behavior Details + Implementation References).Test plan
book/,specs/,.gitbook.yaml).book/src/SUMMARY.md, spec references).