fix(payload): serialize nextL1MessageIndex as bare uint64, not hex string#73
fix(payload): serialize nextL1MessageIndex as bare uint64, not hex string#73
Conversation
…ring morph-geth's gen_l2_ed.go declares NextL1MessageIndex as bare `uint64` (not `hexutil.Uint64`), so it expects a JSON number like `10`, not a hex quantity string like `"0xa"`. Remove the `#[serde(with = "alloy_serde::quantity")]` attribute to match the Go side. All other numeric fields (number, gasLimit, etc.) correctly use hex because morph-geth uses `hexutil.Uint64` for those — NextL1MessageIndex is the sole exception.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoved hex-quantity serde for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/payload/types/src/executable_l2_data.rs (2)
68-72: Avoid hardcoded external line numbers in docs.
gen_l2_ed.go line 31can drift quickly. Prefer symbol/path-only reference or a permalink to a specific commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/types/src/executable_l2_data.rs` around lines 68 - 72, The doc comment references an external file by a hardcoded line number ("gen_l2_ed.go line 31") which can drift; update the comment on the next_l1_message_index field to remove the line number and instead point to a stable identifier — e.g., reference the source file and symbol or a commit permalink (for example "morph/go-ethereum/eth/catalyst/gen_l2_ed.go, see the GenL2ED function/serialization logic" or a specific commit URL) so readers can locate the exact code without relying on a line number.
191-253: Add explicit test to verifynextL1MessageIndexserializes as a JSON number.The code comment (lines 68-71) emphasizes that
nextL1MessageIndexmust serialize as a bare JSON number, unlike other numeric fields. Current tests verify deserialization and roundtrip equality but do not explicitly assert the serialization format usingis_number(). The proposed test would protect against accidental changes that could introduce hex serialization.Proposed test addition
+#[test] +fn test_next_l1_message_index_serializes_as_number() { + let data = ExecutableL2Data { + next_l1_message_index: 10, + ..Default::default() + }; + + let json = serde_json::to_value(&data).expect("serialize"); + assert_eq!(json["nextL1MessageIndex"], serde_json::json!(10)); + assert!(json["nextL1MessageIndex"].is_number()); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/types/src/executable_l2_data.rs` around lines 191 - 253, Add a unit test that builds an ExecutableL2Data with a known next_l1_message_index value, serializes it with serde_json::to_value or to_string, parses to serde_json::Value and asserts the nextL1MessageIndex field exists and value.is_number() is true; place the test near the other serde tests (e.g., name it test_next_l1_message_index_serializes_as_number) and reference ExecutableL2Data and the JSON key "nextL1MessageIndex" to ensure the field is not emitted as a hex/string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/payload/types/src/executable_l2_data.rs`:
- Around line 68-72: The doc comment references an external file by a hardcoded
line number ("gen_l2_ed.go line 31") which can drift; update the comment on the
next_l1_message_index field to remove the line number and instead point to a
stable identifier — e.g., reference the source file and symbol or a commit
permalink (for example "morph/go-ethereum/eth/catalyst/gen_l2_ed.go, see the
GenL2ED function/serialization logic" or a specific commit URL) so readers can
locate the exact code without relying on a line number.
- Around line 191-253: Add a unit test that builds an ExecutableL2Data with a
known next_l1_message_index value, serializes it with serde_json::to_value or
to_string, parses to serde_json::Value and asserts the nextL1MessageIndex field
exists and value.is_number() is true; place the test near the other serde tests
(e.g., name it test_next_l1_message_index_serializes_as_number) and reference
ExecutableL2Data and the JSON key "nextL1MessageIndex" to ensure the field is
not emitted as a hex/string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84a4d90f-7521-466f-9280-d9c3830c4f58
📒 Files selected for processing (1)
crates/payload/types/src/executable_l2_data.rs
chengwenxi
left a comment
There was a problem hiding this comment.
LGTM. Correct fix — matches Go's gen_l2_ed.go where NextL1MessageIndex is plain uint64 (not hexutil.Uint64). Removing alloy_serde::quantity switches serde to bare JSON number, fixing the interop bug. Minor nit: the hardcoded line number in the doc comment (line 31) is brittle — consider a permalink or just the symbol name.
Summary
ExecutableL2Data.nextL1MessageIndexwas decorated with#[serde(with = "alloy_serde::quantity")], causing it to serialize as a hex string ("0xa")gen_l2_ed.godeclaresNextL1MessageIndexas bareuint64(nothexutil.Uint64), so it expects a plain JSON number (10)json: cannot unmarshal string into Go struct field ExecutableL2Data.nextL1MessageIndex of type uint64Fix: Remove the
quantityserde attribute. All other numeric fields (number,gasLimit,gasUsed,timestamp) correctly use hex encoding because morph-geth useshexutil.Uint64for those —NextL1MessageIndexis the sole exception.Reference:
morph/go-ethereum/eth/catalyst/gen_l2_ed.goline 31Test plan
"nextL1MessageIndex": "0xa"→"nextL1MessageIndex": 10cargo test -p morph-payload-typespassesSummary by CodeRabbit
Bug Fixes
Documentation
Tests