feat: adjustable fee configs#1160
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin as Admin Caller
participant Action as admin_bridge_tokens(amount)
participant DB as DB: fee_configs
participant Bridge as ERC20 Bridge
Admin->>Action: invoke(amount)
Action->>DB: get_fee_config_by_type('withdraw')
DB-->>Action: {fee_percentage, treasury_address}
alt fee_percentage > 0
Action->>Bridge: lock(fee = amount * fee_percentage)
Bridge-->>Action: locked
Action->>Bridge: unlock(fee) -> treasury_address
Bridge-->>Action: unlocked
end
Action->>Bridge: bridge(amount - fee)
Bridge-->>Action: tx_result
Action-->>Admin: return tx_result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/migrations/023-fee-configs.sql (2)
16-16: Strengthen treasury address validationConsider enforcing hex via regex (Postgres):
treasury_address ~ '^0x[0-9a-fA-F]{40}$'. Current LIKE/LENGTH allows non-hex characters.- CHECK (treasury_address LIKE '0x%' AND LENGTH(treasury_address) = 42) + CHECK (treasury_address ~ '^0x[0-9a-fA-F]{40}$')
19-23: Seed/data notes: updated_at won’t auto-update; confirm @height
- Add an UPDATE trigger to maintain updated_at on row changes.
- Verify that @height is resolved by your migration engine in prod and local.
If using Postgres, I can provide a small trigger function; want me to add it?
internal/migrations/024-erc20-bridge.sql (1)
27-28: Comment typoSays “withdraw” but queries deposit.
- -- Get fee configuration for withdraw + -- Get fee configuration for deposit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/023-fee-configs.sql(1 hunks)internal/migrations/024-erc20-bridge.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
Time Submission Status
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/migrations/023-fee-configs.sql (2)
6-6: Fix precision to allow 1.0000 and keep types consistent.NUMERIC(4,4) tops out at 0.9999 while the CHECK allows <= 1; also align the view return type with the column.
- fee_percentage NUMERIC(4, 4) NOT NULL, + fee_percentage NUMERIC(5, 4) NOT NULL, ... -PUBLIC VIEW RETURNS ( - fee_percentage NUMERIC(4, 4), +PUBLIC VIEW RETURNS ( + fee_percentage NUMERIC(5, 4),Also applies to: 28-29
11-11: Confirm single-config intent vs. multi-config design.Primary key on operation_type enforces one row per op. If that’s the intended model, great; if you plan multiple fee receivers, switch to surrogate id PK + UNIQUE(operation_type, treasury_address) as previously suggested.
internal/migrations/024-erc20-bridge.sql (1)
27-34: Correct comment and truncate fractional fees to integer units; guard remainder.Token amounts are integer-sized; avoid fractional $fee and ensure non-negative bridge amount.
- -- Get fee configuration for deposit - $fee_percentage, $treasury_address := get_fee_config_by_type('withdraw'); - $fee := $num_amount * $fee_percentage; + -- Get fee configuration for withdraw + $fee_percentage, $treasury_address := get_fee_config_by_type('withdraw'); + -- floor/truncate to integer units + $fee := FLOOR($num_amount * $fee_percentage)::NUMERIC(78, 0); sepolia_bridge.lock($fee); sepolia_bridge.unlock($treasury_address, $fee); -- Bridge the rest to users - sepolia_bridge.bridge($num_amount - $fee); + $bridge_amount := $num_amount - $fee; + IF $bridge_amount < 0 { ERROR 'Bridge amount negative'; } + sepolia_bridge.bridge($bridge_amount);
🧹 Nitpick comments (3)
internal/migrations/023-fee-configs.sql (1)
13-16: Strengthen temporal integrity on timestamps.Ensure updated_at is not earlier than created_at.
-- Constraints CHECK (operation_type IN ('deposit', 'withdraw')), CHECK (fee_percentage >= 0 AND fee_percentage <= 1), -- 0% to 100% - CHECK (treasury_address LIKE '0x%' AND LENGTH(treasury_address) = 42) + CHECK (treasury_address LIKE '0x%' AND LENGTH(treasury_address) = 42), + CHECK (updated_at >= created_at)internal/migrations/024-erc20-bridge.sql (2)
44-54: Unify var naming, fix comment, and truncate fee; mirror sepolia guard.Keep naming consistent and prevent fractional/negative amounts.
- $numAmount := $amount::NUMERIC(78, 0); + $num_amount := $amount::NUMERIC(78, 0); - -- Get fee configuration for deposit - $fee_percentage, $treasury_address := get_fee_config_by_type('withdraw'); - $fee := $numAmount * $fee_percentage; + -- Get fee configuration for withdraw + $fee_percentage, $treasury_address := get_fee_config_by_type('withdraw'); + $fee := FLOOR($num_amount * $fee_percentage)::NUMERIC(78, 0); mainnet_bridge.lock($fee); - mainnet_bridge.unlock($treasury_address, $fee); + mainnet_bridge.unlock($treasury_address, $fee); -- Bridge the rest to users - mainnet_bridge.bridge($numAmount - $fee); + $bridge_amount := $num_amount - $fee; + IF $bridge_amount < 0 { ERROR 'Bridge amount negative'; } + mainnet_bridge.bridge($bridge_amount);
29-31: Optionally skip zero-value lock/unlock for cleanliness.No behavior change, but avoids no-op calls when $fee = 0.
- sepolia_bridge.lock($fee); - sepolia_bridge.unlock($treasury_address, $fee); + IF $fee > 0 { + sepolia_bridge.lock($fee); + sepolia_bridge.unlock($treasury_address, $fee); + }And similarly for mainnet:
- mainnet_bridge.lock($fee); - mainnet_bridge.unlock($treasury_address, $fee); + IF $fee > 0 { + mainnet_bridge.lock($fee); + mainnet_bridge.unlock($treasury_address, $fee); + }Also applies to: 49-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/023-fee-configs.sql(1 hunks)internal/migrations/024-erc20-bridge.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 (1)
internal/migrations/024-erc20-bridge.sql (1)
27-29: Verify operation_type choice (‘withdraw’ vs ‘deposit’).The actions are named “admin_bridge_tokens” (sounds like deposit path) but fetch fees for 'withdraw'. If intentional, ignore; otherwise switch to 'deposit'.
Also applies to: 46-48
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/migrations/024-erc20-bridge.sql (3)
27-38: Guardrail: cap Σ(fee_percentage) <= 100% before processing.Add a pre-check to abort if misconfigured fees exceed 100%. Repeat for both networks.
+ -- Guardrail: total configured withdraw fee must not exceed 100% + $sum_pct := COALESCE((SELECT SUM(fee_percentage) FROM fee_configs WHERE operation_type = 'withdraw'), 0); + IF $sum_pct > 1 { ERROR 'Sum of withdraw fees exceeds 100%'; }Place this before iterating configs.
Also applies to: 50-61
50-61: Mirror sepolia fixes in mainnet block (accumulate, floor, single bridge, 0-config guard).Ensure identical behavior and safety on mainnet.
- -- Get fee configuration for withdraw - FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at LIMIT 1 { - $fee := $num_amount * $config.fee_percentage; - - IF $fee > 0 { - mainnet_bridge.lock($fee); - mainnet_bridge.unlock($config.treasury_address, $fee); - } - - -- Bridge the rest to users - mainnet_bridge.bridge($num_amount - $fee); - } + -- Get fee configuration(s) for withdraw and accumulate fees + $total_fee := 0::NUMERIC(78,0); + $configs := 0::INT8; + FOR $config IN SELECT fee_percentage, treasury_address FROM get_fee_config_by_type('withdraw') { + $fee := FLOOR($num_amount * $config.fee_percentage)::NUMERIC(78,0); + IF $fee > 0 { + mainnet_bridge.lock($fee); + mainnet_bridge.unlock($config.treasury_address, $fee); + $total_fee := $total_fee + $fee; + } + $configs := $configs + 1; + } + IF $configs = 0 { ERROR 'No fee_configs rows for operation_type=withdraw'; } + IF $total_fee > $num_amount { ERROR 'Total fee exceeds amount'; } + $bridge_amount := $num_amount - $total_fee; + mainnet_bridge.bridge($bridge_amount);
27-38: Fix: handle 0-config case, integer rounding, single bridge, and guard against over‑fee.Current logic silently does nothing if no config rows exist, may pass fractional amounts to lock/unlock, and bridges inside the loop. Bridge once after accumulating integer-rounded fees, error on misconfiguration, and prefer the provided view to encapsulate selection.
Apply:
- -- Get fee configuration for withdraw - FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at LIMIT 1 { - $fee := $num_amount * $config.fee_percentage; - - IF $fee > 0 { - sepolia_bridge.lock($fee); - sepolia_bridge.unlock($config.treasury_address, $fee); - } - - -- Bridge the rest to users - sepolia_bridge.bridge($num_amount - $fee); - } + -- Get fee configuration(s) for withdraw and accumulate fees + $total_fee := 0::NUMERIC(78,0); + $configs := 0::INT8; + FOR $config IN SELECT fee_percentage, treasury_address FROM get_fee_config_by_type('withdraw') { + $fee := FLOOR($num_amount * $config.fee_percentage)::NUMERIC(78,0); + IF $fee > 0 { + sepolia_bridge.lock($fee); + sepolia_bridge.unlock($config.treasury_address, $fee); + $total_fee := $total_fee + $fee; + } + $configs := $configs + 1; + } + IF $configs = 0 { ERROR 'No fee_configs rows for operation_type=withdraw'; } + IF $total_fee > $num_amount { ERROR 'Total fee exceeds amount'; } + $bridge_amount := $num_amount - $total_fee; + sepolia_bridge.bridge($bridge_amount);
🧹 Nitpick comments (1)
internal/migrations/024-erc20-bridge.sql (1)
28-29: If you keep selecting from fee_configs directly: choose the latest row.Using ORDER BY created_at without DESC picks the oldest, not the current config.
-FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at LIMIT 1 { +FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 {Note: Prefer the get_fee_config_by_type view for encapsulation.
Also applies to: 51-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/024-erc20-bridge.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). (2)
- GitHub Check: lint
- GitHub Check: acceptance-test
🔇 Additional comments (1)
internal/migrations/024-erc20-bridge.sql (1)
27-38: get_fee_config_by_type present; FLOOR() not used — resolved
- internal/migrations/023-fee-configs.sql defines CREATE OR REPLACE ACTION get_fee_config_by_type($operation_type TEXT) and exposes fee_percentage (NUMERIC(4,4)).
- No runtime use of FLOOR() found in migrations (only a commented formula in internal/migrations/020-digest-actions.sql); no TRUNC() occurrences. No change required in internal/migrations/024-erc20-bridge.sql.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/migrations/024-erc20-bridge.sql (1)
48-61: Mainnet parity: truncation, guards, and 0‑row handling.Align logic with sepolia to prevent fractional amounts and silent no‑ops.
- $num_amount := $amount::NUMERIC(78, 0); - - -- Get fee configuration for withdraw - FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at LIMIT 1 { - $fee := $num_amount * $config.fee_percentage; - - IF $fee > 0::NUMERIC(78, 0) { - mainnet_bridge.lock($fee); - mainnet_bridge.unlock($config.treasury_address, $fee); - } - - -- Bridge the rest to users - mainnet_bridge.bridge($num_amount - $fee); - } + $num_amount := $amount::NUMERIC(78, 0); + $found := 0::INT8; + IF $num_amount < 0::NUMERIC(78,0) { ERROR 'Amount must be non-negative'; } + + -- Get fee configuration for withdraw + FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 { + $fee := FLOOR($num_amount * $config.fee_percentage)::NUMERIC(78,0); + $bridge_amount := $num_amount - $fee; + IF $bridge_amount < 0::NUMERIC(78,0) { ERROR 'Bridge amount negative'; } + + IF $fee > 0::NUMERIC(78, 0) { + mainnet_bridge.lock($fee); + mainnet_bridge.unlock($config.treasury_address, $fee); + } + -- Bridge the rest to users + mainnet_bridge.bridge($bridge_amount); + $found := 1; + } + IF $found = 0 { ERROR 'No fee_configs rows for operation_type=withdraw'; }
🧹 Nitpick comments (1)
internal/migrations/024-erc20-bridge.sql (1)
28-38: Optional: avoid FOR when LIMIT 1; select into locals and bridge once.Cleaner and future‑proof; reduces chances of accidental per‑row bridging if LIMIT is changed.
Also applies to: 51-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/024-erc20-bridge.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 (1)
internal/migrations/024-erc20-bridge.sql (1)
28-38: Resolved — fee_configs enforces a single row per operation_type. internal/migrations/023-fee-configs.sql declares PRIMARY KEY (operation_type) and CHECK(fee_percentage >= 0 AND fee_percentage <= 1), so the LIMIT 1 is redundant and no accumulation logic is required; optionally use get_fee_config_by_type for consistency.
| INSERT INTO fee_configs (operation_type, fee_percentage, treasury_address, created_at, updated_at) VALUES | ||
| ('deposit', 0.0100, '0xDe5B2aBce299eBdC3567895B1B4b02Ca2c33C94A', @height, @height), | ||
| ('withdraw', 0.0100, '0xDe5B2aBce299eBdC3567895B1B4b02Ca2c33C94A', @height, @height) | ||
| ON CONFLICT (operation_type) DO NOTHING; |
There was a problem hiding this comment.
I think it is better for insert statement to be in separate bootstrap migration file like internal/migrations/015-system-roles-bootstrap.sql.
But I think it will be better if it is not included in the regular migration file as it is specific to address 0xDe5B2aBce299eBdC3567895B1B4b02Ca2c33C94A like scripts/migrate.sh#41-51 does. This way, in the future (another PR I guess), we can test it with trufnetwork/kwil-db#1599. The script run using Taskfile.yml#317-328
cc @outerlook
There was a problem hiding this comment.
got it, will separate or remove
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
internal/migrations/024-erc20-bridge.sql (2)
24-43: Truncate fee to integer units, guard negatives, and bridge once with validated remainder (sepolia)Fee math can produce fractional NUMERIC; remainder may go negative if fee > amount. Add truncation and guards.
CREATE OR REPLACE ACTION sepolia_admin_bridge_tokens($amount TEXT) PUBLIC { $num_amount := $amount::NUMERIC(78, 0); - + IF $num_amount < 0::NUMERIC(78,0) { ERROR('Amount must be non-negative'); } -- Get fee configuration for withdraw - $found := 0 + $found := 0; FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 { - $fee := $num_amount * $config.fee_percentage; + $fee := FLOOR($num_amount * $config.fee_percentage)::NUMERIC(78,0); + $bridge_amount := $num_amount - $fee; + IF $bridge_amount < 0::NUMERIC(78,0) { ERROR('Bridge amount negative'); } - IF $fee > 0::NUMERIC(78, 0) { + IF $fee > 0::NUMERIC(78, 0) AND $config.treasury_address IS NOT NULL { sepolia_bridge.lock($fee); sepolia_bridge.unlock($config.treasury_address, $fee); } -- Bridge the rest to users - sepolia_bridge.bridge($num_amount - $fee); + sepolia_bridge.bridge($bridge_amount); $found := 1; } IF $found = 0 { ERROR('No fee_configs rows for operation_type=withdraw'); } };
51-70: Mirror the math/guards for mainnet action-CREATE OR REPLACE ACTION mainnet_admin_bridge_tokens($amount TEXT) PUBLIC { +CREATE OR REPLACE ACTION mainnet_admin_bridge_tokens($amount TEXT) PUBLIC { $num_amount := $amount::NUMERIC(78, 0); - + IF $num_amount < 0::NUMERIC(78,0) { ERROR('Amount must be non-negative'); } -- Get fee configuration for withdraw - $found := 0 + $found := 0; FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 { - $fee := $num_amount * $config.fee_percentage; + $fee := FLOOR($num_amount * $config.fee_percentage)::NUMERIC(78,0); + $bridge_amount := $num_amount - $fee; + IF $bridge_amount < 0::NUMERIC(78,0) { ERROR('Bridge amount negative'); } - IF $fee > 0::NUMERIC(78, 0) { + IF $fee > 0::NUMERIC(78, 0) AND $config.treasury_address IS NOT NULL { mainnet_bridge.lock($fee); mainnet_bridge.unlock($config.treasury_address, $fee); } -- Bridge the rest to users - mainnet_bridge.bridge($num_amount - $fee); + mainnet_bridge.bridge($bridge_amount); $found := 1; } IF $found = 0 { ERROR('No fee_configs rows for operation_type=withdraw'); } };
🧹 Nitpick comments (4)
internal/migrations/024-erc20-bridge.sql (4)
33-35: Null/empty treasury safetyValidate treasury_address before unlock to avoid failing transfers or misrouting.
- IF $fee > 0::NUMERIC(78, 0) AND $config.treasury_address IS NOT NULL { + IF $fee > 0::NUMERIC(78, 0) AND $config.treasury_address IS NOT NULL /* AND LENGTH($config.treasury_address) > 0 */ {If LENGTH is unavailable, keep the NULL check and ensure non-empty at insert time via a CHECK constraint.
Also applies to: 60-62
1-16: Naming/scope: clarify which network get_erc20_bridge_info exposesThis view returns sepolia_bridge.info() but name suggests generic. Consider rename to sepolia_erc20_bridge_info or add a network parameter.
29-29: Indexing for config lookupAdd an index on fee_configs(operation_type, created_at DESC) to support ORDER BY ... LIMIT 1.
Example:
- CREATE INDEX IF NOT EXISTS idx_fee_configs_op_created ON fee_configs(operation_type, created_at DESC);
Also applies to: 56-56
24-43: Atomicity and idempotency considerationsEnsure lock → unlock → bridge runs atomically (single transaction) and is safe to retry (idempotent) to avoid partial transfers on failures.
Also applies to: 51-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/migrations/024-erc20-bridge.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
|
I can't test this rn, probably because of the pruning, the transaction keeps pending |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
internal/migrations/023-fee-configs.sql (4)
6-7: Allow 100% exactly; widen precision and keep CHECK.NUMERIC(4,4) can’t store 1.0000 though CHECK allows <= 1. Bump to NUMERIC(5,4) in both table and action return type.
Apply:
- fee_percentage NUMERIC(4, 4) NOT NULL, + fee_percentage NUMERIC(5, 4) NOT NULL, @@ -PUBLIC VIEW RETURNS ( - fee_percentage NUMERIC(4, 4), +PUBLIC VIEW RETURNS ( + fee_percentage NUMERIC(5, 4),Also applies to: 22-22, 15-15
11-11: Support multiple configs; replace PK with surrogate id + UNIQUE.Code in 024 iterates/filters by operation and created_at; PK on operation_type forbids multiple rows. Use id PK and UNIQUE(operation_type, treasury_address).
Apply:
-CREATE TABLE IF NOT EXISTS fee_configs ( - operation_type TEXT NOT NULL, +CREATE TABLE IF NOT EXISTS fee_configs ( + id BIGSERIAL PRIMARY KEY, + operation_type TEXT NOT NULL, @@ - PRIMARY KEY (operation_type), + UNIQUE (operation_type, treasury_address),
19-28: Guard “not found” in getter action.Return an explicit error when no row exists for the requested type.
Apply:
CREATE OR REPLACE ACTION get_fee_config_by_type($operation_type TEXT) PUBLIC VIEW RETURNS ( @@ ) { - FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = $operation_type { + IF NOT EXISTS (SELECT 1 FROM fee_configs WHERE operation_type = $operation_type) { + ERROR 'No fee_configs row for operation_type=' || $operation_type; + } + FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = $operation_type { RETURN $config.fee_percentage, $config.treasury_address; } };
17-17: Index for latest-config lookup.024 queries ORDER BY created_at DESC LIMIT 1; add a composite index.
Apply:
); +-- Optimize: latest-by-operation lookup +CREATE INDEX IF NOT EXISTS idx_fee_configs_op_created_at +ON fee_configs (operation_type, created_at DESC);internal/migrations/024-erc20-bridge.sql (4)
29-31: Performance: rely on index in 023 for ORDER BY created_at DESC LIMIT 1.Ensure idx_fee_configs_op_created_at exists; otherwise these lookups will scan.
Run to confirm index is present:
#!/bin/bash rg -nP "CREATE\s+INDEX[^\n]+idx_fee_configs_op_created_at" -C1Also applies to: 56-58
24-24: Admin actions should not be PUBLIC; add auth guard or restrict EXECUTE.As PUBLIC, anyone can trigger admin bridging. Add an explicit role check now; adjust privileges in a follow-up migration.
Apply (inline guard):
-CREATE OR REPLACE ACTION sepolia_admin_bridge_tokens($amount TEXT) PUBLIC { +CREATE OR REPLACE ACTION sepolia_admin_bridge_tokens($amount TEXT) PUBLIC { + IF NOT has_role('bridge_admin') { ERROR 'unauthorized'; }-CREATE OR REPLACE ACTION mainnet_admin_bridge_tokens($amount TEXT) PUBLIC { +CREATE OR REPLACE ACTION mainnet_admin_bridge_tokens($amount TEXT) PUBLIC { + IF NOT has_role('bridge_admin') { ERROR 'unauthorized'; }Optionally also drop PUBLIC and manage GRANTs in a separate migration.
Also applies to: 51-51
24-43: Fees must be integer-truncated; add non-negative guard and bridge once per calc.Current code multiplies NUMERIC(78,0) by a fraction, risking fractional amounts and negative remainders. Truncate fee, guard amount >= 0, compute remainder explicitly.
Apply:
CREATE OR REPLACE ACTION sepolia_admin_bridge_tokens($amount TEXT) PUBLIC { $num_amount := $amount::NUMERIC(78, 0); - + IF $num_amount < 0::NUMERIC(78,0) { ERROR 'Amount must be non-negative'; } @@ - FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 { - $fee := $num_amount * $config.fee_percentage; + FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 { + $fee := FLOOR($num_amount * $config.fee_percentage)::NUMERIC(78,0); + $bridge_amount := $num_amount - $fee; + IF $bridge_amount < 0::NUMERIC(78,0) { ERROR 'Bridge amount negative'; } - IF $fee > 0::NUMERIC(78, 0) { + IF $fee > 0::NUMERIC(78, 0) { sepolia_bridge.lock($fee); sepolia_bridge.unlock($config.treasury_address, $fee); } - -- Bridge the rest to users - sepolia_bridge.bridge($num_amount - $fee); + -- Bridge the rest to users + sepolia_bridge.bridge($bridge_amount); $found := 1; }
51-70: Mirror fee truncation and guards for mainnet.Same integer/negative issues as testnet block.
Apply:
CREATE OR REPLACE ACTION mainnet_admin_bridge_tokens($amount TEXT) PUBLIC { $num_amount := $amount::NUMERIC(78, 0); - + IF $num_amount < 0::NUMERIC(78,0) { ERROR 'Amount must be non-negative'; } @@ - FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 { - $fee := $num_amount * $config.fee_percentage; + FOR $config IN SELECT fee_percentage, treasury_address FROM fee_configs WHERE operation_type = 'withdraw' ORDER BY created_at DESC LIMIT 1 { + $fee := FLOOR($num_amount * $config.fee_percentage)::NUMERIC(78,0); + $bridge_amount := $num_amount - $fee; + IF $bridge_amount < 0::NUMERIC(78,0) { ERROR 'Bridge amount negative'; } IF $fee > 0::NUMERIC(78, 0) { mainnet_bridge.lock($fee); mainnet_bridge.unlock($config.treasury_address, $fee); } - -- Bridge the rest to users - mainnet_bridge.bridge($num_amount - $fee); + -- Bridge the rest to users + mainnet_bridge.bridge($bridge_amount); $found := 1; }
🧹 Nitpick comments (2)
internal/migrations/023-fee-configs.sql (1)
16-16: Stricter address validation (optional).If the dialect supports regex, ensure hex-only after 0x; otherwise keep as-is.
Apply (if regex available):
- CHECK (treasury_address LIKE '0x%' AND LENGTH(treasury_address) = 42) + CHECK (treasury_address ~ '^0x[0-9a-fA-F]{40}$')internal/migrations/024-erc20-bridge.sql (1)
1-16: Bridge info only returns Sepolia; clarify or parameterize.Either rename to get_sepolia_bridge_info or accept a network arg and branch.
Apply (rename option):
-CREATE OR REPLACE ACTION get_erc20_bridge_info() +CREATE OR REPLACE ACTION get_sepolia_bridge_info()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/023-fee-configs.sql(1 hunks)internal/migrations/024-erc20-bridge.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
@outerlook this might be related? trufnetwork/kwil-db#1606 |
Might be related. But after resetting, the network is normal again. If it's tried again, we can verify this |
|
Also, the pruning should not prevent any transactions on our mainnet, especially if targetting the gateway/node1/node2 as it's very lightweight for them |
|
@outerlook idk if this is related but previously when I directly multiplying (amount * 0.01) it works well, then now I multiply by config with NUMERIC(4, 4), is it possible that it's the source of error? amount is NUMERIC(78, 0). |
|
this PR is cancelled, will be continued in this Goal: |
3 participants
Related Problem
resolves: https://github.com/trufnetwork/truf-network/issues/1173
How Has This Been Tested?
Summary by CodeRabbit
New Features
Refactor