Fix err return when token info invalid

[Kukoomomo]

Summary by CodeRabbit

• Bug Fixes

  • Safer handling of token/fee conversions — invalid inputs now produce explicit errors instead of crashes, improving reliability.
  • Gas and token fee flows now propagate and log errors more consistently to avoid silent failures.

• Chores

  • Internal API behavior adjusted to return errors from conversion operations for more robust upstream handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Removed/relocated token validation and changed conversion functions to return errors: token rate lookup no longer enforces scale/price checks; EthToAlt/AltToEth now return (*big.Int, error); callers updated to propagate or log errors; some validation checks were removed from preCheck/refund flows.

Changes

Cohort / File(s)	Summary
Rollup fee rate lookup rollup/fees/rate.go	Removed explicit validation paths for token scale/price (no longer returns/logs on invalid scale/price); replaced one error return with explicit "invalid token address"; success path returns rate/scale without propagating previous validation errors.
Token fee conversions core/types/token_fee.go	Changed signatures: EthToAlt and AltToEth now return (*big.Int, error) instead of *big.Int; replaced panics on invalid inputs with error returns; added errors import.
State transition callers core/state_transition.go	Updated callers to handle new (value, error) returns from EthToAlt: buyAltTokenGas now propagates EthToAlt errors; preCheck removed explicit nil/positivity checks for feeRate/tokenScale; refundGas logs EthToAlt errors and continues using returned tokenAmount.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Review focus areas:
  • All call sites of EthToAlt / AltToEth to ensure error handling is correct and consistent.
  • Behavior changes in rollup/fees/rate.go where validations were removed — verify safety assumptions and upstream validations.
  • Spot the tokenSacle typo/variable name inconsistency in callers.
  • Ensure logging vs. error propagation choices in refundGas/buyAltTokenGas match intended semantics.

Possibly related PRs

• Update balance slot decode #244 — Overlapping changes to buyAltTokenGas and refundGas (error/return handling and token transfer calls).

Suggested reviewers

• Web3Jumb0
• FletcherMan

Poem

🐰 I nibble at the diff with glee,
Errors turned polite, now error-free (or queued),
Conversions speak in answers and returns,
I hop, approve, then do a joyful twirl 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title directly addresses the main objective of the changeset - improving error return handling when token information is invalid, which is reflected across all modified files.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch altFee_rate_err

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13b8ea0 and 210a32f.

📒 Files selected for processing (3)

• core/state_transition.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)

• rollup/fees/rate.go

🧰 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/token_fee.go
• core/state_transition.go

🧬 Code graph analysis (2)

core/types/token_fee.go (2)

rollup/fees/rate.go (1)

• EthToAlt (30-36)

accounts/abi/unpack_test.go (1)

• Int (547-547)

core/state_transition.go (2)

core/types/token_fee.go (1)

• EthToAlt (52-66)

rollup/fees/rate.go (1)

• EthToAlt (30-36)

⏰ 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)

core/state_transition.go (2)

315-318: LGTM! Proper error handling for EthToAlt.

The error from EthToAlt is correctly captured and propagated, preventing transactions from proceeding with invalid token conversion rates.

---

683-686: The original review comment flags a nil pointer issue that does not exist. TransferAltTokenHybrid explicitly handles nil amounts on line 49 with if amount == nil || amount.Cmp(big.NewInt(0)) == 0 { return nil }. When EthToAlt returns an error and tokenAmount becomes nil, the subsequent call to TransferAltTokenHybrid gracefully returns without performing any transfer—this is intentional design that allows the refund to degrade safely when the exchange rate conversion fails.

Likely an incorrect or invalid review comment.

core/types/token_fee.go (3)

3-6: LGTM! Import added for error handling.

The errors package import is necessary to support the new error-returning signatures.

---

52-66: LGTM! Improved error handling over panics.

The function now returns errors instead of panicking on invalid inputs, making it safer and allowing callers to handle failures gracefully. The conversion logic with rounding up on remainder remains correct.

---

69-83: LGTM! Consistent error handling.

The changes mirror those in EthToAlt, providing consistent error handling across both conversion functions. The validation and conversion logic remain correct.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

rollup/fees/rate.go (1)

35-38: Good improvement to error clarity.

The explicit error message clearly indicates the validation failure. All three validation errors in this function now have clear, descriptive messages.

Optional enhancement: Consider defining package-level error variables for these common validation errors to enable structured error checking with errors.Is():

var (
    ErrInvalidTokenScale   = errors.New("invalid token scale")
    ErrInvalidTokenAddress = errors.New("invalid token address")
    ErrInvalidTokenPrice   = errors.New("invalid token price")
)

Then return these predefined errors instead of creating new ones inline. This allows callers to check for specific error types.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49fa27b and 13b8ea0.

📒 Files selected for processing (1)

• rollup/fees/rate.go (1 hunks)

⏰ 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 (3)

rollup/fees/rate.go (3)

23-26: Good improvement to error clarity.

The explicit error message clearly indicates the validation failure. The error follows Go conventions (lowercase, no punctuation) and matches the logged information.

---

29-32: Good improvement to error clarity.

The explicit error message clearly indicates the validation failure and is consistent with the other validation errors in this function.

---

40-40: Explicit nil return improves clarity.

Returning nil explicitly for the success case is clearer than returning the err variable, even though err would be nil at this point. This makes the success path immediately obvious to readers.