fix: unlock fee to treasury#1159
Conversation
WalkthroughUpdates ERC20 bridge actions in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin Action
participant Sepolia as sepolia_bridge
participant Treasury as Treasury Address
note over Admin: sepolia_admin_bridge_tokens
Admin->>Admin: Compute $fee = 1% of $num_amount
Admin->>Sepolia: lock($fee)
Sepolia-->>Admin: fee locked
Admin->>Sepolia: unlock($treasury_address, $fee)
Sepolia-->>Treasury: transfer fee
Admin->>Sepolia: bridge($num_amount - $fee)
Sepolia-->>Admin: bridge initiated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
Bug Report Checklist
|
|
@pr-time-tracker bug commit 19c6edd && bug author @williamrusdyputra |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/migrations/erc20-bridge/001-actions.sql (3)
30-35: Confirm side‑effect ordering and atomicity (lock → unlock → bridge).If
unlocksucceeds butbridgefails, you’ve diverted the fee yet not bridged funds. Ensure this action is atomic or add compensations/idempotency.
49-52: Fix chain reference in TODO and prep mainnet parity.Commented example uses
sepolia_bridge.unlock; should bemainnet_bridge.unlock. Also plan the same integer-fee computation and avoid merging TODOs without a feature flag.Apply this diff:
- -- sepolia_bridge.unlock($treasury_address, $fee); + -- mainnet_bridge.unlock($treasury_address, $fee);
26-35: Naming/style consistency: prefer one convention across actions.
$num_amount(snake_case) on Sepolia vs$numAmount(camelCase) on Mainnet is inconsistent. Pick one for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/erc20-bridge/001-actions.sql(2 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 (2)
internal/migrations/erc20-bridge/001-actions.sql (2)
34-34: Type consistency between$num_amount - $feeand bridge input.After switching to integer fee, confirm
sepolia_bridge.bridge(...)expects an integer amount and rejects zero/negative values; add guards if necessary.
30-31: Don't hardcode the treasury address; make it configurable and validate.Hardcoding 0xDe5B2aBce299eBdC3567895B1B4b02Ca2c33C94A in a migration bakes env-specific state. Read the treasury address from config/state (settings table or a constants action) and validate EIP‑55 checksum/format before calling unlock(address, amount). Search shows only this occurrence and a commented TODO; no central treasury config or a
CREATE OR REPLACE ACTION ... unlock(...)definition was found — confirm the action signature or add a vetted wrapper and update the migration.Location: internal/migrations/erc20-bridge/001-actions.sql:30-31
| $num_amount := $amount::NUMERIC(78, 0); | ||
| $fee := $num_amount * 0.01; | ||
| sepolia_bridge.lock($fee); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Use integer math for 1% fee to avoid scale/rounding/type issues.
$num_amount is NUMERIC(78,0). Multiplying by 0.01 risks producing a scaled decimal that may not match downstream expectations and can silently round. Compute the fee via integer math instead.
Apply this diff:
- $fee := $num_amount * 0.01;
+ -- 1% fee using integer arithmetic (floor)
+ $fee := FLOOR($num_amount / 100);Also confirm policy (floor vs round/ceil) and add the same change to mainnet when enabling treasury there.
Use integer math for 1% fee to avoid scale/rounding/type issues.
Multiplying NUMERIC(78,0) by 0.01 creates scaled decimals and can silently round — compute the fee with integer arithmetic (floor) instead.
- $fee := $num_amount * 0.01;
+ -- 1% fee using integer arithmetic (floor)
+ $fee := FLOOR($num_amount / 100);Confirm policy (floor vs round/ceil) and apply the same change to mainnet when enabling the treasury.
📝 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.
| $num_amount := $amount::NUMERIC(78, 0); | |
| $fee := $num_amount * 0.01; | |
| sepolia_bridge.lock($fee); | |
| $num_amount := $amount::NUMERIC(78, 0); | |
| -- 1% fee using integer arithmetic (floor) | |
| $fee := FLOOR($num_amount / 100); | |
| sepolia_bridge.lock($fee); |
🤖 Prompt for AI Agents
internal/migrations/erc20-bridge/001-actions.sql around lines 26 to 28: the
current fee calculation multiplies NUMERIC(78,0) by 0.01 which creates scaled
decimals and risks rounding; replace it with integer arithmetic that computes
the 1% fee using floor semantics (e.g., compute fee as integer division by 100
so fee := floor($num_amount * 1 / 100) or equivalently $num_amount / 100), keep
the rest of the call to sepolia_bridge.lock($fee) unchanged, and apply the same
integer-floor logic to the mainnet/treasury migration when enabling it.
Time Submission Status
|
1 participant
Related Problem
related: https://github.com/trufnetwork/truf-network/issues/1173
How Has This Been Tested?
Summary by CodeRabbit
New Features
Chores
Documentation