fix(staker): optimize EndExternalIncentive refund calculation#1232
fix(staker): optimize EndExternalIncentive refund calculation#1232
EndExternalIncentive refund calculation#1232Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaced an O(n) per-deposit refund loop in external incentives with an O(1) incentive-level refund calculation (duration, RewardPerSecond, distributable, remainder, collected penalties, and unclaimableReward; result clamped). Updated tests to use deterministic forward timelines, tolerance-based assertions, and added a zero-liquidity transition test. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 |
`EndExtenalIncentive` Replace the O(N) deposit scan with an O(1) calculation: refund = calculateUnclaimableReward(incentiveId) + remainder where `unclaimableReward` covers zero-liquidity periods and remainder captures the integer division truncation from `rewardPerSecond`.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contract/r/gnoswap/staker/v1/external_incentive_test.gno (1)
1117-1139: Tolerance-based assertion aligns with documented trade-off.The tolerance formula
numDeposits * numWarmups * 2accounts for per-deposit, per-tier rounding dust mentioned in the PR description.One minor observation: consider adding a comment explaining the tolerance derivation (2 units max per deposit per warmup tier) to aid future maintainability.
🔧 Optional: Add clarifying comment for tolerance calculation
numDeposits := int64(2) numWarmups := int64(len(warmups)) +// Each deposit can accumulate up to 2 units of MulDiv rounding dust per warmup tier tolerance := numDeposits * numWarmups * 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6b0e892-4568-4fc3-bcfe-7d8a87f762f7
⛔ Files ignored due to path filters (6)
contract/r/scenario/staker/external_incentive_complete_distribution_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/staker/external_incentive_drain_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/staker/external_incentive_dust_refund_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/staker/single_gns_external_ends_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/staker/staker_native_create_collect_unstake_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/staker/staker_native_create_collect_unstake_with_finish_filetest.gnois excluded by!**/*filetest.gno
📒 Files selected for processing (3)
contract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/external_incentive_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_incentives_test.gno
notJoon
force-pushed
the
fix/staker-end-external-incentive-dos
branch
from
06c2c32 to
fb87c24
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contract/r/gnoswap/staker/v1/external_incentive.gno (1)
248-259: [LOW] Rule: Fail closed on broken refund accounting.Problem: If the derived penalty/refund state goes negative, clamping to
0hides the mismatch and still finalizes the incentive.Suggested guard
consumed := safeSubInt64(incentiveResolver.TotalRewardAmount(), incentiveResolver.RewardAmount()) + // Invariant: zero-liquidity accrual is returned separately via + // `calculateUnclaimableReward`, so `consumed - distributedRewardAmount` + // should represent collected penalties only. collectedPenalties := safeSubInt64(consumed, incentiveResolver.DistributedRewardAmount()) + if collectedPenalties < 0 { + return nil, 0, makeErrorWithDetails( + errCannotEndIncentive, + ufmt.Sprintf( + "invalid incentive accounting: consumed(%d) < distributed(%d)", + consumed, + incentiveResolver.DistributedRewardAmount(), + ), + ) + } refund := safeAddInt64(safeAddInt64(unclaimableReward, remainder), collectedPenalties) maxRefund := safeSubInt64(incentiveResolver.TotalRewardAmount(), incentiveResolver.DistributedRewardAmount()) if refund > maxRefund { refund = maxRefund } - if refund < 0 { - refund = 0 - }Rationale: A negative
collectedPenaltiesmeans thedistributedRewardAmount/rewardAmountinvariant is already broken; aborting is safer than silently zeroing the refund. Based on learnings,distributedRewardAmountmust only be incremented by the net reward paid to LPs, while external penalties remain in the staker and are refunded duringEndExternalIncentive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b74afa4-8965-4633-bf8f-2c68c5b045bf
📒 Files selected for processing (1)
contract/r/gnoswap/staker/v1/external_incentive.gno
|
1 participant
Description
Replace the O(n) deposit iteration in
endExternalIncentivewith an O(1) refund calculation based on unclaimable period tracking.Changes
Before:
EndExternalIncentiveiterated over every deposit in the pool to compute uncollected rewards. An attacker could create thousands of minimal staked positions, making the transaction prohibitively expensive and blocking the incentive creator from recovering their remaining rewards and GNS deposit.After:
unclaimableReward: rewards for zero-liquidity periods (viacalculateUnclaimableReward, already tracked bymodifyDeposit)remainder: integer division truncation fromrewardPerSecond(L-14 dust)collectedPenalties: warmup penalties retained in staker duringCollectReward, derived from existing accounting fields without iteration:(totalRewardAmount - rewardAmount) - distributedRewardAmountmaxRefundcap (totalRewardAmount - distributedRewardAmount) prevents over-refundTrade-off
Per-deposit
MulDivfloor rounding dust (2–5 units per incentive) now remains in the staker contract instead of being returned to the creator. This is because computing per-deposit rounding requires the iteration we're removing. The amounts are negligible and the direction is conservative (users' collectable rewards are never affected).Related Works
IsExternallyIncentivizedPoolto Check Active Incentives Only #852: Precision loss inrewardPerSecondleading to dust rewardsCollectReward, but retained in staker and returned to creator viaEndExternalIncentive.distributedRewardAmountnow tracks rewards only (not penalties), which required addingcollectedPenaltiesto the O(1) formulaSummary by CodeRabbit
Bug Fixes
Tests