Skip to content

Update balance slot decode#244

Merged
Kukoomomo merged 2 commits intomainfrom
slot_add
Nov 17, 2025
Merged

Update balance slot decode#244
Kukoomomo merged 2 commits intomainfrom
slot_add

Conversation

@Kukoomomo
Copy link
Copy Markdown
Contributor

@Kukoomomo Kukoomomo commented Nov 17, 2025

Summary by CodeRabbit

  • Refactor
    • Improved internal token balance handling and transfer logic for alternate tokens, increasing consistency and reliability.
  • Bug Fixes
    • More consistent error handling during alternate-token refunds and purchases to reduce rare failure cases.

@Kukoomomo Kukoomomo requested a review from a team as a code owner November 17, 2025 04:54
@Kukoomomo Kukoomomo requested review from Web3Jumb0 and removed request for a team November 17, 2025 04:54
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 17, 2025

Walkthrough

Refactors alt-token balance-slot handling by adding HasSlot to TokenInfo, updating transfer/balance functions to accept *fees.TokenInfo (removing separate balanceSlot parameters), and normalizing error assignment in state transition refund/buy flows. No public APIs otherwise added.

Changes

Cohort / File(s) Summary
Token metadata update
rollup/fees/token_info.go
Adds HasSlot bool to TokenInfo and sets it in getTokenInfo after detecting/adjusting stored balance-slot presence.
Alt-token transfer & balance logic
core/token_gas.go
Changes TransferAltTokenHybrid signature to accept tokenInfo *fees.TokenInfo; branches now check tokenInfo.HasSlot instead of comparing a raw slot hash; passes tokenInfo.TokenAddress/tokenInfo.BalanceSlot to state-path helper when needed. GetAltTokenBalanceHybrid updated to use HasSlot as well.
State transition call-sites / error handling
core/state_transition.go
Updates calls to TransferAltTokenHybrid to pass tokenInfo (removing separate slot arg) in buyAltTokenGas and refundGas; replaces short-declaration error assignments with reassignments to an existing err variable.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as state_transition / token_gas callers
  participant TokenInfo as fees.TokenInfo
  participant Transfer as TransferAltTokenHybrid
  participant State as fees.TransferAltTokenByState
  participant EVM as fees.TransferAltTokenByEVM

  Note over Caller,TokenInfo: Caller passes tokenInfo to TransferAltTokenHybrid

  Caller->>Transfer: TransferAltTokenHybrid(tokenInfo, from, to, amount, userBalanceBefore)
  alt tokenInfo.HasSlot == true
    Transfer->>State: fees.TransferAltTokenByState(tokenInfo.TokenAddress, tokenInfo.BalanceSlot, ...)
    State-->>Transfer: result / err
  else tokenInfo.HasSlot == false
    Transfer->>EVM: fees.TransferAltTokenByEVM(tokenInfo.TokenAddress, ...)
    EVM-->>Transfer: result / err
  end
  Transfer-->>Caller: returns error / success
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to correctness of HasSlot detection and the adjusted decoding/offset logic in getTokenInfo.
  • Verify all call-sites updated to the new TransferAltTokenHybrid signature and that no shadowed err variables remain.
  • Confirm EVM vs state-path parameter mapping uses the right fields (TokenAddress, BalanceSlot).

Poem

🐰 I hopped through tokens, slots now in sight,
A flag whispers whether balances take flight.
Calls pass the info, no slots left alone,
Error names settled — tidy and grown. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Update balance slot decode' is specific and directly related to the main changes in the PR, which involve refactoring how balance slots are decoded and handled through the new TokenInfo.HasSlot field and updated parameter passing.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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 slot_add

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
The command is terminated due to an 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.

❤️ Share

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

curryxbo
curryxbo previously approved these changes Nov 17, 2025
Comment thread rollup/fees/token_info.go Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
core/token_gas.go (1)

66-73: Update to use HasSlot for consistency.

The condition still uses the old pattern of comparing info.BalanceSlot.Bytes() to the zero hash. For consistency with GetAltTokenBalanceHybrid (line 31) and the new HasSlot field, refactor this to use info.HasSlot.

Apply this diff:

-	if !bytes.Equal(info.BalanceSlot.Bytes(), common.Hash{}.Bytes()) {
-		// balance slot exist
+	if info.HasSlot {
 		balance, _, err = fees.GetAltTokenBalanceFromSlot(evm.StateDB, info.TokenAddress, user, info.BalanceSlot)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e83cc5 and 76cb87e.

📒 Files selected for processing (3)
  • core/state_transition.go (2 hunks)
  • core/token_gas.go (2 hunks)
  • rollup/fees/token_info.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/state_transition.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/token_gas.go
🧬 Code graph analysis (2)
core/token_gas.go (3)
core/state_transition.go (1)
  • StateTransition (65-82)
rollup/fees/token_info.go (1)
  • TokenInfo (29-36)
rollup/fees/token_transfer.go (1)
  • TransferAltTokenByState (12-37)
rollup/fees/token_info.go (1)
common/types.go (1)
  • BigToHash (62-62)
⏰ 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)
rollup/fees/token_info.go (3)

35-35: Good addition for clarity.

The HasSlot field explicitly indicates whether a token uses a storage slot for balance tracking, improving code readability compared to checking if BalanceSlot is the zero hash.


139-139: LGTM!

The HasSlot field is correctly populated from the computed hasSlot value.


109-118: I need to search the codebase more broadly for where balanceSlot is encoded (set) rather than just decoded. Let me look for all file types and test files.

Document the slot encoding convention with a clear comment explaining the -1 adjustment and link to contract implementation.

The code subtracts 1 from balanceSlotValue (line 116), assuming the contract stores actualSlot + 1 to distinguish slot 0 from "no slot" (zero hash). This encoding convention is not documented in the code or referenced in standard ERC20 patterns. Add an inline comment explaining this encoding scheme and reference the contract implementation that establishes this convention to ensure consistency.

core/token_gas.go (3)

31-31: Good refactoring to use HasSlot.

Using !info.HasSlot is clearer and more explicit than checking if the balance slot equals the zero hash.


47-47: Excellent API simplification.

Consolidating tokenAddress and balanceSlot into a single tokenInfo parameter reduces coupling and prevents parameter mismatches.


51-56: Implementation correctly updated.

The function logic properly uses tokenInfo.HasSlot for branching and accesses tokenInfo.TokenAddress and tokenInfo.BalanceSlot consistently.

@Kukoomomo Kukoomomo merged commit afff4e0 into main Nov 17, 2025
8 checks passed
@Kukoomomo Kukoomomo deleted the slot_add branch November 17, 2025 08:30
This was referenced Dec 16, 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.

3 participants