Open
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two tn_utils precompiles ( Changes
Sequence Diagram(s)sequenceDiagram
participant Migration as Settlement Migration
participant Precompile as tn_utils Precompile
participant Validators as Validators / VoteStore
participant Bridge as Bridge Service
participant DB as Database
Migration->>Precompile: call get_validators()
Precompile->>Validators: query active validators, filter power>0
Precompile->>Precompile: sort by pubkey bytes, map to wallet_hex/wallet_bytes
Precompile-->>Migration: return ordered validator list
Migration->>Migration: compute per_validator = infra_share / n, remainder -> first
loop per validator
Migration->>Bridge: unlock per_validator funds for validator.wallet
Migration->>DB: ensure ob_participants row exists for validator
Migration->>DB: insert/update ob_fee_distribution_details and ob_net_impacts
Migration->>Migration: accumulate actual_validator_fees
end
Migration->>DB: update ob_fee_distributions with final totals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
tests/streams/order_book/fee_distribution_test.go (1)
1170-1171: Consider using a fee amount that produces a non-zero remainder.With
totalFees = 30 TRUF, theinfraShare = 3.75 TRUFdivides evenly by 3 validators (1.25 TRUF each), resulting in zero remainder. The remainder-handling logic (lines 1237-1250) won't be exercised.Consider using a fee like
31 TRUFwhich givesinfraShare = 3.875 TRUF, producing a non-trivial remainder when divided by 3.💡 Suggested change to test remainder handling
- totalFees := new(big.Int).Mul(big.NewInt(30), big.NewInt(1e18)) + // Use 31 TRUF to ensure non-zero remainder when splitting among 3 validators + // infraShare = 31 * 0.125 = 3.875 TRUF; 3.875 / 3 = 1.291... with remainder + totalFees := new(big.Int).Mul(big.NewInt(31), big.NewInt(1e18))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_test.go` around lines 1170 - 1171, The test sets totalFees to 30 TRUF which yields an infraShare that divides evenly across validators and never exercises the remainder branch; update the totalFees initializer (variable totalFees) to use a value like 31 TRUF (e.g., big.NewInt(31) * 1e18) so infraShare (infraShare) produces a non-zero remainder and the remainder-handling logic is executed during the test.internal/migrations/033-order-book-settlement.sql (1)
118-128: Double iteration over validators for counting and distribution.The code iterates over
tn_utils.get_validators()twice: once to count validators (lines 119-121) and again to distribute (line 128). While this works correctly because the result is deterministic within a transaction, it's slightly inefficient.This is likely a Kuneiform limitation (no way to store the result set or count during iteration). The current approach is acceptable given the constraints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/033-order-book-settlement.sql` around lines 118 - 128, The migration currently iterates tn_utils.get_validators() twice—once to compute $validator_count and again to distribute $infra_share—which is inefficient; update the logic in the block around $validator_count, $per_validator, $val_remainder and the subsequent for loop so you only enumerate validators once: either (a) if Kuneiform supports materializing the iterator, collect tn_utils.get_validators() into a single in-memory list/array and derive count and distribution from that list before distributing, or (b) add/use a helper like tn_utils.get_validator_count() (or an equivalent COUNT query) to compute $validator_count without a first iteration, then run the single distribution loop over tn_utils.get_validators(); adjust references to $validator_count, $per_validator, $val_remainder and $first_validator accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 118-128: The migration currently iterates
tn_utils.get_validators() twice—once to compute $validator_count and again to
distribute $infra_share—which is inefficient; update the logic in the block
around $validator_count, $per_validator, $val_remainder and the subsequent for
loop so you only enumerate validators once: either (a) if Kuneiform supports
materializing the iterator, collect tn_utils.get_validators() into a single
in-memory list/array and derive count and distribution from that list before
distributing, or (b) add/use a helper like tn_utils.get_validator_count() (or an
equivalent COUNT query) to compute $validator_count without a first iteration,
then run the single distribution loop over tn_utils.get_validators(); adjust
references to $validator_count, $per_validator, $val_remainder and
$first_validator accordingly.
In `@tests/streams/order_book/fee_distribution_test.go`:
- Around line 1170-1171: The test sets totalFees to 30 TRUF which yields an
infraShare that divides evenly across validators and never exercises the
remainder branch; update the totalFees initializer (variable totalFees) to use a
value like 31 TRUF (e.g., big.NewInt(31) * 1e18) so infraShare (infraShare)
produces a non-zero remainder and the remainder-handling logic is executed
during the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e60e99a8-1235-49e9-b455-85f5e379d4cb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
extensions/tn_utils/precompiles.gogo.modinternal/migrations/033-order-book-settlement.sqltests/streams/order_book/fee_distribution_audit_test.gotests/streams/order_book/fee_distribution_test.gotests/streams/order_book/pnl_impact_test.gotests/streams/order_book/test_helpers_orderbook.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/streams/order_book/fee_distribution_test.go (2)
1115-1118: Consider adding a test assertion for remainder recipient determinism.The test description mentions "remainder going to the first (sorted by pubkey)" but the assertions only verify
diff <= 1without confirming which validator receives the remainder. If the specification requires deterministic remainder assignment (sorted by pubkey), consider:
- Sorting the injected validators by pubkey
- Asserting the first validator (sorted) receives more than the others
This would validate the deterministic ordering behavior described in the function comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_test.go` around lines 1115 - 1118, The test testDistributionMultipleValidators currently only checks fee differences <= 1; update it to also assert deterministic remainder recipient by sorting the injected validators by their public key (use the same pubkey field used in setup) and then assert that the first validator in that sorted order receives the extra remainder (i.e., its received fee > each other validator's fee, or that it equals max and at least one other is less), thereby verifying the comment's "remainder going to the first (sorted by pubkey)" behavior.
1231-1250: Remainder handling assertions may be brittle depending on implementation.The test asserts both
diff <= 1(line 1245) anddiff > 0(line 1249). With 31 TRUF and 3 validators:
infraShare = 3,875,000,000,000,000,000wei- Per-validator =
1,291,666,666,666,666,666.67...The actual
diffdepends on how the SQL implementation handles division:
- Truncation + single-recipient remainder: remainder = 2 wei →
diff = 2(fails <= 1 assertion)- Rounding + negative correction: first validator gets 1 less →
diff = 1(passes)- Truncation + spread remainder 1 wei each: two validators get +1 each →
diff = 1(passes)Consider adding a comment documenting the expected implementation behavior, or relaxing the
<= 1constraint to<= 2if the implementation gives all remainder to one validator.📋 Suggestion: Add implementation documentation or relax constraint
// Payouts should differ by at most 1 wei (remainder handling) // PostgreSQL NUMERIC(78,0) division rounds, so the first validator // gets per_validator + remainder (which may be negative, i.e., 1 wei less) + // NOTE: With infraShare=3875000000000000000 / 3, PostgreSQL banker's rounding + // gives per_validator=1291666666666666667, total=3875000000000000001, + // so remainder=-1 is applied to the first validator. vals := []*big.Int{inc1, inc2, inc3}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_test.go` around lines 1231 - 1250, The two assertions around validator payout differences (variables vals, minVal, maxVal, diff and the require.True checks) are brittle given varying SQL division/remainder strategies; update the test to allow a maximum difference of 2 wei (change the first require.True from diff ≤ 1 to diff ≤ 2) and adjust the second assertion to assert diff ≥ 1 only when the test is meant to guarantee a non-zero remainder, or remove the strict diff.Sign() > 0 check and instead add a clarifying comment describing that SQL implementations may assign remainder up to 2 wei to a single validator; ensure the change updates the message text to reflect the new bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/tn_utils/precompiles.go`:
- Around line 258-271: The current Handler counts all validators with v.Power >
0 directly from app.Validators.GetValidators(), but get_validators() (the
exported precompile function) applies additional filtering (skipping malformed
secp256k1 keys); change the Handler to mirror that exact filtering by either
calling the same get_validators() helper and counting its returned slice or by
applying the same malformed-key validation used in get_validators() when
iterating validators from app.Validators.GetValidators(), and return that
filtered count instead of counting every v.Power > 0.
- Around line 203-211: The code currently calls app.Validators.GetValidators()
and then mutates the returned slice in place via sort.Slice (variable
validators), which can mutate external state; fix by making a shallow copy of
the slice returned by app.Validators.GetValidators() (e.g., validatorsCopy :=
make([]<elementType>, len(validators)); copy(validatorsCopy, validators)) and
then run sort.Slice on validatorsCopy instead of validators so the VIEW
precompile does not produce side effects.
In `@tests/streams/order_book/fee_distribution_test.go`:
- Around line 1128-1131: The test's remainder assumption is wrong: when
distributing 31 TRUF among 3 validators the remainder can be 2 wei, so update
the assertion in fee_distribution_test (around the code using
injectTestValidator and the diff comparison) to allow a difference of up to 2
wei (change big.NewInt(1) to big.NewInt(2)) or alternatively choose a test
amount divisible by 3 to keep remainder ≤1 wei; also update/ remove the
misleading comment about PostgreSQL NUMERIC rounding and ensure the check still
compares balances from the validator payout logic that uses
app.Validators/get_validators.
---
Nitpick comments:
In `@tests/streams/order_book/fee_distribution_test.go`:
- Around line 1115-1118: The test testDistributionMultipleValidators currently
only checks fee differences <= 1; update it to also assert deterministic
remainder recipient by sorting the injected validators by their public key (use
the same pubkey field used in setup) and then assert that the first validator in
that sorted order receives the extra remainder (i.e., its received fee > each
other validator's fee, or that it equals max and at least one other is less),
thereby verifying the comment's "remainder going to the first (sorted by
pubkey)" behavior.
- Around line 1231-1250: The two assertions around validator payout differences
(variables vals, minVal, maxVal, diff and the require.True checks) are brittle
given varying SQL division/remainder strategies; update the test to allow a
maximum difference of 2 wei (change the first require.True from diff ≤ 1 to diff
≤ 2) and adjust the second assertion to assert diff ≥ 1 only when the test is
meant to guarantee a non-zero remainder, or remove the strict diff.Sign() > 0
check and instead add a clarifying comment describing that SQL implementations
may assign remainder up to 2 wei to a single validator; ensure the change
updates the message text to reflect the new bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68b2f492-3f60-4f26-982e-70b3beb247d6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
extensions/tn_utils/precompiles.gogo.modinternal/migrations/033-order-book-settlement.sqltests/streams/order_book/fee_distribution_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- internal/migrations/033-order-book-settlement.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/streams/order_book/fee_distribution_audit_test.go (1)
519-534: Defensive code path that may be unreachable with current test setup.The fallback logic (Lines 524-532) handles the case where User1 is also the validator/leader. However, since
injectTestValidatoralways creates a fresh keypair (see context snippet 1), User1 will never be the validator in this test. This code path may never execute.Consider either:
- Removing the fallback if it's dead code
- Adding a test case that explicitly tests User1 as the validator
This is a minor observation and doesn't affect correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_audit_test.go` around lines 519 - 534, The test contains a defensive fallback that adds the validator share to expectedIncrease1 when increase1 mismatches auditReward1, but given injectTestValidator always generates a new keypair User1 can never be the validator so that branch is effectively dead; either remove the unreachable fallback block (the if that parses totalValFeesStr and updates expectedIncrease1 using infraShareVal) from the test around variables increase1, expectedIncrease1 and auditReward1, or add an explicit test variant that makes User1 the validator by reusing User1's keypair with injectTestValidator (so the fallback is exercised) and assert the validator-share-adjusted expectation instead. Ensure you update or add assertions referencing increase1.String() and expectedIncrease1.String() accordingly.internal/migrations/033-order-book-settlement.sql (1)
141-144: Integer division truncates$per_validator; verify remainder handling is sufficient.The calculation
$infra_share / $validator_countperforms integer division, which truncates. The remainder is computed and given to the first validator. However,$val_pctis computed as12.50 / $validator_countwhich also truncates, and this percentage is added per-validator without accounting for the percentage remainder. Over many validators, the summed percentages inob_fee_distribution_detailsmay not equal exactly 12.50%.This is likely acceptable for audit purposes (small rounding error), but worth noting if exact percentage tracking is required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/033-order-book-settlement.sql` around lines 141 - 144, The current math truncates when computing per-validator shares and the per-validator percentage ($per_validator, $val_remainder, $val_pct) causing lost fractional percentage mass; change $val_pct to compute using a higher-precision NUMERIC (e.g., NUMERIC(10,4) or more) so division preserves fractional cents, then compute a percentage remainder like $pct_remainder := 12.50::NUMERIC(...) - ($val_pct * $validator_count::NUMERIC(...)) and distribute that remainder deterministically across validators (e.g., add the smallest precision unit to the first N validators using the existing $first_validator / loop logic) so the sum written to ob_fee_distribution_details equals exactly 12.50 while keeping $per_validator/$val_remainder handling consistent.
🤖 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 84-99: Migration 033 inserts into ob_fee_distributions and
ob_fee_distribution_details before those tables exist (they are created in
migration 036); fix by either moving the CREATE TABLE statements for
ob_fee_distributions and ob_fee_distribution_details from migration 036 into
migration 033 above the INSERTs, or renumbering migration 033 to run after 036
so the tables exist when INSERT INTO ob_fee_distributions and INSERT INTO
ob_fee_distribution_details execute.
---
Nitpick comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 141-144: The current math truncates when computing per-validator
shares and the per-validator percentage ($per_validator, $val_remainder,
$val_pct) causing lost fractional percentage mass; change $val_pct to compute
using a higher-precision NUMERIC (e.g., NUMERIC(10,4) or more) so division
preserves fractional cents, then compute a percentage remainder like
$pct_remainder := 12.50::NUMERIC(...) - ($val_pct *
$validator_count::NUMERIC(...)) and distribute that remainder deterministically
across validators (e.g., add the smallest precision unit to the first N
validators using the existing $first_validator / loop logic) so the sum written
to ob_fee_distribution_details equals exactly 12.50 while keeping
$per_validator/$val_remainder handling consistent.
In `@tests/streams/order_book/fee_distribution_audit_test.go`:
- Around line 519-534: The test contains a defensive fallback that adds the
validator share to expectedIncrease1 when increase1 mismatches auditReward1, but
given injectTestValidator always generates a new keypair User1 can never be the
validator so that branch is effectively dead; either remove the unreachable
fallback block (the if that parses totalValFeesStr and updates expectedIncrease1
using infraShareVal) from the test around variables increase1, expectedIncrease1
and auditReward1, or add an explicit test variant that makes User1 the validator
by reusing User1's keypair with injectTestValidator (so the fallback is
exercised) and assert the validator-share-adjusted expectation instead. Ensure
you update or add assertions referencing increase1.String() and
expectedIncrease1.String() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f0bf651-efa2-4119-af3d-7277296d4674
📒 Files selected for processing (2)
internal/migrations/033-order-book-settlement.sqltests/streams/order_book/fee_distribution_audit_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/migrations/033-order-book-settlement.sql (1)
95-99:⚠️ Potential issue | 🔴 CriticalCreate the audit tables before inserting the placeholder row.
ob_fee_distributionsandob_fee_distribution_detailsare still defined ininternal/migrations/036-order-book-audit.sql, so this insert will run before the tables exist and fail during migration replay. Move those table definitions earlier or renumber this migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/033-order-book-settlement.sql` around lines 95 - 99, The INSERT into ob_fee_distributions is running before the audit tables are created (they're defined in 036-order-book-audit.sql), causing migrations to fail; fix by ensuring ob_fee_distributions and ob_fee_distribution_details are created before you insert the placeholder row: either move the CREATE TABLE statements from 036-order-book-audit.sql into an earlier migration (before the INSERT in 033-order-book-settlement.sql) or renumber the migration files so that 036 becomes earlier than 033; update references to the INSERT (variables like $distribution_id, $query_id, $block_count, `@block_timestamp`) remain unchanged.
🧹 Nitpick comments (1)
tests/streams/order_book/fee_distribution_audit_test.go (1)
594-624: Make the proposer explicit instead of silently falling back.
NewTestProposerPub(t)is not registered as a validator in this helper, so a call site that forgets to passvalPubstill reachesdistribute_fees, but no longer exercises the validator-aware path these tests are meant to cover. Requiring a registered proposer here would fail fast instead of silently downgrading the scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/fee_distribution_audit_test.go` around lines 594 - 624, The helper fundVaultAndDistributeFees currently silently generates a proposer via NewTestProposerPub when none is passed, which masks tests that should exercise the validator-aware path; change fundVaultAndDistributeFees to require an explicit proposer (remove the fallback that calls NewTestProposerPub) and return an error immediately if proposer is not supplied, and ensure callers are updated to pass the intended validator public key; additionally before calling distribute_fees verify the provided proposer is a registered validator (or fail fast) so tests can't accidentally skip the validator-aware branch.
🤖 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 233-239: The current cast to NUMERIC(10,2) for $curr_pct causes
tiny positive LP shares <0.005 to become 0.00 and then violate the constraint
ob_fee_distribution_details.total_reward_percent > 0; fix by preserving higher
precision for audit rows or skipping zero-percent inserts: stop casting
$res.reward_percent to NUMERIC(10,2) (remove/replace the $curr_pct conversion)
so the inserted/updated total_reward_percent uses a higher-precision numeric
(e.g. same scale as source) or add a guard to only INSERT/UPDATE when $curr_pct
> 0; update references to $curr_pct in the INSERT ... ON CONFLICT SET clause
accordingly.
- Around line 141-185: The loop over tn_utils.get_validators() can assign
$v_payout = 0 when $per_validator is zero, causing the INSERT/UPDATE into
ob_fee_distribution_details to violate the >0 constraint; update the validator
loop (inside the for $v in tn_utils.get_validators() block) to skip any
validator whose computed $v_payout <= 0: before calling the bridge unlock
functions (hooodi_tt2.unlock/sepolia_bridge.unlock/ethereum_bridge.unlock),
before creating/looking up participant rows, before calling
ob_record_net_impact, and before inserting/updating ob_fee_distribution_details,
add a guard that only proceeds when $v_payout > 0 (or alternatively stop after
distributing at most $infra_share smallest positive payouts), ensuring
zero-value payouts are neither unlocked nor written to
ob_fee_distribution_details.
---
Duplicate comments:
In `@internal/migrations/033-order-book-settlement.sql`:
- Around line 95-99: The INSERT into ob_fee_distributions is running before the
audit tables are created (they're defined in 036-order-book-audit.sql), causing
migrations to fail; fix by ensuring ob_fee_distributions and
ob_fee_distribution_details are created before you insert the placeholder row:
either move the CREATE TABLE statements from 036-order-book-audit.sql into an
earlier migration (before the INSERT in 033-order-book-settlement.sql) or
renumber the migration files so that 036 becomes earlier than 033; update
references to the INSERT (variables like $distribution_id, $query_id,
$block_count, `@block_timestamp`) remain unchanged.
---
Nitpick comments:
In `@tests/streams/order_book/fee_distribution_audit_test.go`:
- Around line 594-624: The helper fundVaultAndDistributeFees currently silently
generates a proposer via NewTestProposerPub when none is passed, which masks
tests that should exercise the validator-aware path; change
fundVaultAndDistributeFees to require an explicit proposer (remove the fallback
that calls NewTestProposerPub) and return an error immediately if proposer is
not supplied, and ensure callers are updated to pass the intended validator
public key; additionally before calling distribute_fees verify the provided
proposer is a registered validator (or fail fast) so tests can't accidentally
skip the validator-aware branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc381dd1-a4d7-4a0f-8a50-a84c199b661b
📒 Files selected for processing (2)
internal/migrations/033-order-book-settlement.sqltests/streams/order_book/fee_distribution_audit_test.go
Comment on lines
+141
to
+185
| $per_validator NUMERIC(78, 0) := $infra_share / $validator_count::NUMERIC(78, 0); | ||
| $val_remainder NUMERIC(78, 0) := $infra_share - ($per_validator * $validator_count::NUMERIC(78, 0)); | ||
| $first_validator BOOL := TRUE; | ||
| $val_pct NUMERIC(10, 2) := 12.50::NUMERIC(10, 2) / $validator_count::NUMERIC(10, 2); | ||
| $val_pct_remainder NUMERIC(10, 2) := 12.50::NUMERIC(10, 2) - ($val_pct * $validator_count::NUMERIC(10, 2)); | ||
|
|
||
| for $v in tn_utils.get_validators() { | ||
| -- Extract row fields to local variables (Kuneiform SQL generator limitation) | ||
| $v_wallet_hex TEXT := $v.wallet_hex; | ||
| $v_wallet_bytes BYTEA := $v.wallet_bytes; | ||
| $v_payout NUMERIC(78, 0) := $per_validator; | ||
| $v_pct NUMERIC(10, 2) := $val_pct; | ||
|
|
||
| -- Give remainder to first validator (deterministic: sorted by pubkey) | ||
| if $first_validator { | ||
| $v_payout := $v_payout + $val_remainder; | ||
| $v_pct := $v_pct + $val_pct_remainder; | ||
| $first_validator := FALSE; | ||
| } | ||
|
|
||
| -- Unlock funds via bridge | ||
| if $bridge = 'hoodi_tt2' { hoodi_tt2.unlock($v_wallet_hex, $v_payout); } | ||
| else if $bridge = 'sepolia_bridge' { sepolia_bridge.unlock($v_wallet_hex, $v_payout); } | ||
| else if $bridge = 'ethereum_bridge' { ethereum_bridge.unlock($v_wallet_hex, $v_payout); } | ||
|
|
||
| -- Ensure validator has a participant record | ||
| $v_pid INT; | ||
| for $p in SELECT id FROM ob_participants WHERE wallet_address = $v_wallet_bytes { $v_pid := $p.id; } | ||
| if $v_pid IS NULL { | ||
| INSERT INTO ob_participants (id, wallet_address) | ||
| SELECT COALESCE(MAX(id), 0) + 1, $v_wallet_bytes | ||
| FROM ob_participants; | ||
| for $p in SELECT id FROM ob_participants WHERE wallet_address = $v_wallet_bytes { $v_pid := $p.id; } | ||
| } | ||
| if $v_pid IS NOT NULL { | ||
| $next_id_val INT; | ||
| for $row in SELECT COALESCE(MAX(id), 0::INT) + 1 as val FROM ob_net_impacts { $next_id_val := $row.val; } | ||
| ob_record_net_impact($next_id_val, $query_id, $v_pid, $winning_outcome, 0::INT8, $v_payout, FALSE); | ||
|
|
||
| -- Record validator in distribution details so indexer picks it up | ||
| INSERT INTO ob_fee_distribution_details (distribution_id, participant_id, wallet_address, reward_amount, total_reward_percent) | ||
| VALUES ($distribution_id, $v_pid, $v_wallet_bytes, $v_payout, $v_pct) | ||
| ON CONFLICT (distribution_id, participant_id) DO UPDATE | ||
| SET reward_amount = ob_fee_distribution_details.reward_amount + $v_payout, | ||
| total_reward_percent = ob_fee_distribution_details.total_reward_percent + $v_pct; |
There was a problem hiding this comment.
Guard zero-value validator payouts before writing detail rows.
Lines 141-142 can produce 0 for $per_validator when infra_share < validator_count. Those validators then hit Line 181 with reward_amount = 0, but ob_fee_distribution_details.reward_amount is constrained to > 0, so the whole settlement aborts on small fee pools. Only unlock/record validators with a positive payout, or distribute the remainder across at most $infra_share validators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/migrations/033-order-book-settlement.sql` around lines 141 - 185,
The loop over tn_utils.get_validators() can assign $v_payout = 0 when
$per_validator is zero, causing the INSERT/UPDATE into
ob_fee_distribution_details to violate the >0 constraint; update the validator
loop (inside the for $v in tn_utils.get_validators() block) to skip any
validator whose computed $v_payout <= 0: before calling the bridge unlock
functions (hooodi_tt2.unlock/sepolia_bridge.unlock/ethereum_bridge.unlock),
before creating/looking up participant rows, before calling
ob_record_net_impact, and before inserting/updating ob_fee_distribution_details,
add a guard that only proceeds when $v_payout > 0 (or alternatively stop after
distributing at most $infra_share smallest positive payouts), ensuring
zero-value payouts are neither unlocked nor written to
ob_fee_distribution_details.
Comment on lines
233
to
+239
| $curr_pct NUMERIC(10,2) := $res.reward_percent::NUMERIC(10, 2); | ||
|
|
||
| INSERT INTO ob_fee_distribution_details (distribution_id, participant_id, wallet_address, reward_amount, total_reward_percent) | ||
| VALUES ($distribution_id, $curr_pid, $curr_wallet, $curr_reward, $curr_pct); | ||
| VALUES ($distribution_id, $curr_pid, $curr_wallet, $curr_reward, $curr_pct) | ||
| ON CONFLICT (distribution_id, participant_id) DO UPDATE | ||
| SET reward_amount = ob_fee_distribution_details.reward_amount + $curr_reward, | ||
| total_reward_percent = ob_fee_distribution_details.total_reward_percent + $curr_pct; |
There was a problem hiding this comment.
Tiny LP percentages round down to an invalid audit value.
Line 233 rounds each LP share to NUMERIC(10,2) before insert. Any positive share below 0.005 becomes 0.00, and Line 235 then violates ob_fee_distribution_details.total_reward_percent > 0. This will break distributions once enough LPs participate. Keep higher precision in storage for audit rows, or avoid inserting rounded-zero percentages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/migrations/033-order-book-settlement.sql` around lines 233 - 239,
The current cast to NUMERIC(10,2) for $curr_pct causes tiny positive LP shares
<0.005 to become 0.00 and then violate the constraint
ob_fee_distribution_details.total_reward_percent > 0; fix by preserving higher
precision for audit rows or skipping zero-percent inserts: stop casting
$res.reward_percent to NUMERIC(10,2) (remove/replace the $curr_pct conversion)
so the inserted/updated total_reward_percent uses a higher-precision numeric
(e.g. same scale as source) or add a guard to only INSERT/UPDATE when $curr_pct
> 0; update references to $curr_pct in the INSERT ... ON CONFLICT SET clause
accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves: https://github.com/trufnetwork/truf-network/issues/1380
Summary by CodeRabbit
New Features
Improvements
Tests