feat: apply fee when withdrawing TRUF tokens#1158
Conversation
WalkthroughAdds a public view action exposing ERC20 bridge metadata, removes role-based wallet-balance permission checks and individual admin lock/unlock/issue actions, and consolidates per-network admin bridge flows to calculate a 1% treasury fee, lock it, and bridge the remaining 99%. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin
participant DB as SQL Actions
participant Treasury as Treasury (lock)
participant Bridge as Bridge Engine
participant User as User Wallet
Admin->>DB: sepolia_admin_bridge_tokens(amount)
rect rgba(200,230,255,0.25)
note right of DB: Parse amount\nCompute fee = floor(amount * 0.01)
DB->>Treasury: lock(fee)
DB->>Bridge: bridge(amount - fee) -> target user
end
Bridge-->>User: deliver tokens
alt Success
DB-->>Admin: success
else Failure
DB-->>Admin: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/migrations/erc20-bridge/001-actions.sql (1)
8-15: Avoid duplication; parameterize fee rate.Hard‑coding 1% twice invites drift. Extract a shared constant (e.g., fee_bps) or a config table per network, and compute fee from it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/erc20-bridge/001-actions.sql(1 hunks)
⏰ 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: acceptance-test
🔇 Additional comments (3)
internal/migrations/erc20-bridge/001-actions.sql (3)
8-15: Make sepolia_bridge.lock() + bridge() atomic and idempotentWrap sepolia_bridge.lock(...) and sepolia_bridge.bridge(...) in a single DB transaction with proper exception handling/rollback so either both succeed or neither does; add an idempotency key (e.g., request_id/migration_id) to the bridge operation to prevent double‑spend on retries. File: internal/migrations/erc20-bridge/001-actions.sql (lines 8–15).
24-31: Scope alignment with issue #1173 (fee on deposit and withdraw).This migration only covers admin bridge/withdraw (sepolia_admin_bridge_tokens & mainnet_admin_bridge_tokens in internal/migrations/erc20-bridge/001-actions.sql). A search of internal/migrations found no deposit/mint/issue handlers — confirm deposit paths also apply the 1% fee or document that fees are deferred to admin/withdraw.
8-15: Restrict EXECUTE on admin migration — do not leave it PUBLIC
- internal/migrations/erc20-bridge/001-actions.sql (lines 8–15): function currently exposes sepolia_bridge.lock and sepolia_bridge.bridge to PUBLIC. Revoke PUBLIC and grant EXECUTE only to the admin role (e.g., system:erc20_bridge_writer), or add a server-side role check/SECURITY DEFINER guard.
- Repo search for auth patterns (erc20_bridge_writer / REQUIRES ROLE / has_role / auth) returned no matches — manual confirmation of the repo's role-naming convention and intended grant target required.
Time Submission Status
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/migrations/erc20-bridge/001-actions.sql (2)
25-31: Fee math must be integer-safe; define rounding; validate inputs; cast outputs.Current code multiplies
NUMERIC(78,0)by0.01, producing fractional values and risking type mismatches and over/under‑charging. Use integer math with floor semantics, validateamount > 0, ensurenet >= 0, and cast back toNUMERIC(78,0)for the bridge calls.Apply this diff:
- -- Calculate 1% fee and lock it in our treasury - $numAmount := $amount::NUMERIC(78, 0); - $fee := $numAmount * 0.01; - sepolia_bridge.lock($fee); - - -- Bridge the rest to users - sepolia_bridge.bridge($numAmount - $fee); + -- Calculate 1% fee (floor) and lock it in our treasury + $numAmount := $amount::NUMERIC(78,0); + IF $numAmount <= 0 THEN + ERROR 'amount must be positive'; + END IF; + $fee := floor($numAmount / 100); -- 1% fee, floored + $net := $numAmount - $fee; + IF $net < 0 THEN + ERROR 'fee exceeds amount'; + END IF; + sepolia_bridge.lock($fee::NUMERIC(78,0)); + + -- Bridge the rest to users + sepolia_bridge.bridge($net::NUMERIC(78,0));Follow‑ups:
- Ensure
lock()andbridge()are atomic (same TX) to avoid partial state if either fails.- Add an auth/role guard so only the bridge actor can call this action.
41-47: Replicate integer fee math + validation + casting for mainnet; ensure atomicity and auth.Same issues as sepolia: fractional math, missing input validation, potential type mismatches, and no explicit auth guard/atomicity guarantees.
Apply this diff:
- -- Calculate 1% fee and lock it in our treasury - $numAmount := $amount::NUMERIC(78, 0); - $fee := $numAmount * 0.01; - mainnet_bridge.lock($fee); - - -- Bridge the rest to users - mainnet_bridge.bridge($numAmount - $fee); + -- Calculate 1% fee (floor) and lock it in our treasury + $numAmount := $amount::NUMERIC(78,0); + IF $numAmount <= 0 THEN + ERROR 'amount must be positive'; + END IF; + $fee := floor($numAmount / 100); -- 1% fee, floored + $net := $numAmount - $fee; + IF $net < 0 THEN + ERROR 'fee exceeds amount'; + END IF; + mainnet_bridge.lock($fee::NUMERIC(78,0)); + + -- Bridge the rest to users + mainnet_bridge.bridge($net::NUMERIC(78,0));Also ensure both calls are executed atomically and are restricted to the authorized bridge role.
🧹 Nitpick comments (5)
internal/migrations/erc20-bridge/001-actions.sql (5)
1-16: Bridge info action appears scoped to sepolia only; confirm multi-network intent and cardinality.This returns rows only from
sepolia_bridge.info(). If the intent is to expose info for both sepolia and mainnet, either parameterize the chain or union both sources. Also, the body loops and immediatelyRETURNs a row; ifinfo()can yield multiple rows, this effectively returns just the first row. Adjust the return shape (set-returning) or add a WHERE/limit to make the single-row contract explicit.
18-22: PUBLIC balance view: consider access controls or rate limiting.Making wallet balances publicly queryable can enable address enumeration or scraping. If intentional, ignore; otherwise gate via a role (e.g.,
system:erc20_bridge_reader) or add an auth guard.
34-38: PUBLIC balance view (mainnet): mirror the access-control stance.Same concern as sepolia. If balances should be public, fine; otherwise add a role check or throttle.
24-32: Optional: centralize fee rate and unit semantics.
- Replace the magic “1%” with a basis‑points variable (e.g.,
$fee_bps := 100) so the rate is configurable and consistent across networks.- Confirm
amountis expressed in smallest token units (matchesNUMERIC(78,0)integer semantics) to avoid implicit decimal scaling errors.Also applies to: 40-48
24-32: Amount type: consider NUMERIC input instead of TEXT.If the caller can supply a numeric, prefer
($amount NUMERIC(78,0))and drop the cast to reduce invalid‑cast errors. Keep as TEXT only if required by transport/API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/erc20-bridge/001-actions.sql(1 hunks)
⏰ 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: acceptance-test
🔇 Additional comments (1)
internal/migrations/erc20-bridge/001-actions.sql (1)
24-32: Verify deposit path applies same 1% fee and roundingAdmin action internal/migrations/erc20-bridge/001-actions.sql (lines 24–32) deducts 1% and locks it; confirm the deposit/receive path implements identical fee semantics and numeric rounding. rg search returned no matches — unable to locate a deposit action; verify and update the deposit implementation/migrations if missing.
2 participants
Related Problem
resolves: https://github.com/trufnetwork/truf-network/issues/1173
How Has This Been Tested?
Initial Balance on Layer-1
Bridge 2 tokens, (fee 1%)
Claim tokens on Layer-1 (deducted by fee already)
Fee is still in our treasury
Summary by CodeRabbit