WIP:implement erc20 fee #205
Conversation
generate new type tx
WalkthroughAdds an ERC20-fee transaction type (ERC20FeeTx) and threads an optional FeeTokenID through transactions, messages, APIs, pool validation, signing, state transition, RPC/GraphQL, receipts, and rollup fee logic; plus one simulated backend callMsg accessor. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as eth_sendTransaction
participant Args as SendTxArgs/TransactionArgs
participant Pool as TxPool
participant Sign as Signer
participant Chain as Blockchain
participant ST as StateTransition
participant RPC as RPC/GraphQL
C->>API: Submit tx (optional FeeTokenID)
API->>Args: ToTransaction()/ToMessage
alt FeeTokenID set AND MaxFeePerGas set
Args-->>API: Build ERC20FeeTx
else
Args-->>API: Build existing tx type
end
API->>Pool: validateTx(tx)
alt tx is ERC20FeeTx AND !eip1559
Pool-->>API: ErrTxTypeNotSupported
else
Pool-->>API: accepted
API->>Sign: Sign (London treats ERC20 like Dynamic)
Sign-->>API: Signed tx
API->>Chain: Submit
Chain->>ST: ApplyMessage(msg with FeeTokenID)
alt FeeTokenID != nil
Note right of ST: new ERC20 fee handling branch (exchange/fee conversion)
end
ST-->>Chain: Execution result + typed receipt
Chain-->>RPC: Expose tx/receipt (includes FeeTokenID/gas caps)
RPC-->>C: JSON/GraphQL response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/types/transaction_signing.go (1)
258-268: Fix ERC20FeeTx signing: handle type and include FeeTokenID in hashCurrent code:
- SignatureValues only supports *DynamicFeeTx; ERC20 falls back to EIP-2930 and fails.
- Hash path for ERC20 omits FeeTokenID, so signatures don’t bind the token id.
Apply these diffs:
- Support ERC20 in SignatureValues:
func (s londonSigner) SignatureValues(tx *Transaction, sig []byte) (R, S, V *big.Int, err error) { if tx.IsL1MessageTx() { return nil, nil, nil, fmt.Errorf("l1 message tx do not have a signature") } - txdata, ok := tx.inner.(*DynamicFeeTx) - if !ok { - return s.eip2930Signer.SignatureValues(tx, sig) - } - // Check that chain ID of tx matches the signer. We also accept ID zero here, - // because it indicates that the chain ID was not specified in the tx. - if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { - return nil, nil, nil, ErrInvalidChainId - } + switch txdata := tx.inner.(type) { + case *DynamicFeeTx: + if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { + return nil, nil, nil, ErrInvalidChainId + } + case *ERC20FeeTx: + if txdata.ChainID.Sign() != 0 && txdata.ChainID.Cmp(s.chainId) != 0 { + return nil, nil, nil, ErrInvalidChainId + } + default: + return s.eip2930Signer.SignatureValues(tx, sig) + } R, S, _ = decodeSignature(sig) V = big.NewInt(int64(sig[64])) return R, S, V, nil }
- Bind FeeTokenID in the ERC20 hash:
func (s londonSigner) Hash(tx *Transaction) common.Hash { if tx.IsL1MessageTx() { panic("l1 message tx cannot be signed and do not have a signing hash") } - if tx.Type() != DynamicFeeTxType && tx.Type() != ERC20FeeTxType { + if tx.Type() != DynamicFeeTxType && tx.Type() != ERC20FeeTxType { return s.eip2930Signer.Hash(tx) } - return prefixedRlpHash( - tx.Type(), - []interface{}{ - s.chainId, - tx.Nonce(), - tx.GasTipCap(), - tx.GasFeeCap(), - tx.Gas(), - tx.To(), - tx.Value(), - tx.Data(), - tx.AccessList(), - }) + if tx.Type() == ERC20FeeTxType { + return prefixedRlpHash( + tx.Type(), + []interface{}{ + s.chainId, + tx.Nonce(), + tx.GasTipCap(), + tx.GasFeeCap(), + tx.Gas(), + tx.To(), + tx.Value(), + tx.Data(), + tx.AccessList(), + tx.FeeTokenID(), + }) + } + // DynamicFeeTxType + return prefixedRlpHash( + tx.Type(), + []interface{}{ + s.chainId, + tx.Nonce(), + tx.GasTipCap(), + tx.GasFeeCap(), + tx.Gas(), + tx.To(), + tx.Value(), + tx.Data(), + tx.AccessList(), + }) }These align the signing/verification with the ERC20FeeTx fields.
Also applies to: 300-315
core/types/transaction.go (1)
764-785: PlumbfeeTokenIDthroughAsMessage
StateTransitionnow expectsMessage.FeeTokenID(), butAsMessagenever assigns it, so the accessor always yieldsnil. Please propagate the ID for ERC20-fee txs.msg := Message{ @@ - isL1MessageTx: tx.IsL1MessageTx(), + isL1MessageTx: tx.IsL1MessageTx(), } + if tx.Type() == ERC20FeeTxType { + if erc20 := tx.inner.(*ERC20FeeTx); erc20 != nil { + id := erc20.FeeTokenID + msg.feeTokenID = &id + } + }
🧹 Nitpick comments (1)
core/types/receipt_test.go (1)
83-101: Add tests that exercise ERC20 typed receiptsThe ERC20FeeReceipt fixture is unused. Add marshal/unmarshal parity checks for ERC20FeeTxType (as done for 2930/1559) to guard the new decode/encode paths.
I can draft a test that asserts MarshalBinary == Receipts.EncodeIndex and round-trip UnmarshalBinary for ERC20 receipts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
accounts/abi/bind/backends/simulated.go(1 hunks)accounts/external/backend.go(1 hunks)core/state_transition.go(3 hunks)core/tx_pool.go(1 hunks)core/types/erc20_fee_tx.go(1 hunks)core/types/receipt.go(2 hunks)core/types/receipt_test.go(1 hunks)core/types/transaction.go(4 hunks)core/types/transaction_marshalling.go(1 hunks)core/types/transaction_signing.go(2 hunks)graphql/graphql.go(3 hunks)interfaces.go(1 hunks)internal/ethapi/api.go(2 hunks)signer/core/apitypes/types.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
internal/ethapi/api.go (4)
common/hexutil/json.go (1)
Big(155-155)core/types/transaction.go (1)
ERC20FeeTxType(57-57)core/types/access_list_tx.go (1)
AccessList(30-30)common/math/big.go (1)
BigMin(147-152)
graphql/graphql.go (2)
core/types/transaction.go (1)
ERC20FeeTxType(57-57)common/math/big.go (1)
BigMin(147-152)
core/types/transaction_marshalling.go (3)
core/types/transaction.go (1)
ERC20FeeTxType(57-57)core/types/erc20_fee_tx.go (1)
ERC20FeeTx(27-44)core/types/access_list_tx.go (1)
AccessList(30-30)
accounts/external/backend.go (1)
core/types/transaction.go (1)
ERC20FeeTxType(57-57)
core/types/receipt_test.go (4)
core/types/receipt.go (2)
Receipt(52-81)ReceiptStatusFailed(45-45)core/types/log.go (1)
Log(31-56)common/types.go (2)
BytesToAddress(206-210)HexToHash(66-66)core/types/transaction.go (1)
ERC20FeeTxType(57-57)
core/types/receipt.go (2)
core/types/transaction.go (5)
DynamicFeeTxType(52-52)AccessListTxType(51-51)BlobTxType(53-53)L1MessageTxType(56-56)ERC20FeeTxType(57-57)rlp/encode.go (1)
Encode(61-73)
core/types/transaction_signing.go (1)
core/types/transaction.go (2)
DynamicFeeTxType(52-52)ERC20FeeTxType(57-57)
core/types/transaction.go (3)
core/types/dynamic_fee_tx.go (1)
DynamicFeeTx(27-42)core/state_transition.go (1)
Message(79-95)rollup/fees/rollup_fee.go (1)
Message(26-38)
signer/core/apitypes/types.go (1)
core/types/erc20_fee_tx.go (1)
ERC20FeeTx(27-44)
core/tx_pool.go (2)
core/types/transaction.go (2)
ERC20FeeTxType(57-57)ErrTxTypeNotSupported(39-39)core/error.go (1)
ErrTxTypeNotSupported(89-89)
core/types/erc20_fee_tx.go (3)
core/types/transaction.go (2)
TxData(81-101)ERC20FeeTxType(57-57)common/bytes.go (1)
CopyBytes(37-45)rlp/decode.go (1)
DecodeBytes(92-106)
🔇 Additional comments (3)
core/types/receipt.go (1)
244-251: Add ERC20 typed receipt decode/encode: LGTMDecoding and EncodeIndex now recognize ERC20FeeTxType. Looks consistent with other typed receipts.
Please add/extend tests to cover ERC20 typed receipt marshal/unmarshal parity (similar to existing legacy/2930/1559 cases).
Also applies to: 442-445
interfaces.go (1)
145-147: CallMsg.FeeTokenID added: LGTMField addition aligns with ERC20-fee plumbing.
Confirm eth_call/estimateGas RPC paths map JSON args into CallMsg.FeeTokenID and state transition reads it.
core/tx_pool.go (1)
641-645: Reject ERC20 fee txs pre-1559: LGTMValidation mirrors DynamicFeeTxType gating. Consistent with fork flags.
| case types.ERC20FeeTxType: | ||
| args.MaxFeePerGas = (*hexutil.Big)(tx.GasFeeCap()) | ||
| args.MaxPriorityFeePerGas = (*hexutil.Big)(tx.GasTipCap()) | ||
| //args.FeeTokenID = tx. | ||
| default: |
There was a problem hiding this comment.
Propagate FeeTokenID to external signer args
ERC20 fee txs need FeeTokenID in SendTxArgs; it's currently omitted (commented). Without it, clef cannot construct ERC20FeeTx.
Apply this diff:
case types.ERC20FeeTxType:
args.MaxFeePerGas = (*hexutil.Big)(tx.GasFeeCap())
args.MaxPriorityFeePerGas = (*hexutil.Big)(tx.GasTipCap())
- //args.FeeTokenID = tx.
+ // Wire the fee token id through to clef
+ if tx.FeeTokenID() != nil {
+ args.FeeTokenID = tx.FeeTokenID()
+ }If Transaction lacks FeeTokenID(), assert the inner type and extract the field.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In accounts/external/backend.go around lines 224 to 228, the SendTxArgs for
ERC20FeeTxType currently omits FeeTokenID which prevents clef from building
ERC20 fee transactions; update the case to assert the tx inner type to the ERC20
fee transaction concrete type, extract its FeeTokenID field (ensuring non-nil)
and assign it to args.FeeTokenID (convert to the expected hexutil or pointer
type used elsewhere), and if the transaction lacks a FeeTokenID return or
propagate an error so the caller knows the field is required.
| if msg.FeeTokenID() != nil { | ||
| // TODO | ||
| evm.Context.BaseFee = big.NewInt(0).Mul(evm.Context.BaseFee, big.NewInt(1)) | ||
| l1DataFee = big.NewInt(0).Mul(l1DataFee, big.NewInt(1)) | ||
| } | ||
| return NewStateTransition(evm, msg, gp, l1DataFee).TransitionDb() |
There was a problem hiding this comment.
Avoid panicking when BaseFee is nil
evm.Context.BaseFee is legitimately nil in several code paths (e.g. eth_call with NoBaseFee, pre-London rules). Calling big.Int.Mul with a nil operand will panic, so this TODO stub now crashes any such call. Please guard the mutation or skip it until the final logic is ready.
- if msg.FeeTokenID() != nil {
- // TODO
- evm.Context.BaseFee = big.NewInt(0).Mul(evm.Context.BaseFee, big.NewInt(1))
- l1DataFee = big.NewInt(0).Mul(l1DataFee, big.NewInt(1))
- }
+ if msg.FeeTokenID() != nil {
+ // TODO
+ if evm.Context.BaseFee != nil {
+ evm.Context.BaseFee = new(big.Int).Mul(evm.Context.BaseFee, big.NewInt(1))
+ }
+ l1DataFee = new(big.Int).Mul(l1DataFee, big.NewInt(1))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if msg.FeeTokenID() != nil { | |
| // TODO | |
| evm.Context.BaseFee = big.NewInt(0).Mul(evm.Context.BaseFee, big.NewInt(1)) | |
| l1DataFee = big.NewInt(0).Mul(l1DataFee, big.NewInt(1)) | |
| } | |
| return NewStateTransition(evm, msg, gp, l1DataFee).TransitionDb() | |
| if msg.FeeTokenID() != nil { | |
| // TODO | |
| if evm.Context.BaseFee != nil { | |
| evm.Context.BaseFee = new(big.Int).Mul(evm.Context.BaseFee, big.NewInt(1)) | |
| } | |
| l1DataFee = new(big.Int).Mul(l1DataFee, big.NewInt(1)) | |
| } | |
| return NewStateTransition(evm, msg, gp, l1DataFee).TransitionDb() |
🤖 Prompt for AI Agents
In core/state_transition.go around lines 219 to 224, the TODO stub uses
big.Int.Mul with possibly nil operands which can panic; guard the mutation by
checking evm.Context.BaseFee and l1DataFee for nil before calling Mul (or skip
the multiplication if nil), e.g. only perform the Mul and assign back when the
value is non-nil; if you intend to treat nil as zero, explicitly set a new
big.Int(0) instead of calling Mul on nil.
| FeeTokenID uint16 | ||
|
|
||
| // Signature values | ||
| V *big.Int `json:"v" gencodec:"required"` | ||
| R *big.Int `json:"r" gencodec:"required"` | ||
| S *big.Int `json:"s" gencodec:"required"` | ||
| } | ||
|
|
||
| // copy creates a deep copy of the transaction data and initializes all fields. | ||
| func (tx *ERC20FeeTx) copy() TxData { | ||
| cpy := &ERC20FeeTx{ | ||
| Nonce: tx.Nonce, | ||
| To: copyAddressPtr(tx.To), | ||
| Data: common.CopyBytes(tx.Data), | ||
| Gas: tx.Gas, | ||
| // These are copied below. | ||
| AccessList: make(AccessList, len(tx.AccessList)), | ||
| Value: new(big.Int), | ||
| ChainID: new(big.Int), | ||
| GasTipCap: new(big.Int), | ||
| GasFeeCap: new(big.Int), | ||
| V: new(big.Int), | ||
| R: new(big.Int), | ||
| S: new(big.Int), | ||
| } | ||
| copy(cpy.AccessList, tx.AccessList) | ||
| if tx.Value != nil { | ||
| cpy.Value.Set(tx.Value) | ||
| } | ||
| if tx.ChainID != nil { | ||
| cpy.ChainID.Set(tx.ChainID) | ||
| } | ||
| if tx.GasTipCap != nil { | ||
| cpy.GasTipCap.Set(tx.GasTipCap) | ||
| } | ||
| if tx.GasFeeCap != nil { | ||
| cpy.GasFeeCap.Set(tx.GasFeeCap) | ||
| } | ||
| if tx.V != nil { | ||
| cpy.V.Set(tx.V) | ||
| } | ||
| if tx.R != nil { | ||
| cpy.R.Set(tx.R) | ||
| } | ||
| if tx.S != nil { | ||
| cpy.S.Set(tx.S) | ||
| } | ||
| return cpy |
There was a problem hiding this comment.
Preserve FeeTokenID in copy()
The deep copy drops FeeTokenID, so any cloned tx loses its fee token metadata. Please copy the scalar as well.
cpy := &ERC20FeeTx{
Nonce: tx.Nonce,
To: copyAddressPtr(tx.To),
Data: common.CopyBytes(tx.Data),
Gas: tx.Gas,
+ FeeTokenID: tx.FeeTokenID,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FeeTokenID uint16 | |
| // Signature values | |
| V *big.Int `json:"v" gencodec:"required"` | |
| R *big.Int `json:"r" gencodec:"required"` | |
| S *big.Int `json:"s" gencodec:"required"` | |
| } | |
| // copy creates a deep copy of the transaction data and initializes all fields. | |
| func (tx *ERC20FeeTx) copy() TxData { | |
| cpy := &ERC20FeeTx{ | |
| Nonce: tx.Nonce, | |
| To: copyAddressPtr(tx.To), | |
| Data: common.CopyBytes(tx.Data), | |
| Gas: tx.Gas, | |
| // These are copied below. | |
| AccessList: make(AccessList, len(tx.AccessList)), | |
| Value: new(big.Int), | |
| ChainID: new(big.Int), | |
| GasTipCap: new(big.Int), | |
| GasFeeCap: new(big.Int), | |
| V: new(big.Int), | |
| R: new(big.Int), | |
| S: new(big.Int), | |
| } | |
| copy(cpy.AccessList, tx.AccessList) | |
| if tx.Value != nil { | |
| cpy.Value.Set(tx.Value) | |
| } | |
| if tx.ChainID != nil { | |
| cpy.ChainID.Set(tx.ChainID) | |
| } | |
| if tx.GasTipCap != nil { | |
| cpy.GasTipCap.Set(tx.GasTipCap) | |
| } | |
| if tx.GasFeeCap != nil { | |
| cpy.GasFeeCap.Set(tx.GasFeeCap) | |
| } | |
| if tx.V != nil { | |
| cpy.V.Set(tx.V) | |
| } | |
| if tx.R != nil { | |
| cpy.R.Set(tx.R) | |
| } | |
| if tx.S != nil { | |
| cpy.S.Set(tx.S) | |
| } | |
| return cpy | |
| func (tx *ERC20FeeTx) copy() TxData { | |
| cpy := &ERC20FeeTx{ | |
| Nonce: tx.Nonce, | |
| To: copyAddressPtr(tx.To), | |
| Data: common.CopyBytes(tx.Data), | |
| Gas: tx.Gas, | |
| FeeTokenID: tx.FeeTokenID, | |
| // These are copied below. | |
| AccessList: make(AccessList, len(tx.AccessList)), | |
| Value: new(big.Int), | |
| ChainID: new(big.Int), | |
| GasTipCap: new(big.Int), | |
| GasFeeCap: new(big.Int), | |
| V: new(big.Int), | |
| R: new(big.Int), | |
| S: new(big.Int), | |
| } | |
| copy(cpy.AccessList, tx.AccessList) | |
| if tx.Value != nil { | |
| cpy.Value.Set(tx.Value) | |
| } | |
| if tx.ChainID != nil { | |
| cpy.ChainID.Set(tx.ChainID) | |
| } | |
| if tx.GasTipCap != nil { | |
| cpy.GasTipCap.Set(tx.GasTipCap) | |
| } | |
| if tx.GasFeeCap != nil { | |
| cpy.GasFeeCap.Set(tx.GasFeeCap) | |
| } | |
| if tx.V != nil { | |
| cpy.V.Set(tx.V) | |
| } | |
| if tx.R != nil { | |
| cpy.R.Set(tx.R) | |
| } | |
| if tx.S != nil { | |
| cpy.S.Set(tx.S) | |
| } | |
| return cpy | |
| } |
🤖 Prompt for AI Agents
In core/types/erc20_fee_tx.go around lines 38 to 85, the copy() method builds a
new ERC20FeeTx but omits copying the FeeTokenID field so cloned transactions
lose their fee token metadata; include FeeTokenID in the copied struct (e.g.,
set FeeTokenID: tx.FeeTokenID in the struct literal or assign cpy.FeeTokenID =
tx.FeeTokenID immediately after creating cpy) so the scalar is preserved in the
deep copy.
| } | ||
| // TODO FeeTokenID | ||
| itx.Value = (*big.Int)(dec.Value) | ||
| if dec.Input == nil { | ||
| return errors.New("missing required field 'input' in transaction") | ||
| } | ||
| itx.Data = *dec.Input | ||
| if dec.AccessList != nil { | ||
| itx.AccessList = *dec.AccessList | ||
| } | ||
|
|
There was a problem hiding this comment.
Don’t drop feeTokenID during JSON decode
UnmarshalJSON never copies the feeTokenID, so round-tripping JSON nixes the token identifier. Please parse and assign it.
- // TODO FeeTokenID
- itx.Value = (*big.Int)(dec.Value)
+ itx.Value = (*big.Int)(dec.Value)
+ if dec.FeeTokenID != nil {
+ itx.FeeTokenID = uint16(dec.FeeTokenID.ToInt().Uint64())
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| // TODO FeeTokenID | |
| itx.Value = (*big.Int)(dec.Value) | |
| if dec.Input == nil { | |
| return errors.New("missing required field 'input' in transaction") | |
| } | |
| itx.Data = *dec.Input | |
| if dec.AccessList != nil { | |
| itx.AccessList = *dec.AccessList | |
| } | |
| } | |
| itx.Value = (*big.Int)(dec.Value) | |
| if dec.FeeTokenID != nil { | |
| itx.FeeTokenID = uint16(dec.FeeTokenID.ToInt().Uint64()) | |
| } | |
| if dec.Input == nil { | |
| return errors.New("missing required field 'input' in transaction") | |
| } | |
| itx.Data = *dec.Input | |
| if dec.AccessList != nil { | |
| itx.AccessList = *dec.AccessList | |
| } |
🤖 Prompt for AI Agents
In core/types/transaction_marshalling.go around lines 576 to 586, the
UnmarshalJSON path sets Value, Input, Data and AccessList but never copies
feeTokenID from the decoded struct; update the routine to check dec.FeeTokenID
for nil and, if present, assign it to the transaction (e.g., set itx.FeeTokenID
= *dec.FeeTokenID or set the pointer appropriately to preserve the identifier),
ensuring any necessary type conversion is applied and nil is handled to avoid
losing feeTokenID during JSON round-trip.
| al := tx.AccessList() | ||
| result.Accesses = &al | ||
| result.ChainID = (*hexutil.Big)(tx.ChainId()) | ||
| result.GasFeeCap = (*hexutil.Big)(tx.GasFeeCap()) | ||
| result.GasTipCap = (*hexutil.Big)(tx.GasTipCap()) | ||
| // TODO | ||
| //result.FeeTokenID = (*hexutil.Big)() | ||
| // if the transaction has been mined, compute the effective gas price | ||
| if baseFee != nil && blockHash != (common.Hash{}) { | ||
| // price = min(tip, gasFeeCap - baseFee) + baseFee | ||
| // TODO base fee -> erc20 fee | ||
| price := math.BigMin(new(big.Int).Add(tx.GasTipCap(), baseFee), tx.GasFeeCap()) | ||
| result.GasPrice = (*hexutil.Big)(price) | ||
| } else { | ||
| result.GasPrice = (*hexutil.Big)(tx.GasFeeCap()) | ||
| } |
There was a problem hiding this comment.
Populate feeTokenID in RPC payloads
We add feeTokenID to the RPC surface, but the ERC20 branch leaves it nil, so clients never learn which token paid the fee. Please plumb the actual ID from the transaction before shipping.
case types.ERC20FeeTxType:
al := tx.AccessList()
result.Accesses = &al
result.ChainID = (*hexutil.Big)(tx.ChainId())
result.GasFeeCap = (*hexutil.Big)(tx.GasFeeCap())
result.GasTipCap = (*hexutil.Big)(tx.GasTipCap())
- // TODO
- //result.FeeTokenID = (*hexutil.Big)()
+ if tx.FeeTokenID() != nil {
+ result.FeeTokenID = (*hexutil.Big)(new(big.Int).SetUint64(uint64(*tx.FeeTokenID())))
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/ethapi/api.go around lines 1442 to 1457, the RPC payload leaves
result.FeeTokenID nil for ERC20-fee transactions; populate it from the
transaction's fee token field. Read the transaction's FeeTokenID (or equivalent
getter), convert/wrap it into the same hexutil.Big type used for other big
fields, and assign it to result.FeeTokenID, guarding for nil/empty values so you
don't dereference a nil pointer.
| FeeTokenID *uint16 `json:"feeTokenID,omitempty"` | ||
| } |
There was a problem hiding this comment.
ERC20FeeTx construction is overwritten; use else-branch
When FeeTokenID is set, you build ERC20FeeTx, then immediately overwrite with DynamicFeeTx.
Apply this diff:
@@
type SendTxArgs struct {
@@
- ChainID *hexutil.Big `json:"chainId,omitempty"`
- FeeTokenID *uint16 `json:"feeTokenID,omitempty"`
+ ChainID *hexutil.Big `json:"chainId,omitempty"`
+ FeeTokenID *uint16 `json:"feeTokenID,omitempty"`
@@
- if args.FeeTokenID != nil {
- data = &types.ERC20FeeTx{
- To: to,
- ChainID: (*big.Int)(args.ChainID),
- Nonce: uint64(args.Nonce),
- Gas: uint64(args.Gas),
- GasFeeCap: (*big.Int)(args.MaxFeePerGas),
- GasTipCap: (*big.Int)(args.MaxPriorityFeePerGas),
- FeeTokenID: *args.FeeTokenID,
- Value: (*big.Int)(&args.Value),
- Data: input,
- AccessList: al,
- }
- }
- data = &types.DynamicFeeTx{
+ if args.FeeTokenID != nil {
+ data = &types.ERC20FeeTx{
+ To: to,
+ ChainID: (*big.Int)(args.ChainID),
+ Nonce: uint64(args.Nonce),
+ Gas: uint64(args.Gas),
+ GasFeeCap: (*big.Int)(args.MaxFeePerGas),
+ GasTipCap: (*big.Int)(args.MaxPriorityFeePerGas),
+ FeeTokenID: *args.FeeTokenID,
+ Value: (*big.Int)(&args.Value),
+ Data: input,
+ AccessList: al,
+ }
+ } else {
+ data = &types.DynamicFeeTx{
To: to,
ChainID: (*big.Int)(args.ChainID),
Nonce: uint64(args.Nonce),
Gas: uint64(args.Gas),
GasFeeCap: (*big.Int)(args.MaxFeePerGas),
GasTipCap: (*big.Int)(args.MaxPriorityFeePerGas),
Value: (*big.Int)(&args.Value),
Data: input,
AccessList: al,
- }
+ }
+ }Also applies to: 124-149
🤖 Prompt for AI Agents
In signer/core/apitypes/types.go around lines 90-91 (and similarly lines
124-149), the code constructs an ERC20FeeTx when FeeTokenID is present but then
immediately overwrites it with a DynamicFeeTx; change the control flow so that
the DynamicFeeTx construction happens only in the else branch when FeeTokenID is
nil. Specifically, wrap the DynamicFeeTx assignment in an else block
corresponding to the FeeTokenID check (and apply the same change to the second
occurrence at lines 124-149) so that ERC20FeeTx is preserved when FeeTokenID is
set.
* clean msg * fix asUnsignedTx * fix decodeTyped
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/types/transaction.go (1)
766-783: *Update NewMessage signature to accept uint16 and refactor all callers
- Change signature in core/types/transaction.go to
feeTokenID *uint16and assign directly.- Update callers to pass a
*uint16(e.g.&feeTokenIDor existingargs.FeeTokenID) ornilin:
• tests/state_test_util.go
• les/odr_test.go
• light/odr_test.go
• internal/ethapi/transaction_args.go
♻️ Duplicate comments (1)
core/types/transaction.go (1)
821-821: FeeTokenID getter now returns the correct field — resolvedMessage.FeeTokenID() correctly returns m.feeTokenID.
🧹 Nitpick comments (1)
internal/ethapi/transaction_args.go (1)
52-54: JSON field naming: prefer camelCase for consistencyOther args use camelCase (e.g., maxFeePerGas). Consider
json:"feeTokenID,omitempty"to align RPC ergonomics.- FeeTokenID *hexutil.Uint64 `json:"fee_token_id,omitempty"` + FeeTokenID *hexutil.Uint64 `json:"feeTokenID,omitempty"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
accounts/abi/bind/backends/simulated.go(1 hunks)core/state_transition.go(3 hunks)core/types/transaction.go(8 hunks)internal/ethapi/transaction_args.go(4 hunks)rollup/fees/rollup_fee.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/state_transition.go
🧰 Additional context used
🧬 Code graph analysis (4)
core/types/transaction.go (3)
core/types/erc20_fee_tx.go (1)
ERC20FeeTx(27-44)core/state_transition.go (1)
Message(79-95)rollup/fees/rollup_fee.go (1)
Message(26-39)
internal/ethapi/transaction_args.go (5)
common/hexutil/json.go (1)
Uint64(275-275)core/types/transaction.go (1)
NewMessage(766-782)core/types/erc20_fee_tx.go (1)
ERC20FeeTx(27-44)internal/ethapi/api.go (1)
AccessList(1534-1606)core/types/dynamic_fee_tx.go (1)
DynamicFeeTx(27-42)
rollup/fees/rollup_fee.go (3)
core/state_transition.go (1)
Message(79-95)core/types/transaction.go (1)
Message(750-764)core/types/erc20_fee_tx.go (1)
ERC20FeeTx(27-44)
accounts/abi/bind/backends/simulated.go (1)
interfaces.go (1)
CallMsg(129-147)
🔇 Additional comments (3)
accounts/abi/bind/backends/simulated.go (1)
918-918: FeeTokenID accessor correctly delegates — LGTMReturning m.CallMsg.FeeTokenID matches the Message interface and unblocks fee-token aware paths.
internal/ethapi/transaction_args.go (1)
167-171: Good: propagate FeeTokenID to gas estimationIncluding FeeTokenID in callArgs ensures gas estimation reflects ERC20-fee semantics.
core/types/transaction.go (1)
211-213: Correct decoder: ERC20FeeTxType now uses ERC20FeeTxThis resolves mis-parsing of ERC20-fee typed txs in decodeTyped.
| var feeTokenID uint16 | ||
| if args.FeeTokenID != nil { | ||
| feeTokenID = uint16(*args.FeeTokenID) | ||
| } | ||
| msg := types.NewMessage(addr, args.To, 0, value, gas, gasPrice, gasFeeCap, gasTipCap, feeTokenID, data, accessList, true) | ||
| return msg, nil |
There was a problem hiding this comment.
Bug: All messages get treated as ERC20 due to non‑nil pointer; also missing u64→u16 bounds check
- NewMessage(uint16) always sets a non‑nil pointer internally, so when args.FeeTokenID is nil, msg.FeeTokenID() still becomes non‑nil, causing asUnsignedTx to pick the ERC20 path for all 1559 calls.
- Casting hexutil.Uint64→uint16 without range validation silently truncates.
Fix by:
- Changing types.NewMessage to take a *uint16 (nil means “no ERC20 fee token”).
- Passing a nil or validated pointer here.
- Adding a bounds check.
Apply this diff here (paired with the types.NewMessage change below):
- var feeTokenID uint16
- if args.FeeTokenID != nil {
- feeTokenID = uint16(*args.FeeTokenID)
- }
- msg := types.NewMessage(addr, args.To, 0, value, gas, gasPrice, gasFeeCap, gasTipCap, feeTokenID, data, accessList, true)
+ // Validate and build optional fee token pointer
+ var feeTokenIDPtr *uint16
+ if args.FeeTokenID != nil {
+ if uint64(*args.FeeTokenID) > 0xFFFF {
+ return types.Message{}, fmt.Errorf("feeTokenID out of range: %d", uint64(*args.FeeTokenID))
+ }
+ v := uint16(*args.FeeTokenID)
+ feeTokenIDPtr = &v
+ }
+ msg := types.NewMessage(addr, args.To, 0, value, gas, gasPrice, gasFeeCap, gasTipCap, feeTokenIDPtr, data, accessList, true)Additionally, ensure setDefaults enforces the same 0xFFFF bound so toTransaction cannot observe an invalid value. Example insertion in setDefaults (conceptual):
if args.FeeTokenID != nil && uint64(*args.FeeTokenID) > 0xFFFF {
return fmt.Errorf("feeTokenID out of range: %d", uint64(*args.FeeTokenID))
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var feeTokenID uint16 | |
| if args.FeeTokenID != nil { | |
| feeTokenID = uint16(*args.FeeTokenID) | |
| } | |
| msg := types.NewMessage(addr, args.To, 0, value, gas, gasPrice, gasFeeCap, gasTipCap, feeTokenID, data, accessList, true) | |
| return msg, nil | |
| // Validate and build optional fee token pointer | |
| var feeTokenIDPtr *uint16 | |
| if args.FeeTokenID != nil { | |
| if uint64(*args.FeeTokenID) > 0xFFFF { | |
| return types.Message{}, fmt.Errorf("feeTokenID out of range: %d", uint64(*args.FeeTokenID)) | |
| } | |
| v := uint16(*args.FeeTokenID) | |
| feeTokenIDPtr = &v | |
| } | |
| msg := types.NewMessage(addr, args.To, 0, value, gas, gasPrice, gasFeeCap, gasTipCap, feeTokenIDPtr, data, accessList, true) | |
| return msg, nil |
| if args.FeeTokenID != nil { | ||
| data = &types.ERC20FeeTx{ | ||
| To: args.To, | ||
| ChainID: (*big.Int)(args.ChainID), | ||
| Nonce: uint64(*args.Nonce), | ||
| Gas: uint64(*args.Gas), | ||
| GasFeeCap: (*big.Int)(args.MaxFeePerGas), | ||
| GasTipCap: (*big.Int)(args.MaxPriorityFeePerGas), | ||
| FeeTokenID: uint16(*args.FeeTokenID), | ||
| Value: (*big.Int)(args.Value), | ||
| Data: args.data(), | ||
| AccessList: al, | ||
| } | ||
| } else { | ||
| data = &types.DynamicFeeTx{ | ||
| To: args.To, | ||
| ChainID: (*big.Int)(args.ChainID), | ||
| Nonce: uint64(*args.Nonce), | ||
| Gas: uint64(*args.Gas), | ||
| GasFeeCap: (*big.Int)(args.MaxFeePerGas), | ||
| GasTipCap: (*big.Int)(args.MaxPriorityFeePerGas), | ||
| Value: (*big.Int)(args.Value), | ||
| Data: args.data(), | ||
| AccessList: al, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Guard against out‑of‑range FeeTokenID before building ERC20FeeTx
This path truncates without validation: uint16(*args.FeeTokenID). Since toTransaction can’t return an error, rely on setDefaults to validate (see prior comment). If not feasible, early‑reject in the RPC layer before calling toTransaction.
Would you like me to add the validation in setDefaults and wire an error back through the API?
🏁 Script executed:
#!/bin/bash
set -e
echo "Finding TransactionArgs struct and FeeTokenID in transaction_args.go"
rg -n "type .*Args" -g "internal/ethapi/transaction_args.go" || true
rg -n "FeeTokenID" -g "internal/ethapi/transaction_args.go" || true
echo "Searching for setDefaults definitions"
rg -n "func .*setDefaults" -g "*.go" || true
echo "Searching for toTransaction methods"
rg -n "func .*toTransaction" -g "*.go" || trueLength of output: 981
🏁 Script executed:
#!/bin/bash
set -e
echo "=== setDefaults ==="
rg -n "func (args \\*TransactionArgs) setDefaults" -C5 internal/ethapi/transaction_args.go
echo ""
echo "=== toTransaction ==="
rg -n "func (args \\*TransactionArgs) toTransaction" -C5 internal/ethapi/transaction_args.goLength of output: 19
🏁 Script executed:
#!/bin/bash
set -e
echo "=== setDefaults definition ==="
rg -n '^func \(args \*TransactionArgs\) setDefaults' -C10 internal/ethapi/transaction_args.go
echo ""
echo "=== toTransaction definition ==="
rg -n '^func \(args \*TransactionArgs\) toTransaction' -C10 internal/ethapi/transaction_args.goLength of output: 1711
Guard FeeTokenID range in setDefaults
toTransaction truncates uint64→uint16 without validation, so values > 65535 wrap silently. Add a range check in TransactionArgs.setDefaults to reject out-of-range FeeTokenID (return an error) or validate before calling toTransaction.
🤖 Prompt for AI Agents
In internal/ethapi/transaction_args.go around lines 272 to 297, to prevent
silent truncation when converting FeeTokenID from uint64 to uint16 in
toTransaction, add a validation in TransactionArgs.setDefaults that checks if
FeeTokenID (when non-nil) is <= 65535 and return an explicit error if it is out
of range; ensure callers handle that error before toTransaction is invoked (or
alternatively validate and coerce into a safe uint16 there) so toTransaction no
longer needs to assume the value fits.
| func ExchangeToERC20(state StateDB, feeTokenID *uint16, feeByEth *big.Int) *big.Int { | ||
| // TODO | ||
| return big.NewInt(1) | ||
| } | ||
|
|
||
| func ExchangeToEth(state StateDB, feeTokenID *uint16, feeByERC20 *big.Int) *big.Int { | ||
| // TODO | ||
| return big.NewInt(1) | ||
| } |
There was a problem hiding this comment.
Critical: Exchange stubs return constant 1 — breaks fee accounting
Returning 1 wei/token yields grossly incorrect fees and risks acceptance/economic safety. Until implemented, pass through the original amount to avoid under/overcharging.
Apply this safe placeholder diff:
-func ExchangeToERC20(state StateDB, feeTokenID *uint16, feeByEth *big.Int) *big.Int {
- // TODO
- return big.NewInt(1)
-}
+func ExchangeToERC20(state StateDB, feeTokenID *uint16, feeByEth *big.Int) *big.Int {
+ // TODO: implement proper exchange via state (e.g., oracle). Temporary passthrough.
+ if feeByEth == nil {
+ return new(big.Int)
+ }
+ return new(big.Int).Set(feeByEth)
+}
-func ExchangeToEth(state StateDB, feeTokenID *uint16, feeByERC20 *big.Int) *big.Int {
- // TODO
- return big.NewInt(1)
-}
+func ExchangeToEth(state StateDB, feeTokenID *uint16, feeByERC20 *big.Int) *big.Int {
+ // TODO: implement proper exchange via state (e.g., oracle). Temporary passthrough.
+ if feeByERC20 == nil {
+ return new(big.Int)
+ }
+ return new(big.Int).Set(feeByERC20)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func ExchangeToERC20(state StateDB, feeTokenID *uint16, feeByEth *big.Int) *big.Int { | |
| // TODO | |
| return big.NewInt(1) | |
| } | |
| func ExchangeToEth(state StateDB, feeTokenID *uint16, feeByERC20 *big.Int) *big.Int { | |
| // TODO | |
| return big.NewInt(1) | |
| } | |
| func ExchangeToERC20(state StateDB, feeTokenID *uint16, feeByEth *big.Int) *big.Int { | |
| // TODO: implement proper exchange via state (e.g., oracle). Temporary passthrough. | |
| if feeByEth == nil { | |
| return new(big.Int) | |
| } | |
| return new(big.Int).Set(feeByEth) | |
| } | |
| func ExchangeToEth(state StateDB, feeTokenID *uint16, feeByERC20 *big.Int) *big.Int { | |
| // TODO: implement proper exchange via state (e.g., oracle). Temporary passthrough. | |
| if feeByERC20 == nil { | |
| return new(big.Int) | |
| } | |
| return new(big.Int).Set(feeByERC20) | |
| } |
🤖 Prompt for AI Agents
In rollup/fees/rollup_fee.go around lines 259 to 267 the ExchangeToERC20 and
ExchangeToEth stubs return constant 1 which breaks fee accounting; replace them
with safe pass-throughs that return the original amount unchanged:
ExchangeToERC20 should return feeByEth (or a copy like
new(big.Int).Set(feeByEth)) and ExchangeToEth should return feeByERC20 (or a
copy), and ensure nil input handling (return zero value new(big.Int).SetInt64(0)
if the incoming *big.Int is nil) so callers receive the original amount until
real exchange logic is implemented.
1 participant
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
New Features
Behavior Changes
Tests