Skip to content

WIP:Fix #260

Closed
curryxbo wants to merge 1 commit intomainfrom
fix-altFee
Closed

WIP:Fix #260
curryxbo wants to merge 1 commit intomainfrom
fix-altFee

Conversation

@curryxbo
Copy link
Copy Markdown
Contributor

@curryxbo curryxbo commented Dec 15, 2025

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:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

Summary by CodeRabbit

  • Bug Fixes

    • Gas estimation now properly incorporates fee limits when calculating transaction costs for improved accuracy.
    • Corrected variable naming inconsistencies in fee rate calculations.
  • Documentation

    • Updated code comments for improved clarity.

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

@curryxbo curryxbo requested a review from a team as a code owner December 15, 2025 08:41
@curryxbo curryxbo requested review from r3aker86 and removed request for a team December 15, 2025 08:41
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 15, 2025

Walkthrough

This pull request contains four localized changes: removal of a nil-check in transaction list cost tracking, propagation of FeeLimit in gas estimation requests, a variable name typo fix in fee rate calculation, and a comment update in receipt storage.

Changes

Cohort / File(s) Summary
Transaction cost tracking
core/tx_list.go
Removed nil-check on l.costcap.Eth() in the Add method's Eth cost update conditional, changing from l.costcap.Eth() == nil || l.costcap.Eth() < ethCost to l.costcap.Eth() < ethCost.
Transaction estimation and arguments
internal/ethapi/transaction_args.go
Updated gas-estimation path in setDefaults to propagate FeeLimit from original args into the temporary callArgs used for DoEstimateGas, aligning estimation parameters with actual transaction fields.
Fee rate calculation
rollup/fees/rate.go
Corrected variable name typo from tokenSacle to tokenScale at two TokenRate call sites and updated related type conversion function usage.
Receipt metadata
core/types/receipt.go
Removed parenthetical "2024-11" reference from the v7StoredReceiptRLP comment string.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • core/tx_list.go requires attention due to removal of nil-check on l.costcap.Eth(), which could introduce a panic if the value is nil—verify the nil-safety assumptions in all call paths.
  • internal/ethapi/transaction_args.go change is localized to gas-estimation logic but should be verified for consistency with actual transaction execution.

Possibly related PRs

  • Implement alt token fee #205: Directly related—both modify the tx_list.Add logic in core/tx_list.go, with this PR removing the nil-check while the related PR overhauls cost tracking with per-token costcap.
  • Fix receipt rlp #247: Related through fee-related code paths—both modify FeeLimit propagation in transaction_args and token scale handling in rollup fees.
  • Update trace #245: Related through FeeLimit handling—this PR propagates FeeLimit into gas-estimation, while the related PR adds FeeLimit field to TransactionData for tracing.

Suggested reviewers

  • r3aker86
  • FletcherMan

Poem

🐰 A rabbit hops through the code with glee,
Fixing typos and fees, clean as can be,
FeeLimit now flows where it ought,
Nil-checks and comments—all battles fought!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'WIP:Fix' is vague and does not clearly convey what was fixed; it lacks specificity about the actual changes across multiple files. Revise the title to be more specific and follow Conventional Commits format (e.g., 'fix: correct variable naming, nil-check handling, and gas estimation parameters').
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ 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 fix-altFee

📜 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 49fa27b and e6d20fc.

📒 Files selected for processing (4)
  • core/tx_list.go (1 hunks)
  • core/types/receipt.go (1 hunks)
  • internal/ethapi/transaction_args.go (1 hunks)
  • rollup/fees/rate.go (1 hunks)
🧰 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:

  • internal/ethapi/transaction_args.go
  • core/tx_list.go
🧬 Code graph analysis (1)
rollup/fees/rate.go (1)
core/types/token_fee.go (2)
  • EthToAlt (49-63)
  • AltToEth (66-80)
⏰ 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 (1)

127-127: LGTM: Minor documentation update.

The removal of the date reference from the comment is a reasonable cleanup.

rollup/fees/rate.go (1)

43-55: LGTM: Typo correction.

The variable name has been corrected from tokenSacle to tokenScale, fixing the typo.

internal/ethapi/transaction_args.go (1)

173-173: LGTM: Correct propagation of FeeLimit for gas estimation.

The FeeLimit field is now properly propagated to the gas estimation call, ensuring that AltFee transaction constraints are respected during estimation.

core/tx_list.go (1)

359-363: No nil pointer dereference risk exists.

The SuperAccount.Eth() method (core/types/token_fee.go, lines 19-24) is defensively implemented with a nil check that returns new(big.Int) if the internal ethAmount is nil, ensuring the method never returns nil. The call to .Cmp() on line 360 is safe.

Likely an incorrect or invalid review comment.


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

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

@curryxbo curryxbo closed this Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant