chore: remove withdrawal fee from non-custodial bridge#1299
Conversation
📝 WalkthroughWalkthroughThe pull request removes withdrawal fee collection logic from Hoodi ERC20 bridge token actions, simplifying balance checks and transaction processing. Associated test files are updated to validate no-fee behavior, and a new coexistence test ensures two bridge instances operate independently. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/streams/hoodi_bridge_coexistence_test.go (2)
38-43: Package-level mutable state may cause issues in parallel test execution.The package-level variables
ttPointCounter,tt2PointCounter,ttPrevPoint, andtt2PrevPointare modified during test execution. While the tests currently run sequentially viaRunSchemaTest, this pattern could lead to data races if tests are ever parallelized or if multiple test functions in this package run concurrently.Consider encapsulating this state in a test struct or resetting these values atomically in
setupBridgeCoexistenceEnvironment. The current reset ofttPrevPointandtt2PrevPointinsetupBridgeCoexistenceEnvironment(lines 64-65) helps, but the counters are not reset.
301-370: Consider extracting common helper logic to reduce duplication.The
giveTTBalance/executeTTWithdrawalfunctions are nearly identical togiveTT2Balance/executeTT2Withdrawal, differing only in the chain/escrow/ERC20 constants and action name. This is acceptable for test code clarity, but could be refactored into parameterized helpers if the pattern expands further.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/migrations/erc20-bridge/001-actions.sqltests/streams/hoodi_bridge_coexistence_test.gotests/streams/hoodi_withdrawal_fee_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/streams/hoodi_withdrawal_fee_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(37-116)internal/migrations/migration.go (1)
GetSeedScriptStatements(60-109)
⏰ 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: slow-integration-tests
- GitHub Check: acceptance-test
🔇 Additional comments (15)
internal/migrations/erc20-bridge/001-actions.sql (4)
24-37: LGTM! Fee removal fromhoodi_tt_bridge_tokensis correctly implemented.The balance check now correctly validates only against
$withdrawal_amount, and the error message clearly indicates the required token amount. The logic flow is clean: check balance → resolve recipient → execute bridge.
62-75: LGTM!hoodi_tt2_bridge_tokensmirrorshoodi_tt_bridge_tokenscorrectly.Both Hoodi bridge actions follow the same no-fee pattern, ensuring consistency across the two bridge instances.
100-133: Verify that retaining fees for Sepolia is intentional.The Sepolia bridge (
sepolia_bridge_tokens) still includes the 40 TRUF withdrawal fee,@leader_sendercheck, andrecord_transaction_eventcall. This differs from the Hoodi bridges which have had fees removed.Is retaining fees for Sepolia intentional? If the goal is to remove fees from all non-custodial bridges, this action may need similar updates.
158-191: Verify that retaining fees for Ethereum mainnet is intentional.The Ethereum mainnet bridge (
ethereum_bridge_tokens) retains the same fee structure as Sepolia. If this PR is specifically targeting Hoodi testnet only, this is fine, but please confirm this is the intended scope.tests/streams/hoodi_bridge_coexistence_test.go (6)
60-105: LGTM! Setup function correctly initializes both bridge instances.The setup properly:
- Resets point trackers for deposit chaining
- Sets the system admin deployer
- Syncs both
hoodi_ttandhoodi_tt2bridge instances- Initializes the extension to load instances into the singleton
This ensures a clean state for each test run.
107-133: LGTM! Test 1 correctly validates TT deposit isolation.The test verifies that depositing to the TT bridge only affects the TT balance while leaving TT2 balance at zero. Good use of distinct user addresses.
135-161: LGTM! Test 2 correctly validates TT2 deposit isolation.Mirrors Test 1's pattern for the TT2 bridge, ensuring symmetrical coverage.
163-223: LGTM! Test 3 thoroughly validates withdrawal isolation.The test:
- Resets deposit chains to ensure clean state
- Gives equal balances to both bridges
- Verifies TT withdrawal doesn't affect TT2 balance and vice versa
Good coverage of the no-fee withdrawal logic.
225-297: LGTM! Test 4 provides comprehensive cross-contamination validation.Tests that operations by different users on different bridges remain completely isolated. This is the most thorough test in the suite.
341-370: LGTM!executeTTWithdrawalhelper correctly constructs the transaction context.The helper properly:
- Creates a
TxContextwith the signer and leader proposer- Calls the
hoodi_tt_bridge_tokensaction- Handles both direct errors and result errors
tests/streams/hoodi_withdrawal_fee_test.go (5)
45-58: LGTM! Test suite correctly renamed to reflect no-fee semantics.The test suite and identifier updates are consistent with the PR objectives. The function tests properly validate:
- Setup environment
- No fee is charged on withdrawal
- Insufficient balance rejection
- Leader receives no fees
96-139: LGTM!testHoodiWithdrawalNoFeecorrectly validates no-fee withdrawal.The test verifies that withdrawing 10 TRUF from a 100 TRUF balance results in exactly 90 TRUF remaining, confirming no fee is deducted.
141-170: LGTM! Insufficient balance test updated correctly.The test now gives the user 5 TRUF and attempts a 10 TRUF withdrawal, which correctly fails. This aligns with the no-fee logic where only the withdrawal amount is checked.
172-225: LGTM! Leader fee test correctly updated to verify no fees are transferred.The test now asserts that the leader's balance remains unchanged after a withdrawal, confirming no fee transfer occurs.
271-307: LGTM! Helper functions remain consistent with the no-fee implementation.The
callHoodiWithdrawalActionhelper correctly callshoodi_tt_bridge_tokenswith just the recipient and amount parameters, and the comment at line 291 accurately reflects "no fees".
resolves: https://github.com/truflation/website/issues/3115
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.