Conversation
WalkthroughThese changes modify storage change recording, token registry proof accumulation, and fee conversion logic. The tracer now unconditionally records storage updates instead of skipping during system calls. Storage proofs for the token registry are collected from logged storage updates and aggregated into a proof map. Fee rate calculation is simplified to use direct scale values. Changes
Sequence DiagramssequenceDiagram
participant TxTrace as Transaction<br/>Tracer
participant Logger as Storage<br/>Logger
participant Tracing as Tracing<br/>Collection
participant Registry as Token<br/>Registry Proofs
TxTrace->>Logger: OnStorageChange (now always records)
Logger->>Logger: Record UpdatedStorages<br/>(no skip check)
TxTrace->>Tracing: Finalize transaction trace
Tracing->>Logger: Read UpdatedStorages
Logger-->>Tracing: Storage updates
Tracing->>Registry: Accumulate token registry<br/>slot values into proofStorages
Registry-->>Tracing: Proof map populated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![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
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)
23-37: Add early returns after validation failures.The validation logic logs errors but continues execution, allowing invalid values (nil or zero scale, zero address, nil or zero rate) to be returned. This will cause incorrect fee calculations or runtime panics in downstream functions like
EthToAltandAltToETH.Apply this diff to fix the validation logic:
if info.Scale == nil || info.Scale.Sign() == 0 { log.Error("Invalid token scale", "tokenID", tokenID, "tokenAddr", info.TokenAddress.Hex()) + return nil, nil, errors.New("invalid token scale") } // 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 + return nil, nil, errors.New("invalid token address") } // 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 + return nil, nil, errors.New("invalid token price") }
🧹 Nitpick comments (2)
rollup/fees/rate.go (1)
43-43: Fix variable name typo.The variable name
tokenSacleshould betokenScalefor consistency and readability.Apply this diff:
- rate, tokenSacle, err := TokenRate(state, tokenID) + rate, tokenScale, err := TokenRate(state, tokenID) if err != nil { return nil, err } - return types.EthToAlt(amount, rate, tokenSacle), nil + return types.EthToAlt(amount, rate, tokenScale), nil- rate, tokenSacle, err := TokenRate(state, tokenID) + rate, tokenScale, err := TokenRate(state, tokenID) if err != nil { return nil, err } - return types.AltToEth(amount, rate, tokenSacle), nil + return types.AltToEth(amount, rate, tokenScale), nilAlso applies to: 51-51
rollup/tracing/tracing.go (1)
411-411: Consider reducing log level for production.These Info-level logs are in the transaction execution hot path and will be emitted for every AltFeeTx transaction. Consider using Debug level or removing them to avoid performance impact and log spam.
Apply this diff:
- log.Info("base slot", "base slot", baseSlot.String(), "token id", tx.FeeTokenID(), "token address", tokenInfo.TokenAddress.String()) + log.Debug("collecting token registry proofs", "baseSlot", baseSlot.String(), "tokenID", tx.FeeTokenID(), "tokenAddress", tokenInfo.TokenAddress.String())- log.Info("offset slot", "offset", offset, "slot", slot.String()) + log.Debug("collecting registry slot proof", "offset", offset, "slot", slot.String())Also applies to: 420-420
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eth/tracers/logger/logger.go(0 hunks)rollup/fees/rate.go(2 hunks)rollup/tracing/tracing.go(3 hunks)
💤 Files with no reviewable changes (1)
- eth/tracers/logger/logger.go
🧰 Additional context used
🧠 Learnings (1)
📚 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:
rollup/tracing/tracing.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 (5)
rollup/tracing/tracing.go (4)
371-371: LGTM!Capturing updated storages from the struct logger is the correct approach for building storage proofs.
405-407: LGTM!Defensive initialization of the nested map is correct and prevents potential nil map assignment panics.
468-469: LGTM!Removing the redundant reinitialization of
proofStoragesis correct. The variable is already populated fromstructLogger.UpdatedStorages()at line 371 and augmented with token registry slots, so no redeclaration is needed.
410-428: Slot calculation functions are correctly implemented and configured.All three helper functions (
GetTokenInfoStructBaseSlot,CalculateStructFieldSlot,CalculateUint16MappingSlot) exist and correctly calculate Solidity storage slots:
CalculateUint16MappingSlotproperly implements standard Solidity mapping storage with keccak256 hashingCalculateStructFieldSlotcorrectly adds field offsets to base slots for struct field access- Configuration values are properly set:
TokenRegistrySlot=151,PriceRatioSlot=153- The registrySlots offsets
[0, 1, 2, 3]correctly correspond to TokenInfo struct fields (tokenAddress, balanceSlot, isActive+decimals, scale)rollup/fees/rate.go (1)
39-39: No issues found—the change is correct.The verification confirms that
info.ScalefromGetTokenInfoFromStoragematches the value thatGetTokenScaleByIDWithStatewould return. Both functions derive from the sameGetTokenInfocall, guaranteeing the scale values are identical. The change correctly consolidates multiple state lookups into a single function call without altering functionality or return values.
2 participants
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.