Conversation
|
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 four tn_utils precompile helpers (caller/leader hex and bytes); updates multiple migrations and order-book/settlement actions to use these helpers for safer address normalization; changes an order-book action to a VIEW; removes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
Bug Report Checklist
If this commit or author is incorrect, reply in this PR with:
Use git blame to identify the right commit and author before posting. See available commands to help comply with our Guidelines. |
Member
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/migrations/043-order-book-pnl.sql (1)
76-104:⚠️ Potential issue | 🟠 MajorMake net-impact ID allocation deterministic.
ob_record_net_impactstill allocatesidwithMAX(id) + 1, so the order of this loop becomes persisted state.SELECT DISTINCTwithoutORDER BYdoes not guarantee a stable(participant_id, outcome)order, which makes theob_net_impacts.idsequence unstable even though this file usesidas the indexer sync cursor.🔁 Keep the materialization order stable
- for $p in SELECT DISTINCT participant_id, outcome FROM ob_tx_payouts { + for $p in + SELECT participant_id, outcome + FROM ob_tx_payouts + GROUP BY participant_id, outcome + ORDER BY participant_id ASC, outcome ASC {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/043-order-book-pnl.sql` around lines 76 - 104, The loop over SELECT DISTINCT participant_id, outcome from ob_tx_payouts produces non-deterministic ordering which makes ob_record_net_impact's MAX(id)+1 allocation unstable; fix by (1) making the SELECT DISTINCT deterministic: add ORDER BY participant_id, outcome to the for $p in SELECT DISTINCT ... loop so the iteration order is stable, and (2) eliminate MAX(id)+1 in ob_record_net_impact: change its id allocation to use a proper sequence (e.g., nextval on ob_net_impacts_id_seq) or accept an explicit id parameter computed deterministically upstream — reference ob_tx_payouts, the for $p loop, ob_record_net_impact, and ob_net_impacts.id when making these changes.
🤖 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 65-73: The helpers are returning raw public-key bytes
(ctx.TxContext.Signer and ctx.TxContext.BlockContext.Proposer.Bytes()) while the
*_hex helpers use normalized Ethereum identifiers, causing lookup mismatches;
update get_caller_bytes (and getCallerBytesMethod), get_leader_bytes (and
getLeaderBytesMethod) to return the bytes of the normalized address instead of
raw pubkey bytes by decoding ctx.TxContext.Caller into the address bytes (for
callers) and converting the proposer pubkey to the normalized identifier bytes
via auth.GetUserIdentifier() (for leaders), or alternatively add new explicit
helpers that return raw public-key bytes and keep the existing *_hex helpers
as-is so callers can choose the correct representation.
- Around line 62-64: Handlers get_caller_hex, get_caller_bytes, get_leader_hex,
and get_leader_bytes currently dereference ctx.TxContext without checking ctx
itself and can panic when called with a nil context; update each handler to
first guard with "ctx == nil ||" before checking ctx.TxContext (e.g., change "if
ctx.TxContext == nil { return resultFn([]any{\"\"}) }" to "if ctx == nil ||
ctx.TxContext == nil { return resultFn([]any{\"\"}) }") so they return the empty
sentinel value instead of panicking.
---
Outside diff comments:
In `@internal/migrations/043-order-book-pnl.sql`:
- Around line 76-104: The loop over SELECT DISTINCT participant_id, outcome from
ob_tx_payouts produces non-deterministic ordering which makes
ob_record_net_impact's MAX(id)+1 allocation unstable; fix by (1) making the
SELECT DISTINCT deterministic: add ORDER BY participant_id, outcome to the for
$p in SELECT DISTINCT ... loop so the iteration order is stable, and (2)
eliminate MAX(id)+1 in ob_record_net_impact: change its id allocation to use a
proper sequence (e.g., nextval on ob_net_impacts_id_seq) or accept an explicit
id parameter computed deterministically upstream — reference ob_tx_payouts, the
for $p loop, ob_record_net_impact, and ob_net_impacts.id when making these
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5cc80ad-f355-4590-91c9-1a2085049708
📒 Files selected for processing (7)
extensions/tn_utils/precompiles.gointernal/migrations/024-attestation-actions.sqlinternal/migrations/031-order-book-vault.sqlinternal/migrations/032-order-book-actions.sqlinternal/migrations/033-order-book-settlement.sqlinternal/migrations/041-settlement-config-actions.sqlinternal/migrations/043-order-book-pnl.sql
💤 Files with no reviewable changes (1)
- internal/migrations/031-order-book-vault.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/043-order-book-pnl.sql`:
- Around line 133-141: The new ACTION ob_record_net_impact signature added a
required $id INT parameter which breaks existing callers (notably the settlement
call in internal/migrations/033-order-book-settlement.sql that still uses the
6-argument form); fix by restoring backward compatibility: either add a thin
wrapper ACTION with the old 6-argument signature that generates or supplies the
missing $id (or delegates to the new ob_record_net_impact) or update the
invocation in the earlier migration to pass the new $id argument and adjust any
callers accordingly so the arity matches ob_record_net_impact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19673205-c2c1-474f-b787-e3556523722b
📒 Files selected for processing (2)
extensions/tn_utils/precompiles.gointernal/migrations/043-order-book-pnl.sql
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/tn_utils/precompiles.go (1)
101-106: Fallback to Signer may return different-length bytes.When the caller is not a valid 20-byte Ethereum address, the fallback returns
ctx.TxContext.Signerwhich is the raw public key (typically 33 or 65 bytes for secp256k1). This length mismatch could cause unexpected behavior in migrations that expect 20-byte addresses.Consider whether this fallback is intentional for non-EVM key types, or if it should return an empty byte array to fail explicitly.
🔧 Optional: Return empty bytes instead of Signer for consistency
if b, err := hex.DecodeString(caller); err == nil && len(b) == 20 { return resultFn([]any{b}) } - // Fallback to Signer (public key) if not a hex address - return resultFn([]any{ctx.TxContext.Signer}) + // Return empty bytes if not a valid 20-byte address + return resultFn([]any{[]byte{}})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_utils/precompiles.go` around lines 101 - 106, The fallback currently returns ctx.TxContext.Signer (raw public key) when hex.DecodeString(caller) is not a 20-byte address, causing length mismatch; change the fallback in the function that calls resultFn to return a consistent 20-byte empty slice instead (e.g. a zeroed []byte of length 20) rather than the raw signer bytes so callers expecting 20-byte addresses don't break; update the branch that now does return resultFn([]any{ctx.TxContext.Signer}) to return resultFn([]any{make([]byte, 20)}) and adjust/add tests around caller parsing to cover non-hex and non-20-byte cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extensions/tn_utils/precompiles.go`:
- Around line 101-106: The fallback currently returns ctx.TxContext.Signer (raw
public key) when hex.DecodeString(caller) is not a 20-byte address, causing
length mismatch; change the fallback in the function that calls resultFn to
return a consistent 20-byte empty slice instead (e.g. a zeroed []byte of length
20) rather than the raw signer bytes so callers expecting 20-byte addresses
don't break; update the branch that now does return
resultFn([]any{ctx.TxContext.Signer}) to return resultFn([]any{make([]byte,
20)}) and adjust/add tests around caller parsing to cover non-hex and
non-20-byte cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9193d591-3912-41bd-90c4-6b43e66cca65
📒 Files selected for processing (2)
extensions/tn_utils/precompiles.gointernal/migrations/033-order-book-settlement.sql
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/041-settlement-config-actions.sql (1)
7-7:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency: comment says 30-minute but actual default is 5-minute.
Line 7 states "Enables settlement with 30-minute schedule by default" but lines 16 and 21 configure a 5-minute schedule (
*/5 * * * *).📝 Proposed fix
-- Changes: -- - Enables settlement with 30-minute schedule by default +-- - Enables settlement with 5-minute schedule by default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/041-settlement-config-actions.sql` at line 7, Update the misleading comment that reads "Enables settlement with 30-minute schedule by default" to reflect the actual configured cron schedule (*/5 * * * * = 5-minute) or, alternatively, change the cron expressions (*/5 * * * *) to a 30-minute schedule if the intended default is 30 minutes; locate the comment text and the cron occurrences (the literal "Enables settlement with 30-minute schedule by default" and the cron expressions "*/5 * * * *") and make them consistent (either modify the comment to say 5-minute or modify the cron to a 30-minute pattern such as "*/30 * * * *").
🤖 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 96-100: The call to ob_record_net_impact is missing the required
leading $id INT parameter (migration 043 changed the signature), causing runtime
failures; generate a new unique id (e.g., $next_id) before the data provider
payout block (where $dp_pid and $dp_addr are used) and pass that id as the first
argument to both ob_record_net_impact calls (the one setting infra share and the
other later in the same payout section), ensuring you use the same $next_id
generation logic used elsewhere in this migration for unique ids.
- Around line 111-117: The call to ob_record_net_impact inside the leader-bytes
branch is missing the final $id argument (it currently passes 6 args); update
the ob_record_net_impact($query_id, $val_pid, $winning_outcome, 0::INT8,
$infra_share, FALSE) call to include the missing $id parameter as the last
argument (i.e., ob_record_net_impact(..., FALSE, $id)) so it matches the
expected 7-argument signature used elsewhere.
- Around line 35-50: Both ob_batch_unlock_collateral and ob_cleanup_tx_payouts
generate IDs non-atomically by querying SELECT COALESCE(MAX(id),0)+1 FROM
ob_net_impacts (using the local $next_id and calls to ob_record_net_impact),
which can cause duplicate primary-key collisions when both run in the same
transaction; replace this pattern with an atomic ID source such as a dedicated
sequence (e.g., create and use nextval('ob_net_impacts_id_seq')) or acquire a
row-level lock on ob_net_impacts before computing the next id, and update usages
in the migration procedure (the $next_id logic and calls to
ob_record_net_impact) to obtain IDs from the sequence (or after locking) instead
of computing MAX(id)+1.
---
Outside diff comments:
In `@internal/migrations/041-settlement-config-actions.sql`:
- Line 7: Update the misleading comment that reads "Enables settlement with
30-minute schedule by default" to reflect the actual configured cron schedule
(*/5 * * * * = 5-minute) or, alternatively, change the cron expressions (*/5 * *
* *) to a 30-minute schedule if the intended default is 30 minutes; locate the
comment text and the cron occurrences (the literal "Enables settlement with
30-minute schedule by default" and the cron expressions "*/5 * * * *") and make
them consistent (either modify the comment to say 5-minute or modify the cron to
a 30-minute pattern such as "*/30 * * * *").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3ef9f7f-0d7c-4d84-82db-116f3924b4b5
📒 Files selected for processing (4)
extensions/tn_settlement/settlement_integration_test.gointernal/migrations/033-order-book-settlement.sqlinternal/migrations/041-settlement-config-actions.sqlinternal/migrations/043-order-book-pnl.sql
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/migrations/033-order-book-settlement.sql (2)
25-31: No error handling for unknown bridge values.If
$bridgedoes not match any of the three known values (hoodi_tt2,sepolia_bridge,ethereum_bridge), the unlock operation silently fails without any error or notification. This could mask configuration issues or data corruption where an invalid bridge is stored.Consider adding an
elseclause to raise an error for unknown bridges.🛡️ Proposed fix
if $bridge = 'hoodi_tt2' { hoodi_tt2.unlock($payout.wallet_hex, $payout.amount); } else if $bridge = 'sepolia_bridge' { sepolia_bridge.unlock($payout.wallet_hex, $payout.amount); } else if $bridge = 'ethereum_bridge' { ethereum_bridge.unlock($payout.wallet_hex, $payout.amount); + } else { + ERROR('Unknown bridge: ' || $bridge); }🤖 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 25 - 31, The branch handling for $bridge currently only calls hoodi_tt2.unlock, sepolia_bridge.unlock, or ethereum_bridge.unlock and silently does nothing for any other value; add a final else branch that throws/raises an explicit error (e.g., RAISE EXCEPTION or equivalent in this migration language) including the invalid $bridge value and context (payout.wallet_hex, payout.amount) so invalid bridge values are surfaced and fail the migration instead of being ignored; update the conditional around $bridge and ensure the error message identifies $bridge and the payout being processed.
89-93: Same missing error handling for unknown bridges indistribute_fees.The DP and validator payout sections (lines 91-93 and 109-111) have the same issue: unknown bridge values silently skip the unlock without error. Apply the same fix pattern as suggested for
ob_batch_unlock_collateral.Also applies to: 109-111
🤖 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 89 - 93, The DP and validator payout unlock branches in distribute_fees silently ignore unknown $bridge values; add explicit error handling like in ob_batch_unlock_collateral by appending an ELSE branch that RAISEs an exception with the offending $bridge value (e.g. "unknown bridge: %", $bridge) after the hoodi_tt2.unlock / sepolia_bridge.unlock / ethereum_bridge.unlock conditionals for both the DP payout block (uses $dp_wallet_hex and $infra_share) and the validator payout block (the equivalent wallet/amount variables) so unknown bridge names fail loudly.
🤖 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 25-31: The branch handling for $bridge currently only calls
hoodi_tt2.unlock, sepolia_bridge.unlock, or ethereum_bridge.unlock and silently
does nothing for any other value; add a final else branch that throws/raises an
explicit error (e.g., RAISE EXCEPTION or equivalent in this migration language)
including the invalid $bridge value and context (payout.wallet_hex,
payout.amount) so invalid bridge values are surfaced and fail the migration
instead of being ignored; update the conditional around $bridge and ensure the
error message identifies $bridge and the payout being processed.
- Around line 89-93: The DP and validator payout unlock branches in
distribute_fees silently ignore unknown $bridge values; add explicit error
handling like in ob_batch_unlock_collateral by appending an ELSE branch that
RAISEs an exception with the offending $bridge value (e.g. "unknown bridge: %",
$bridge) after the hoodi_tt2.unlock / sepolia_bridge.unlock /
ethereum_bridge.unlock conditionals for both the DP payout block (uses
$dp_wallet_hex and $infra_share) and the validator payout block (the equivalent
wallet/amount variables) so unknown bridge names fail loudly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 361574e2-6766-4335-9fe2-419143ac5fd9
📒 Files selected for processing (2)
internal/migrations/033-order-book-settlement.sqlinternal/migrations/041-settlement-config-actions.sql
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/truflation/website/issues/3432
Summary by CodeRabbit
New Features
Bug Fixes
Refactoring
Chores