Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReworks order-book settlement fee distribution into a multi-stage flow (lp_share / infra_share), adds bridge-specific unlocks for Data Provider and Validator, updates per-LP zero-loss distribution and audit columns, and increases LP rewards sampling interval default. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/migrations/036-order-book-audit.sql (1)
152-177:⚠️ Potential issue | 🔴 CriticalFix positional field indices in test code; schema change breaks consumers.
The schema adds 2 new fields (total_dp_fees, total_validator_fees) at positions 2–3, shifting lp_count and block_count to positions 4–5. Test code at
tests/streams/order_book/fee_distribution_audit_test.golines 113 and 354 reads row.Values[2] expecting lp_count but now receives total_dp_fees (a NUMERIC type), causing type assertion failures at runtime.Update test indices: row.Values[4] for total_lp_count, row.Values[5] for block_count. Any external API consumer using positional parsing (strict decoder or positional reader) will similarly fail and must be updated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/036-order-book-audit.sql` around lines 152 - 177, The schema change to get_distribution_summary added two NUMERIC fields (total_dp_fees, total_validator_fees) shifting positional columns: update any tests or consumers that read positional columns (e.g., code referencing row.Values[2] expecting lp_count) to use the new indices—use row.Values[4] for total_lp_count and row.Values[5] for block_count—and scan/update any other positional readers/decoders to account for the two inserted columns so type assertions match the renamed field types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 150-171: The audit is recording $infra_share as paid even when the
DP or validator payouts are skipped (missing $dp_addr or `@leader_sender` or
unsupported $bridge); update the payout logic around $dp_addr/$dp_wallet and
`@leader_sender/`$validator_wallet (and calls hoodi_tt2.unlock,
sepolia_bridge.unlock, ethereum_bridge.unlock) to track whether each unlock
actually executed (e.g., set dp_paid and validator_paid flags or accumulate an
actual_paid amount) and then persist the true executed amount (or only mark as
paid when the corresponding unlock ran) instead of always persisting
$infra_share in the audit rows.
- Around line 205-206: The NUMERIC(78,0) casts in the SUM and per-participant
allocation expressions (involving $lp_share, total_percent_numeric,
$block_count) perform rounding and can violate LP conservation; replace those
casts with explicit truncation using TRUNC(...) to force deterministic
truncation toward zero for both the aggregated total calculation (used to set
$total_distributed_base) and the per-participant allocation expressions
(including the adjustment applied to $min_participant_id and the filtering
condition) so that the sum of individual allocations never exceeds $lp_share.
In `@internal/migrations/036-order-book-audit.sql`:
- Around line 53-54: The new monetary columns total_dp_fees and
total_validator_fees lack non-negative constraints; update the migration to
enforce CHECK >= 0 for each column (e.g., add a column-level CHECK or named
CONSTRAINT like total_dp_fees_nonneg and total_validator_fees_nonneg ensuring
total_dp_fees >= 0 and total_validator_fees >= 0) to match the existing
fee/count guardrails in the table definition.
---
Outside diff comments:
In `@internal/migrations/036-order-book-audit.sql`:
- Around line 152-177: The schema change to get_distribution_summary added two
NUMERIC fields (total_dp_fees, total_validator_fees) shifting positional
columns: update any tests or consumers that read positional columns (e.g., code
referencing row.Values[2] expecting lp_count) to use the new indices—use
row.Values[4] for total_lp_count and row.Values[5] for block_count—and
scan/update any other positional readers/decoders to account for the two
inserted columns so type assertions match the renamed field types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4ca9572-040f-4213-91c8-2848a74bad92
📒 Files selected for processing (3)
internal/migrations/033-order-book-settlement.sqlinternal/migrations/036-order-book-audit.sqlinternal/migrations/042-lp-rewards-config.sql
1 participant
resolves: https://github.com/truflation/website/issues/3419
Summary by CodeRabbit
Bug Fixes
New Features
Chores