docs: reference system-contract bytecode via versioned artifacts#340
docs: reference system-contract bytecode via versioned artifacts#340RealiCZ wants to merge 3 commits into
Conversation
…ead of inline hex
🧬 Mutation testingNo results at |
🧬 Mutation testing — ✅ PASSNothing to test — no mutants were generated on the changed lines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cd972833f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
✅ Clean
- Reviewed a docs-only change that replaces inline hex blobs on 5 system-contract spec pages with truncated prefixes plus links to the versioned artifact JSONs and a
cast keccak/jqreproduction recipe. - Verified against
crates/system-contracts/artifacts/: all 7 artifact files exist, every truncated prefix matches its artifactdeployedBytecode, and everyCode hash:in the docs equals its artifactcodeHash. - Code hash remains the normative anchor, and the version-pinned filenames (
Oracle-1.0.0.json, etc.) keep themain-branch links stable for frozen spec versions. - No new actionable findings. No prior review threads to reconcile.
flyq
left a comment
There was a problem hiding this comment.
- State the working directory for the verification command.
jq -r .deployedBytecode crates/system-contracts/artifacts/…assumes the reader runs it from a repo checkout root; the spec pages are published on docs.megaeth.com where that context is not obvious.
A short lead-in such as "From the repository root:" above the code block (once per page) would remove the ambiguity. - Consider one sentence making the hash's normative role explicit.
The PR body says "the code hash stated on each page remains the normative identity of the bytes", but no page says this in so many words.
A single sentence on the system-contracts overview page (e.g. "The code hash is the normative identity of a contract version; the linked artifact JSON is the canonical source of the full bytes.") would make the intent survive future edits.
The truncated 12-byte prefixes are cosmetic either way — all seven start with the same Solidity prologue (0x6080604052…), so they carry no discriminating value and could equally be dropped.
Host each version's deployed bytecode under docs/spec/static/bytecode/ (OP-specs convention) instead of linking implementation-repo artifact paths, and add a consistency test pinning the static files 1:1 to the versioned artifacts.
There was a problem hiding this comment.
- Summary: Re-review of the second commit that publishes each system-contract version's full deployed bytecode under
docs/spec/static/bytecode/, links it from the spec pages, addsdocs/spec/AGENTS.mdguidance, and introducesdocs_bytecode_consistencyto pin the static files 1:1 to the versioned artifacts. - The refactor cleanly addresses the earlier immutability concern (bytes now live in-spec, no
blob/mainmutation surface) and the new test makes the invariant enforceable. - Inline comments: 1 anchorable finding posted on a changed line.
- Body-only items: none.
Still open from earlier reviews: none (prior Codex thread is resolved and the previous Claude review posted a Clean verdict).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29f0c80ea2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Back to referencing versioned artifact JSONs directly; add a working-directory lead-in to the verification commands and state the code hash's normative role on the overview page.
|
@flyq
On the truncated prefixes: kept them intentionally. The format follows the OP specs convention, where the prefix is likewise the shared Solidity prologue plus a full-bytecode link: It serves as a visual cue that the value is bytecode, not as a discriminator. |
There was a problem hiding this comment.
✅ Clean
- Reviewed the re-review scope: the third commit (
9dfb3ee) reverted the static-bytecode-files approach; the PR now contains only doc-only changes acrossoverview.mdand six system-contract pages, replacing inline hex blobs with a truncated prefix, an artifact-JSON link, and acast keccak ... deployedBytecode ...reproduction command. - Verified the artifact JSON fields the new commands target (
deployedBytecode,codeHash) exist in each referenced artifact, and the code hashes stated in the docs match the artifacts. - No new actionable findings from me this round.
Resolved from earlier rounds:
- My prior [Minor] on the reverted
crates/system-contracts/tests/docs_bytecode_consistency.rsskip logic — that file no longer exists in this PR.
Still open from earlier rounds (other reviewer, unchanged status — flagging for awareness, not re-litigating):
- High-Precision Timestamp not covered by the new overview.md claim — HPT has a
HIGH_PRECISION_TIMESTAMP_ORACLE_CODE_HASHincrates/mega-evm/src/system/oracle.rsbut no artifact JSON and no code-hash entry onhigh-precision-timestamp.md, so the new sentence about the linked artifact JSON being the canonical source of the full deployed bytecode does not quite hold for that page.
Summary
crates/system-contracts/artifacts/.Deployed bytecode:entry now shows a truncated prefix, a link to the artifact JSON (deployedBytecodefield), and acast keccak/jqone-liner that re-derives the code hash; the code hash stated on each page remains the normative identity of the bytes.crates/system-contracts/build.rs, following the OP Stack specs precedent: the code hash is the normative anchor, full bytes live outside the prose page.Verification
deployedBytecodebefore deletion, and every stated code hash equals the artifact'scodeHash.main.