chore: distribute LP rewards after market settle#1309
Merged
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new tn_lp_rewards extension (scheduling and broadcasting sample_lp_rewards) with engine helper ops and tests; introduces DB migration and actions for lp_rewards_config; updates settlement and migration SQL (collateral check, settlement defaults, payout semantics); prevents overlapping settlement runs via an atomic processing guard and adds nonce-aware retries in settlement broadcast paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Ext as "tn_lp_rewards Extension"
participant EngOps as "EngineOperations"
participant DB as "Database"
participant Accts as "Accounts"
participant RPC as "JSON-RPC Broadcaster"
participant Engine as "Engine/Service"
Ext->>EngOps: reloadConfig(ctx)
EngOps->>DB: LoadLPRewardsConfig()
DB-->>EngOps: (enabled, interval, maxMarkets)
EngOps-->>Ext: updated config
Ext->>EngOps: sampleLPRewards(ctx, blockHeight)
EngOps->>DB: GetActiveMarkets(limit)
DB-->>EngOps: [marketIDs]
loop per market id
EngOps->>Engine: resolve chain/market metadata
Engine-->>EngOps: chainID
EngOps->>Accts: fetch signer account & next nonce
Accts-->>EngOps: account + nonce
EngOps->>EngOps: build & sign sample_lp_rewards tx
EngOps->>RPC: BroadcastTx(signedTx, sync)
RPC-->>EngOps: txHash, txResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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: 5
🤖 Fix all issues with AI agents
In `@extensions/tn_lp_rewards/internal/engine_ops.go`:
- Around line 164-254: The BroadcastSampleLPRewards function currently reads a
fresh nonce from the DB each call (nextNonce) which causes nonce reuse in async
batch calls; change the signature of BroadcastSampleLPRewards to accept an
explicit caller-supplied nonce (e.g. add parameter callerNonce uint64) and use
that when non-zero, otherwise fall back to the existing DB lookup logic; replace
usages of nextNonce in CreateNodeTransaction and the debug log with the resolved
nonce variable (e.g. nonce := callerNonce; if nonce == 0 { ... existing DB logic
sets nonce }) and update all call sites to pass a monotonically increasing nonce
when batching; ensure the function still returns errors as before.
- Around line 44-55: The getFreshReadTx method on EngineOperations currently
falls back to returning e.db without checking for nil; update
EngineOperations.getFreshReadTx to check if e.dbPool is nil and then verify e.db
is non-nil before returning — if e.db is nil return a non-nil error (and a no-op
cleanup func) so callers like ExecuteWithoutEngineCtx don't proceed with a nil
DB; keep the existing behavior when e.db is present and preserve the readTx
cleanup closure for e.dbPool paths.
In `@extensions/tn_lp_rewards/tn_lp_rewards.go`:
- Around line 381-389: The loop calling BroadcastSampleLPRewards re-reads the
account nonce for each market causing nonce reuse because broadcasts are async;
fix by reading the current nonce once before the loop and deterministically
assigning unique nonces per tx (e.g., call your existing nonce-read routine
once, store as baseNonce, then for each market pass baseNonce+i into
BroadcastSampleLPRewards or add a nonce parameter to BroadcastSampleLPRewards so
it no longer fetches the nonce internally), or replace the per-call DB read with
a proper nonce manager that atomically reserves/increments nonces; update the
call sites (the for loop over markets using ext.engOps.BroadcastSampleLPRewards
and the BroadcastSampleLPRewards signature/implementation) to use the
single-read-and-increment approach.
- Around line 294-325: leaderEndBlock reads extension config fields without
locking while reloadConfig and sampleLPRewards run concurrently, causing races;
fix by taking ext.mu.RLock in leaderEndBlock, snapshot the needed fields
(enabled, samplingIntervalBlocks, configReloadInterval, maxMarketsPerRun) into
local variables, release the RLock, and then use those locals for the checks and
decisions (including atomic handling of lastCheckedHeight). Do not access
ext.enabled or ext.samplingIntervalBlocks after releasing the lock; instead pass
any needed snapshot like maxMarketsPerRun as an argument to sampleLPRewards (or
have sampleLPRewards accept a config struct) so sampleLPRewards does not read
the field unprotected. Ensure reloadConfig continues to use ext.mu for writes.
In `@tests/streams/order_book/lp_rewards_config_test.go`:
- Around line 64-68: The anonymous row handler currently uses unsafe type
assertions row.Values[0].(bool), row.Values[1].(int64), row.Values[2].(int64)
which can panic depending on backend; replace these with guarded type switches
like the pattern already used for max_markets_per_run so that enabled,
samplingInterval and maxMarkets are assigned by switching on both int and int64
(and handle unexpected types), and apply the same defensive switch-based casting
to the other instances where those columns are read (the other test handlers
that set enabled, samplingInterval, maxMarkets).
🧹 Nitpick comments (3)
extensions/tn_lp_rewards/tn_lp_rewards.go (1)
126-174: Surface config-load errors instead of ignoring.
LoadLPRewardsConfigerrors are currently dropped, which hides DB/migration issues. Logging once at startup would aid diagnostics.Suggested tweak
- enabled, interval, maxMarkets, _ := engOps.LoadLPRewardsConfig(ctx) + enabled, interval, maxMarkets, err := engOps.LoadLPRewardsConfig(ctx) + if err != nil { + logger.Warn("failed to load LP rewards config; using defaults", "error", err) + }internal/migrations/042-lp-rewards-config.sql (2)
83-119: Add explicit input guards and a reasonable upper bound.
enabledcan be NULL and will hit a NOT NULL constraint with a generic error, andmax_markets_per_runis unbounded. Consider explicit checks (e.g., 1–100) for clearer errors and safer limits.Suggested validation
if $has_role = FALSE { ERROR('Only network_writer can update LP rewards config'); } -- Validate inputs + if $enabled IS NULL { + ERROR('enabled cannot be NULL'); + } if $sampling_interval_blocks < 1 { ERROR('sampling_interval_blocks must be >= 1'); } - if $max_markets_per_run < 1 { - ERROR('max_markets_per_run must be >= 1'); + if $max_markets_per_run < 1 OR $max_markets_per_run > 100 { + ERROR('max_markets_per_run must be between 1 and 100'); }
130-138: Validate $limit to avoid negative or unbounded queries.
A simple guard prevents accidental full-table scans or unexpected behavior.Suggested guard
CREATE OR REPLACE ACTION get_active_markets_for_sampling($limit INT) PUBLIC VIEW RETURNS (query_id INT) { + if $limit IS NULL OR $limit < 1 { + ERROR('limit must be >= 1'); + } for $row in SELECT id FROM ob_queries WHERE settled = FALSE ORDER BY id ASC LIMIT $limit
MicBun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@extensions/tn_lp_rewards/internal/engine_ops.go`:
- Around line 168-259: The code in BroadcastSampleLPRewards can pass a nil e.db
when e.dbPool is nil; replace the manual dbPool check with the existing helper
getFreshReadTx to obtain a validated read transaction (and ensure it is closed
with defer readTx.Rollback(ctx)), then call e.accounts.GetAccount(ctx, readTx,
signerAccountID) instead of passing e.db; keep the same not-found handling to
set nextNonce and preserve existing error returns and logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@extensions/tn_lp_rewards/tn_lp_rewards.go`:
- Around line 141-155: The code assigns enabled/interval/maxMarkets even when
engOps.LoadLPRewardsConfig(ctx) returns an error, overwriting defaults; change
the logic so after calling LoadLPRewardsConfig you only set ext.enabled,
ext.samplingIntervalBlocks, and ext.maxMarketsPerRun when err == nil (i.e.,
successful load), leaving the existing defaults on the extension (obtained via
GetExtension()) untouched when there is an error; use the existing logger.Warn
path for failures and perform assignments inside the success branch that
references enabled, interval, and maxMarkets (or keep the current ext.* values
otherwise).
- Around line 326-335: The config reload is guarded by the extension enabled
check in leaderEndBlock, preventing ext.reloadConfig from running when
enabled==false and making re-enabling impossible without restart; move the
reload logic that checks configReloadInterval,
atomic.LoadInt64(&ext.lastCheckedHeight) and invokes go
ext.reloadConfig(context.Background()) and
atomic.StoreInt64(&ext.lastCheckedHeight, blockHeight) to run before the enabled
guard so config can be reloaded even when the extension is currently disabled.
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/3256
Summary by CodeRabbit