chore: increase max limit of LP rewards sampling and batch#1321
Merged
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
📝 WalkthroughWalkthroughReplaces leader-only, broadcasted LP-reward sampling with an internal end-block hook that calls a new DB batch action (sample_all_active_lp_rewards), removes broadcaster/signer abstractions, increases max markets per run to 1000, updates migrations, and switches tests to batch sampling calls. Changes
Sequence Diagram(s)sequenceDiagram
participant Node as All Nodes
participant EndBlock as End-Block Hook
participant Engine as Engine Ops
participant DB as Database
Node->>EndBlock: block finalizes
EndBlock->>EndBlock: check context & reload config
EndBlock->>Engine: performInternalSampling(ctx, app, height, maxMarkets)
Engine->>DB: CALL sample_all_active_lp_rewards(block, market_limit=1000)
DB->>DB: iterate active markets (<= market_limit)
DB->>DB: per-market sample_lp_rewards (p1/p2 magnitude, scoring)
DB->>DB: normalize reward_percent across participants
DB-->>Engine: result / errors
Engine-->>EndBlock: log outcome (no error propagation)
EndBlock-->>Node: end-block processing complete
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
extensions/tn_lp_rewards/internal/engine_ops.go (1)
61-72:⚠️ Potential issue | 🟡 MinorInconsistent default values:
maxMarketsinitialized to 1000 but fallbacks return 50.The variable is initialized to 1000 at line 65, but error/fallback paths at lines 71, 107, 109, and 112 still return 50. This creates inconsistent behavior depending on how the function exits.
🔧 Suggested fix
func (e *EngineOperations) LoadLPRewardsConfig(ctx context.Context) (bool, int, int, error) { var ( enabled bool = true interval int = 10 maxMarkets int = 1000 found bool ) db, cleanup, err := e.getFreshReadTx(ctx) if err != nil { - return true, 10, 50, fmt.Errorf("get fresh read tx: %w", err) + return true, 10, 1000, fmt.Errorf("get fresh read tx: %w", err) } defer cleanup() // ... (lines 75-106) if strings.Contains(msg, "lp_rewards_config") && (strings.Contains(msg, "does not exist") || strings.Contains(msg, "no such table") || strings.Contains(msg, "undefined table") || strings.Contains(msg, "not found")) { e.logger.Info("lp_rewards_config table not found; using defaults") - return true, 10, 50, nil + return true, 10, 1000, nil } - return true, 10, 50, err + return true, 10, 1000, err } if !found { - return true, 10, 50, nil + return true, 10, 1000, nil } return enabled, interval, maxMarkets, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/tn_lp_rewards/internal/engine_ops.go` around lines 61 - 72, The function LoadLPRewardsConfig has mismatched defaults: maxMarkets is initialized to 1000 but several error/fallback returns use 50; update the function to use a single source of truth by either changing the initial maxMarkets to 50 or replacing hardcoded fallback returns with the maxMarkets variable (preferably define a const like defaultMaxMarkets and use it to initialize maxMarkets and in all return/error paths), and ensure getFreshReadTx error return and all subsequent early returns return the same maxMarkets value alongside enabled and interval.internal/migrations/034-order-book-rewards.sql (1)
35-37:⚠️ Potential issue | 🟡 MinorOutdated documentation: action is now PRIVATE, not PUBLIC.
The comment states "Can be called by anyone (PUBLIC action)" but
sample_lp_rewardswas changed toPRIVATEat line 123. This documentation is now misleading.📝 Suggested fix
* Sampling Trigger: -* - External system calls sample_lp_rewards($query_id, $block) periodically -* - Recommended: Every 50 blocks (~10 minutes with 12s block times) -* - Can be called by anyone (PUBLIC action) +* - Internal hook calls sample_all_active_lp_rewards periodically +* - Recommended: Every 10 blocks (configurable via lp_rewards_config) +* - Called internally during block finalization (PRIVATE action)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/034-order-book-rewards.sql` around lines 35 - 37, Update the top-of-file documentation to reflect that sample_lp_rewards is PRIVATE (no longer callable by anyone); find the descriptive block mentioning "Can be called by anyone (PUBLIC action)" and change it to indicate the action is PRIVATE (e.g., "Can only be invoked internally (PRIVATE action)" or "Requires PRIVILEGE: PRIVATE"), ensuring the comment matches the actual function declaration sample_lp_rewards which was made PRIVATE.
🧹 Nitpick comments (1)
tests/streams/order_book/rewards_test.go (1)
29-31: Consider preserving error chain for better debugging.The current error wrapping loses the original error type. Using
%wwould preserve the error chain.♻️ Suggested improvement
if res.Error != nil { - return fmt.Errorf("%s", res.Error.Error()) + return fmt.Errorf("sample_lp_rewards failed: %w", res.Error) }Same applies to line 51:
if res.Error != nil { - return fmt.Errorf("%s", res.Error.Error()) + return fmt.Errorf("sample_all_active_lp_rewards failed: %w", res.Error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/streams/order_book/rewards_test.go` around lines 29 - 31, The current error wrapping in the test uses fmt.Errorf("%s", res.Error.Error()), which discards the original error chain; change this to wrap the original error with fmt.Errorf("...: %w", res.Error) (or otherwise return res.Error directly) in the same block where res is checked so the original error type is preserved; apply the same fix to the other occurrence noted (the similar check around line 51) to keep the error chain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@extensions/tn_lp_rewards/internal/engine_ops.go`:
- Around line 61-72: The function LoadLPRewardsConfig has mismatched defaults:
maxMarkets is initialized to 1000 but several error/fallback returns use 50;
update the function to use a single source of truth by either changing the
initial maxMarkets to 50 or replacing hardcoded fallback returns with the
maxMarkets variable (preferably define a const like defaultMaxMarkets and use it
to initialize maxMarkets and in all return/error paths), and ensure
getFreshReadTx error return and all subsequent early returns return the same
maxMarkets value alongside enabled and interval.
In `@internal/migrations/034-order-book-rewards.sql`:
- Around line 35-37: Update the top-of-file documentation to reflect that
sample_lp_rewards is PRIVATE (no longer callable by anyone); find the
descriptive block mentioning "Can be called by anyone (PUBLIC action)" and
change it to indicate the action is PRIVATE (e.g., "Can only be invoked
internally (PRIVATE action)" or "Requires PRIVILEGE: PRIVATE"), ensuring the
comment matches the actual function declaration sample_lp_rewards which was made
PRIVATE.
---
Nitpick comments:
In `@tests/streams/order_book/rewards_test.go`:
- Around line 29-31: The current error wrapping in the test uses
fmt.Errorf("%s", res.Error.Error()), which discards the original error chain;
change this to wrap the original error with fmt.Errorf("...: %w", res.Error) (or
otherwise return res.Error directly) in the same block where res is checked so
the original error type is preserved; apply the same fix to the other occurrence
noted (the similar check around line 51) to keep the error chain intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
extensions/tn_lp_rewards/internal/engine_ops.goextensions/tn_lp_rewards/tn_lp_rewards.gointernal/benchmark/types.gointernal/migrations/034-order-book-rewards.sqltests/streams/order_book/fee_distribution_audit_test.gotests/streams/order_book/fee_distribution_test.gotests/streams/order_book/rewards_test.go
MicBun
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/migrations/042-lp-rewards-config.sql (1)
37-39: Table CHECK constraint does not enforce the upper bound of 1000.The
update_lp_rewards_configaction validatesmax_markets_per_run <= 1000(lines 111-112), but the table's CHECK constraint only enforces>= 1. Direct inserts or updates bypassing the action could store values exceeding 1000.Consider adding the upper bound to the CHECK constraint for defense-in-depth:
Proposed fix
CHECK (id = 1), CHECK (sampling_interval_blocks >= 1), - CHECK (max_markets_per_run >= 1) + CHECK (max_markets_per_run >= 1 AND max_markets_per_run <= 1000) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/migrations/042-lp-rewards-config.sql` around lines 37 - 39, The table CHECK constraint currently enforces CHECK (max_markets_per_run >= 1) but not the upper bound enforced by the update_lp_rewards_config action; change the table constraint that references max_markets_per_run to also enforce the upper bound of 1000 (e.g. require max_markets_per_run BETWEEN 1 AND 1000 or add AND max_markets_per_run <= 1000) so direct inserts/updates cannot exceed 1000 while leaving the existing id and sampling_interval_blocks checks intact.
🤖 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/042-lp-rewards-config.sql`:
- Around line 37-39: The table CHECK constraint currently enforces CHECK
(max_markets_per_run >= 1) but not the upper bound enforced by the
update_lp_rewards_config action; change the table constraint that references
max_markets_per_run to also enforce the upper bound of 1000 (e.g. require
max_markets_per_run BETWEEN 1 AND 1000 or add AND max_markets_per_run <= 1000)
so direct inserts/updates cannot exceed 1000 while leaving the existing id and
sampling_interval_blocks checks intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/migrations/041-settlement-config-actions.sqlinternal/migrations/042-lp-rewards-config.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/3400
Summary by CodeRabbit
New Features
Refactor
Migrations
Tests