chore: implement fee for withdrawal#1251
Conversation
Implement flat 40 TRUF fee for L1 bridge withdrawals to cover validator transaction costs and compensate block proposers. resolves: trufnetwork/truf-network#1315
WalkthroughThe changes implement fee-gated withdrawals to L1 by modifying ERC20 bridge actions to charge a fixed 40 TRUF fee. The SQL migration adds balance verification, fee collection logic, and transfers the fee to a leader address before executing the withdrawal. Comprehensive tests validate fee deduction, insufficient balance scenarios, and leader fee distribution. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bridge Action
participant Balance Check
participant Fee Transfer
participant Bridge Execution
User->>Bridge Action: withdraw(amount, recipient)
activate Bridge Action
Bridge Action->>Bridge Action: withdrawal_fee := 40 TRUF
Bridge Action->>Bridge Action: total_required := amount + withdrawal_fee
Bridge Action->>Balance Check: query caller balance
activate Balance Check
Balance Check-->>Bridge Action: balance
deactivate Balance Check
alt balance < total_required
Bridge Action-->>User: ❌ Insufficient balance error
else balance >= total_required
Bridge Action->>Fee Transfer: transfer withdrawal_fee to leader_sender
activate Fee Transfer
Fee Transfer-->>Bridge Action: ✓ Fee transferred
deactivate Fee Transfer
rect rgb(200, 220, 240)
note right of Bridge Execution: Original bridge logic executes<br/>with computed withdrawal_amount
Bridge Action->>Bridge Execution: execute withdrawal(withdrawal_amount)
Bridge Execution-->>User: ✓ Withdrawal confirmed
end
end
deactivate Bridge Action
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 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 |
Time Submission Status
|
![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
🧹 Nitpick comments (5)
internal/migrations/erc20-bridge/001-actions.sql (3)
32-37: Inconsistent units in error message.The error message formats
$total_requiredin TRUF but shows$withdrawal_amountin wei, creating a confusing message like "Required: 50 TRUF (10000000000000000000 wei withdrawal + 40 TRUF fee)".Consider formatting all amounts consistently in TRUF for better user experience.
Apply this diff:
IF $caller_balance < $total_required { ERROR('Insufficient balance for withdrawal. Required: ' || ($total_required / '1000000000000000000'::NUMERIC(78, 0))::TEXT || - ' TRUF (' || $withdrawal_amount::TEXT || ' wei withdrawal + ' || + ' TRUF (' || ($withdrawal_amount / '1000000000000000000'::NUMERIC(78, 0))::TEXT || ' TRUF withdrawal + ' || ($withdrawal_fee / '1000000000000000000'::NUMERIC(78, 0))::TEXT || ' TRUF fee)'); }
78-83: Inconsistent units in error message.Same issue as in
sepolia_bridge_tokens: the error message mixes wei and TRUF units, which can confuse users.Apply this diff:
IF $caller_balance < $total_required { ERROR('Insufficient balance for withdrawal. Required: ' || ($total_required / '1000000000000000000'::NUMERIC(78, 0))::TEXT || - ' TRUF (' || $withdrawal_amount::TEXT || ' wei withdrawal + ' || + ' TRUF (' || ($withdrawal_amount / '1000000000000000000'::NUMERIC(78, 0))::TEXT || ' TRUF withdrawal + ' || ($withdrawal_fee / '1000000000000000000'::NUMERIC(78, 0))::TEXT || ' TRUF fee)'); }
26-26: Consider centralizing the fee constant.The 40 TRUF fee is hardcoded in both
sepolia_bridge_tokensandethereum_bridge_tokens. Future fee adjustments will require updating both locations.While SQL doesn't offer elegant constant mechanisms, you could consider storing the fee in a configuration table or view to maintain a single source of truth.
Also applies to: 72-72
tests/streams/withdrawal_fee_test.go (2)
208-226: Remove unused helper functions.Both
createEngineContext(lines 208-226) andexecuteWithdrawal(lines 292-302) are defined but never called. These appear to be leftover from refactoring or alternative implementations.Removing dead code improves maintainability and reduces confusion for future developers.
Apply this diff to remove the unused functions:
-// createEngineContext creates a standard EngineContext for withdrawal tests -func createEngineContext(ctx context.Context, platform *kwilTesting.Platform, signerBytes []byte, caller string, height int64) *common.EngineContext { - // Generate a leader public key for fee collection - _, leaderPubGeneric, err := crypto.GenerateSecp256k1Key(nil) - if err != nil { - panic(fmt.Sprintf("failed to generate leader key: %v", err)) - } - leaderPub := leaderPubGeneric.(*crypto.Secp256k1PublicKey) - - return &common.EngineContext{ - TxContext: &common.TxContext{ - Ctx: ctx, - BlockContext: &common.BlockContext{Height: height, Proposer: leaderPub}, - Signer: signerBytes, - Caller: caller, - TxID: platform.Txid(), - Authenticator: coreauth.EthPersonalSignAuth, - }, - } -} - // giveWithdrawalBalance credits TRUF balance to a wallet using ERC20 inject [... continue with removal of executeWithdrawal ...] -// executeWithdrawal executes a withdrawal with a randomly generated leader -func executeWithdrawal(ctx context.Context, platform *kwilTesting.Platform, signer *util.EthereumAddress, recipient string, amount string) error { - // Generate random leader - _, pubGeneric, err := crypto.GenerateSecp256k1Key(nil) - if err != nil { - return err - } - pub := pubGeneric.(*crypto.Secp256k1PublicKey) - - return callWithdrawalAction(ctx, platform, signer, pub, recipient, amount) -} - // executeWithdrawalWithLeader executes a withdrawal with a specific leader (for testing fee recipient)Also applies to: 292-302
279-279: Consider adding a comment about test coverage.The tests only invoke
sepolia_bridge_tokens, notethereum_bridge_tokens. While this is sufficient becausemigration.goreplacesethereum_bridgewithsepolia_bridgein tests (making both functions effectively identical), a comment would help future maintainers understand the test scope.res, err := platform.Engine.Call( engineCtx, platform.DB, "", - "sepolia_bridge_tokens", // Test-specific action (ethereum_bridge_tokens also becomes this in tests) + "sepolia_bridge_tokens", // Test-specific action. Note: migration.go replaces ethereum_bridge with sepolia_bridge + // in test environment, so this covers both sepolia_bridge_tokens and ethereum_bridge_tokens []any{recipient, amount}, func(row *common.Row) error { return nil }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/erc20-bridge/001-actions.sql(2 hunks)tests/streams/withdrawal_fee_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: williamrusdyputra
Repo: trufnetwork/node PR: 1228
File: internal/migrations/erc20-bridge/000-extension.sql:16-19
Timestamp: 2025-10-23T07:00:26.796Z
Learning: In internal/migrations/erc20-bridge/000-extension.sql, the active sepolia_bridge configuration is intentionally for test environments only and does not include the distribution_period parameter. The commented sepolia_bridge and ethereum_bridge blocks with distribution_period are production configurations.
📚 Learning: 2025-10-23T07:00:26.796Z
Learnt from: williamrusdyputra
Repo: trufnetwork/node PR: 1228
File: internal/migrations/erc20-bridge/000-extension.sql:16-19
Timestamp: 2025-10-23T07:00:26.796Z
Learning: In internal/migrations/erc20-bridge/000-extension.sql, the active sepolia_bridge configuration is intentionally for test environments only and does not include the distribution_period parameter. The commented sepolia_bridge and ethereum_bridge blocks with distribution_period are production configurations.
Applied to files:
internal/migrations/erc20-bridge/001-actions.sql
📚 Learning: 2025-09-12T01:19:04.545Z
Learnt from: outerlook
Repo: trufnetwork/node PR: 1147
File: tests/extensions/erc20/erc20_bridge_actions_smoke_test.go:40-47
Timestamp: 2025-09-12T01:19:04.545Z
Learning: In ERC20 bridge tests, the maintainer (outerlook) prefers fail-fast behavior through panics when schema changes occur. Direct indexing and type assertions in test row handlers should be maintained to immediately catch unexpected schema changes, rather than using graceful error handling.
Applied to files:
internal/migrations/erc20-bridge/001-actions.sql
🧬 Code graph analysis (1)
tests/streams/withdrawal_fee_test.go (5)
tests/streams/utils/runner.go (1)
RunSchemaTest(37-116)internal/migrations/migration.go (1)
GetSeedScriptStatements(60-109)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(75-88)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-85)tests/streams/utils/erc20/helper.go (1)
GetUserBalance(49-74)
⏰ 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). (2)
- GitHub Check: acceptance-test
- GitHub Check: lint
🔇 Additional comments (3)
tests/streams/withdrawal_fee_test.go (2)
24-86: LGTM!The test constants are well-defined, and the setup function correctly initializes the bridge environment. The use of
ForTestingForceSyncInstanceandForTestingInitializeExtensionproperly prepares the test environment.
88-203: LGTM!The test cases comprehensively validate the withdrawal fee implementation:
- Fee deduction correctness (withdrawal amount + 40 TRUF)
- Insufficient balance error handling
- Leader fee distribution
Each test properly re-initializes the extension and uses unique addresses to prevent test interference.
internal/migrations/erc20-bridge/001-actions.sql (1)
30-40: Critical: Wrong bridge namespace in sepolia_bridge_tokens.Lines 30 and 40 reference
ethereum_bridgeinstead ofsepolia_bridge. This will fail in production because:
- Line 30:
ethereum_bridge.balance(@caller)should besepolia_bridge.balance(@caller)- Line 40:
ethereum_bridge.transfer(...)should besepolia_bridge.transfer(...)This bug is masked in tests because
migration.goperforms string replacement (ethereum_bridge→sepolia_bridge), but production will use the wrong bridge instance.Apply this diff:
- $caller_balance := COALESCE(ethereum_bridge.balance(@caller), 0::NUMERIC(78, 0)); + $caller_balance := COALESCE(sepolia_bridge.balance(@caller), 0::NUMERIC(78, 0)); IF $caller_balance < $total_required { ERROR('Insufficient balance for withdrawal. Required: ' || ($total_required / '1000000000000000000'::NUMERIC(78, 0))::TEXT || ' TRUF (' || $withdrawal_amount::TEXT || ' wei withdrawal + ' || ($withdrawal_fee / '1000000000000000000'::NUMERIC(78, 0))::TEXT || ' TRUF fee)'); } $leader_addr TEXT := encode(@leader_sender, 'hex')::TEXT; - ethereum_bridge.transfer($leader_addr, $withdrawal_fee); + sepolia_bridge.transfer($leader_addr, $withdrawal_fee);⛔ Skipped due to learnings
Learnt from: williamrusdyputra Repo: trufnetwork/node PR: 1228 File: internal/migrations/erc20-bridge/000-extension.sql:16-19 Timestamp: 2025-10-23T07:00:26.796Z Learning: In internal/migrations/erc20-bridge/000-extension.sql, the active sepolia_bridge configuration is intentionally for test environments only and does not include the distribution_period parameter. The commented sepolia_bridge and ethereum_bridge blocks with distribution_period are production configurations.Learnt from: outerlook Repo: trufnetwork/node PR: 1147 File: tests/extensions/erc20/erc20_bridge_actions_smoke_test.go:40-47 Timestamp: 2025-09-12T01:19:04.545Z Learning: In ERC20 bridge tests, the maintainer (outerlook) prefers fail-fast behavior through panics when schema changes occur. Direct indexing and type assertions in test row handlers should be maintained to immediately catch unexpected schema changes, rather than using graceful error handling.
2 participants
Implement flat 40 TRUF fee for L1 bridge withdrawals to cover validator transaction costs and compensate block proposers.
resolves: https://github.com/trufnetwork/truf-network/issues/1315
Summary by CodeRabbit
New Features
Tests