Conversation
WalkthroughWidened the FeeTokenID field from *uint16 to *uint64 across storage, in-memory types, and JSON (de)serialization; also renamed several fee-related JSON tags from snake_case to camelCase. No control-flow or error-handling changes were introduced. Changes
Sequence Diagram(s)(omitted — changes are type/serialization and do not alter control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/rawdb/accessors_chain.go(1 hunks)core/types/gen_receipt_json.go(4 hunks)core/types/receipt.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
📚 Learning: 2025-11-10T06:56:24.937Z
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
Applied to files:
core/rawdb/accessors_chain.gocore/types/receipt.gocore/types/gen_receipt_json.go
🧬 Code graph analysis (2)
core/types/receipt.go (1)
common/hexutil/json.go (2)
Uint64(275-275)Big(155-155)
core/types/gen_receipt_json.go (1)
common/hexutil/json.go (3)
Uint64(275-275)Big(155-155)Uint(339-339)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
core/types/receipt.go (2)
120-124: Storage encoding updated consistently.The
storedReceiptRLPstruct correctly reflects the FeeTokenID type change to*uint64. The encoding/decoding logic inEncodeRLP(lines 339-354) and decode functions (lines 385-406) properly handle the updated type.
134-138: v7StoredReceiptRLP updated consistently.The
v7StoredReceiptRLPstruct and its decoder (decodeV7StoredReceiptRLP, lines 408-429) correctly handle the FeeTokenID type change to*uint64.core/types/gen_receipt_json.go (3)
36-36: JSON marshaling correctly updated for FeeTokenID.The generated code properly handles marshaling of
FeeTokenIDfrom*uint64to*hexutil.Uint64for JSON encoding. The pointer cast at line 59 is consistent with the pattern used for other hexutil types throughout this generated file.Also applies to: 59-59
86-86: JSON unmarshaling correctly updated for FeeTokenID.The generated code properly handles unmarshaling of
FeeTokenIDfrom*hexutil.Uint64back to*uint64. The pointer cast at line 152 follows the same pattern as other hexutil type conversions in this file.Also applies to: 152-152
1-1: Auto-generated file correctly regenerated.Verification confirms that
gen_receipt_json.gowas properly regenerated using thegencodecgenerator. The go:generate directive atcore/types/receipt.go:34is correctly configured and in place.core/rawdb/accessors_chain.go (1)
622-622: The review comment premise is factually incorrect, but the underlying backward compatibility concern is valid for a different reason.The git diff shows that
FeeTokenID,FeeRate,TokenScale, andFeeLimitare new fields being added tostoredReceiptRLP, not a type change from*uint16to*uint64. The field did not previously exist in the storage struct.However, the core concern remains valid: adding new fields to a serialized storage format without introducing a new version constant (like
v8StoredReceiptRLP) may cause issues. Old receipts lacking these fields could fail to decode if the decoder doesn't account for them gracefully.Additionally, duplicate
storedReceiptRLPstruct definitions exist in bothcore/types/receipt.goandcore/rawdb/accessors_chain.go, which could lead to versioning mismatches if they diverge.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/types/l2trace.go (1)
60-62: Inconsistent JSON tag naming convention.The
ExecutionResultstruct has mixed JSON tag naming:fee_token_idandtoken_scaleuse snake_case (lines 60, 62), whilefeeRateuses camelCase (line 61). For consistency and to align with the PR's goal of standardizing on camelCase, consider updating the snake_case tags.Apply this diff to standardize on camelCase:
- FeeTokenID *uint64 `json:"fee_token_id,omitempty"` + FeeTokenID *uint64 `json:"feeTokenID,omitempty"` FeeRate *hexutil.Big `json:"feeRate,omitempty"` - TokenScale *hexutil.Big `json:"token_scale,omitempty"` + TokenScale *hexutil.Big `json:"tokenScale,omitempty"`Note: This is a breaking change for API consumers. Ensure this aligns with your versioning and compatibility requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/state_processor.go(1 hunks)core/types/l2trace.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
📚 Learning: 2025-11-10T06:56:24.937Z
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
Applied to files:
core/state_processor.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
core/state_processor.go (1)
196-197: LGTM: Explicit cast correctly handles type widening.The explicit cast to
uint64on line 196 appropriately handles the widening ofFeeTokenIDfromuint16touint64, aligning with the updatedReceipt.FeeTokenIDtype. The widening conversion is safe and prevents any data loss.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ethapi/transaction_args.go (1)
309-312: Add bounds validation: FeeTokenID narrowed from uint64 to uint16 without checking maximum value.The API accepts
FeeTokenIDas a*hexutil.Uint64(line 54), but it's cast touint16at lines 311 and 399 without bounds checking. Values exceeding 65535 will silently truncate to the lower 16 bits, potentially causing unexpected behavior or transaction routing to wrong fee tokens.Fix by validating the value before narrowing:
var feeTokenID uint16 if args.FeeTokenID != nil { + if *args.FeeTokenID > math.MaxUint16 { + return types.Message{}, fmt.Errorf("feeTokenID exceeds maximum value of %d", math.MaxUint16) + } feeTokenID = uint16(*args.FeeTokenID) }Apply the same validation at line 399 where
AltFeeTxis constructed.
♻️ Duplicate comments (1)
internal/ethapi/transaction_args.go (1)
399-399: Same truncation issue when constructing AltFeeTx.Similar to the issue at lines 309-312, this line narrows
*hexutil.Uint64touint16without validation. Apply the same fix here to ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/types/l2trace.go(3 hunks)internal/ethapi/transaction_args.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
📚 Learning: 2025-11-10T06:56:24.937Z
Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.
Applied to files:
core/types/l2trace.go
🧬 Code graph analysis (1)
internal/ethapi/transaction_args.go (1)
common/hexutil/json.go (2)
Uint64(275-275)Big(155-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
core/types/l2trace.go (2)
141-141: Type consistency restored.The
TransactionData.FeeTokenIDfield is now consistently*uint64, matchingExecutionResult.FeeTokenID(line 60). This addresses the inconsistency flagged in the previous review.
199-203: Ignore FeeTokenID widening concern. The coreTransaction,Message, andAltFeeTxtypes intentionally useuint16forFeeTokenID(core/types/transaction.go 364, alt_fee_tx.go 38), with JSON serializers usinguint64externally. The cast in l2trace.go is safe and by design.
1 participant
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.