Conversation
curryxbo
requested review from
tomatoishealthy
and removed request for
a team
WalkthroughAdds v7 and v6 stored-receipt encodings/decoders, extends stored receipt storage with four optional fee fields, updates legacy decoding tests, and adjusts token-fee validation messages plus a small token-scale validation log in fee rate retrieval. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as ReceiptForStorage.DecodeRLP
participant v7 as decodeV7StoredReceiptRLP
participant v6 as decodeV6StoredReceiptRLP
participant legacy as decodeV3/4/5
Caller->>v7: attempt decode v7 format
alt v7 success
v7-->>Caller: populate Status, CumulativeGasUsed, Logs, Bloom, L1Fee, AltFee
else v7 fail
Caller->>v6: attempt decode v6 format
alt v6 success
v6-->>Caller: populate Status, CumulativeGasUsed, Logs, Bloom, L1Fee
else v6 fail
Caller->>legacy: attempt decode v3/v4/v5 formats
legacy-->>Caller: populate legacy fields (no L1Fee/AltFee)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (2)
core/types/receipt.go (1)
340-362: Update comment to document v6 format in decoding order.The comment on lines 347-348 should be updated to mention the v6 format, as it's now part of the decoding fallback chain. The decoding logic itself is correct.
Apply this diff to update the comment:
// Try decoding from the newest format for future proofness, then the older one - // for old nodes that just upgraded. V4 was an intermediate unreleased format so - // we do need to decode it, but it's not common (try last). + // for old nodes that just upgraded. Try v6 next for compatibility with data + // written before alt fee fields were added. V4 was an intermediate unreleased + // format so we do need to decode it, but it's not common (try last).core/types/receipt_test.go (1)
211-222: LGTM - v6 encoding function correctly implemented.The encoding function correctly creates v6StoredReceiptRLP with L1Fee but without alt fee fields, matching the v6 format definition.
Note: The test receipt doesn't set L1Fee (it remains nil), so the test validates nil value round-tripping. Consider adding a test case with non-nil L1Fee to ensure it's preserved correctly, though the current coverage is acceptable.
📜 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/receipt.go(3 hunks)core/types/receipt_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/types/receipt.go (4)
core/types/log.go (2)
LogForStorage(103-103)Log(31-56)rlp/decode.go (1)
DecodeBytes(92-106)core/types/gen_receipt_json.go (2)
Receipt(18-40)Receipt(68-90)core/types/bloom9.go (1)
CreateBloom(104-116)
core/types/receipt_test.go (2)
core/types/receipt.go (1)
Receipt(52-86)core/types/log.go (1)
LogForStorage(103-103)
⏰ 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 (4)
core/types/receipt.go (2)
122-129: LGTM - v6 intermediate storage format correctly defined.The v6StoredReceiptRLP struct appropriately serves as an intermediate migration format between v5 (no L1Fee) and the current format (with L1Fee and alt fee fields). The comment clearly documents its purpose.
387-404: LGTM - v6 decoding function correctly implemented.The decoding logic correctly populates receipt fields from the v6 storage format and regenerates the Bloom filter. The alt fee fields remain nil as expected for this intermediate format, consistent with how other legacy decoders handle newer fields.
core/types/receipt_test.go (1)
120-123: LGTM - Test coverage for v6 format added.The V6StoredReceiptRLP test case appropriately extends coverage to ensure the v6 storage format encodes and decodes correctly, consistent with existing test patterns.
core/rawdb/accessors_chain.go (1)
617-626: LGTM - Storage format extended consistently.The storedReceiptRLP struct correctly adds the alt fee fields (FeeTokenID, FeeRate, TokenScale, FeeLimit) and matches the definition in core/types/receipt.go. The intentional duplication avoids import cycles while maintaining consistency between the two definitions.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/types/receipt.go(3 hunks)core/types/receipt_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/types/receipt_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/types/receipt.go (3)
core/types/log.go (2)
LogForStorage(103-103)Log(31-56)rlp/decode.go (1)
DecodeBytes(92-106)core/types/bloom9.go (1)
CreateBloom(104-116)
⏰ 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/types/receipt.go (1)
136-143: v6 format implementation looks correct.The v6 storage format and its decode function correctly handle backwards compatibility:
- v6 includes
L1Feebut excludes AltFee fields, representing a format that predates the AltFee featuredecodeV6StoredReceiptRLPproperly populates status, gas, logs, Bloom, and L1Fee- AltFee fields (
FeeTokenID,FeeRate,TokenScale,FeeLimit) remain unset (nil), which is correct for receipts stored before AltFee was introduced- Bloom is correctly recalculated via
CreateBloomThis pattern is consistent with how other legacy formats (v5, v4, v3) handle fields that didn't exist in their respective versions.
Also applies to: 427-444
| // v7StoredReceiptRLP is the storage encoding of a receipt used in database version 7. | ||
| // This version was introduced when AltFee feature was added (2024-11). | ||
| // It includes L1Fee and all AltFee fields (FeeTokenID, FeeRate, TokenScale, FeeLimit). | ||
| type v7StoredReceiptRLP struct { | ||
| PostStateOrStatus []byte | ||
| CumulativeGasUsed uint64 | ||
| Logs []*LogForStorage | ||
| L1Fee *big.Int | ||
| FeeTokenID *uint16 | ||
| FeeRate *big.Int | ||
| TokenScale *big.Int | ||
| FeeLimit *big.Int | ||
| } |
There was a problem hiding this comment.
Remove redundant v7StoredReceiptRLP or clarify its purpose.
The v7StoredReceiptRLP struct is identical to storedReceiptRLP (lines 111-120). Both have the exact same fields: PostStateOrStatus, CumulativeGasUsed, Logs, L1Fee, and all four AltFee fields (FeeTokenID, FeeRate, TokenScale, FeeLimit).
This creates unnecessary code duplication and confusion. If v7 is meant to represent the current storage format, then storedReceiptRLP should be removed in favor of v7. If storedReceiptRLP is the current format and v7 represents an older version, then v7 should have different (fewer) fields.
Consider one of these approaches:
- If v7 is the current format, remove
storedReceiptRLPand use v7 throughout - If storedReceiptRLP is current and v7 is historical, ensure v7 reflects the actual historical schema (likely without all AltFee fields)
- If they're intentionally identical, consolidate them into a single type
🤖 Prompt for AI Agents
In core/types/receipt.go around lines 122-134 there is a redundant type
v7StoredReceiptRLP that is identical to storedReceiptRLP; remove the duplication
by consolidating to a single canonical type and update all references and
serialization/deserialization code to use that single type (or, if v7 was
intended to be a historical schema, change v7StoredReceiptRLP to match the
actual historical fields and update migration/decoding logic accordingly);
ensure imports, type assertions, RLP encoding/decoding switch cases, and unit
tests are updated to reference the chosen type so compilation and
backward-compatibility paths remain correct.
| if err := decodeV7StoredReceiptRLP(r, blob); err == nil { | ||
| return nil | ||
| } | ||
| if err := decodeV6StoredReceiptRLP(r, blob); err == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The v7 decode path is unreachable.
Since storedReceiptRLP and v7StoredReceiptRLP are identical (same fields in same order), and decodeStoredReceiptRLP is attempted first (line 363), the call to decodeV7StoredReceiptRLP on line 366 will never succeed. Any RLP blob that would decode as v7 will have already decoded successfully as storedReceiptRLP.
This creates unreachable code that will never execute in production.
After resolving the struct redundancy issue, ensure the decode order reflects the actual format evolution. The newest format should be tried first, followed by progressively older formats (v6, v5, v4, v3).
🤖 Prompt for AI Agents
In core/types/receipt.go around lines 366 to 371, the v7 decode path is
unreachable because decodeStoredReceiptRLP (which uses the same struct as v7) is
attempted first; fix by ensuring the decode order tries the newest format first
(attempt decodeV7StoredReceiptRLP before decodeStoredReceiptRLP) and/or remove
the redundant identical struct definitions so that v7 is distinct; reorder the
decode attempts to try v7, then v6, v5, v4, v3 (newest-to-oldest) so newer
formats are matched first.
| func decodeV7StoredReceiptRLP(r *ReceiptForStorage, blob []byte) error { | ||
| var stored v7StoredReceiptRLP | ||
| if err := rlp.DecodeBytes(blob, &stored); err != nil { | ||
| return err | ||
| } | ||
| if err := (*Receipt)(r).setStatus(stored.PostStateOrStatus); err != nil { | ||
| return err | ||
| } | ||
| r.CumulativeGasUsed = stored.CumulativeGasUsed | ||
| r.Logs = make([]*Log, len(stored.Logs)) | ||
| for i, log := range stored.Logs { | ||
| r.Logs[i] = (*Log)(log) | ||
| } | ||
| r.Bloom = CreateBloom(Receipts{(*Receipt)(r)}) | ||
| r.L1Fee = stored.L1Fee | ||
| r.FeeTokenID = stored.FeeTokenID | ||
| r.FeeRate = stored.FeeRate | ||
| r.TokenScale = stored.TokenScale | ||
| r.FeeLimit = stored.FeeLimit | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Duplicated decode logic.
The decodeV7StoredReceiptRLP function is nearly identical to decodeStoredReceiptRLP (lines 381-402). Both:
- Decode the same fields in the same order
- Set status via
setStatus - Convert
LogForStoragetoLog - Recalculate Bloom via
CreateBloom - Populate L1Fee and all AltFee fields
This duplication increases maintenance burden. If the structs are truly identical (as they currently are), only one decode function is needed.
curryxbo
force-pushed
the
fixReceiptRlp
branch
from
3c544e2 to
5be7250
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rollup/fees/rate.go (1)
24-33: TokenRate returns nil error for invalid address/price and still panics on invalid scale.On Line 24–33, when
info.TokenAddressis zero orrateis nil/zero, you log an error but return(nil, nil, err)whereerris still nil fromGetTokenInfoFromStorage. Callers seeerr == niland proceed;EthToAlt/AltToETHthen panic intypes.EthToAlt/types.AltToEthdue to nil/zero rate. Similarly, the new invalid-scale log on Line 41–43 does not return an error, so invalid-scale cases also rely on a downstream panic.This makes TokenRate’s contract misleading and pushes configuration/price issues into panics instead of explicit errors.
Consider tightening TokenRate’s contract so all three invalid cases (address, price, scale) return a non‑nil error and never rely on panics:
func TokenRate(state StateDB, tokenID uint16) (*big.Int, *big.Int, error) { @@ - info, rate, err := GetTokenInfoFromStorage(state, TokenRegistryAddress, tokenID) + info, rate, err := GetTokenInfoFromStorage(state, TokenRegistryAddress, tokenID) if err != nil { log.Error("Failed to get token info from storage", "tokenID", tokenID, "error", err) return nil, nil, err } // If token address is zero, this is not a valid token if info.TokenAddress == (common.Address{}) { - log.Error("Invalid token address", "tokenID", tokenID) - return nil, nil, err + err = errors.New("invalid token address") + log.Error("Invalid token address", "tokenID", tokenID, "error", err) + return nil, nil, err } // If price is nil or zero, this token doesn't have a valid price if rate == nil || rate.Sign() == 0 { - log.Error("Invalid token price", "tokenID", tokenID, "tokenAddr", info.TokenAddress.Hex()) - return nil, nil, err + err = errors.New("invalid token price") + log.Error("Invalid token price", "tokenID", tokenID, "tokenAddr", info.TokenAddress.Hex(), "error", err) + return nil, nil, err } @@ // Get scale from token info scale, err := GetTokenScaleByIDWithState(state, tokenID) if err != nil { log.Error("Failed to get token scale", "tokenID", tokenID, "error", err) return nil, nil, err } - if scale == nil || scale.Sign() == 0 { - log.Error("Invalid token scale", "tokenID", tokenID, "tokenAddr", info.TokenAddress.Hex()) - } - - return rate, scale, err + if scale == nil || scale.Sign() == 0 { + err = errors.New("invalid token scale") + log.Error("Invalid token scale", "tokenID", tokenID, "tokenAddr", info.TokenAddress.Hex(), "error", err) + return nil, nil, err + } + + return rate, scale, nil }With this, EthToAlt/AltToETH will consistently surface misconfigurations as regular errors instead of panics, and callers of TokenRate alone also get a clear error signal.
Also applies to: 41-43
♻️ Duplicate comments (2)
core/types/receipt.go (2)
111-120: v7/v6 structs: versioning clarified but v7 remains a duplicate of storedReceiptRLP.
storedReceiptRLPandv7StoredReceiptRLPcurrently have identical fields; the new comments help explain that v7 is the DB‑versioned schema, but as long as the layouts are the same, v7 is redundant and risks divergence later.Consider one of:
- Treat
storedReceiptRLPas the current version (e.g. v8) and change its comment/name accordingly, making it clear that v7 is historical.- Or, drop
v7StoredReceiptRLPfor now and re‑introduce it only oncestoredReceiptRLPactually diverges (e.g. on the next schema change).- If you prefer to keep both for future‑proofing, add an explicit note that they are intentionally identical today and that
storedReceiptRLPis intended to evolve whilev7StoredReceiptRLPis frozen to the 2024‑11 schema.Also applies to: 122-144
363-379: decodeV7 path is effectively dead today and decode logic is duplicated across versions.
ReceiptForStorage.DecodeRLPfirst callsdecodeStoredReceiptRLP, thendecodeV7StoredReceiptRLP, thendecodeV6StoredReceiptRLP. BecausestoredReceiptRLPandv7StoredReceiptRLPhave identical layouts, any blob that would decode as v7 will already decode successfully viadecodeStoredReceiptRLP, sodecodeV7StoredReceiptRLPis currently unreachable. The body ofdecodeV7StoredReceiptRLPalso duplicatesdecodeStoredReceiptRLPalmost line‑for‑line, anddecodeV6StoredReceiptRLPduplicates most of that logic again.To reduce confusion and maintenance overhead, consider:
- Either removing
decodeV7StoredReceiptRLPuntilstoredReceiptRLPdiverges, or- Consolidating the shared “apply decoded stored receipt to ReceiptForStorage” logic into a helper used by
decodeStoredReceiptRLP,decodeV7StoredReceiptRLP, anddecodeV6StoredReceiptRLP. For example, a helper that takesPostStateOrStatus,CumulativeGasUsed,Logs, and optional fee fields and fillsrplus recomputesBloom.This keeps the versioned decode paths explicit while avoiding three copies of the same assignment sequence.
Also applies to: 381-402, 404-425, 427-444
🧹 Nitpick comments (1)
rollup/fees/rate.go (1)
48-54: Fix typo intokenSaclevariable name.
tokenSacleis misspelled in both EthToAlt and AltToETH; renaming it totokenScaleimproves readability and reduces future confusion.func EthToAlt(state StateDB, tokenID uint16, amount *big.Int) (*big.Int, error) { - rate, tokenSacle, err := TokenRate(state, tokenID) + rate, tokenScale, err := TokenRate(state, tokenID) @@ - return types.EthToAlt(amount, rate, tokenSacle), nil + return types.EthToAlt(amount, rate, tokenScale), nil } func AltToETH(state StateDB, tokenID uint16, amount *big.Int) (*big.Int, error) { - rate, tokenSacle, err := TokenRate(state, tokenID) + rate, tokenScale, err := TokenRate(state, tokenID) @@ - return types.AltToEth(amount, rate, tokenSacle), nil + return types.AltToEth(amount, rate, tokenScale), nil }Also applies to: 56-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/types/receipt.go(3 hunks)core/types/receipt_test.go(2 hunks)core/types/token_fee.go(2 hunks)rollup/fees/rate.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/types/receipt_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/types/receipt.go (3)
core/types/log.go (2)
LogForStorage(103-103)Log(31-56)rlp/decode.go (1)
DecodeBytes(92-106)core/types/bloom9.go (1)
CreateBloom(104-116)
⏰ 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/types/token_fee.go (1)
50-55: Panic messages now correctly match the validated parameter (LGTM).The panics for rate vs. tokenScale are now labeled correctly, with no behavioral change to the validation logic.
Also applies to: 67-72
2 participants
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