refactor(staker): unify reward lastCollectTime invariants#1263
refactor(staker): unify reward lastCollectTime invariants#1263
lastCollectTime invariants#1263Conversation
Collapse the repeated `!skipInternalUpdate` checks in `CollectReward` into a single internal reward path.
Rename the internal reward guard from `skipInternalUpdate` to `hasInternalReward` to avoid the double negative branch Also narrow internal reward accounting variables to the branch where they are used and return explicit zero values on the no internal reward path.
WalkthroughChanges refactor the staker contract's reward collection logic by introducing a deposit resolver pattern for timestamp resolution, reorganizing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22b4fc2e-f72e-44b5-81c2-e599bb43525d
📒 Files selected for processing (4)
contract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/v1/staker.gnocontract/r/gnoswap/staker/v1/type.gno
| // Unclaimable must still be processed when regular internal rewards are | ||
| // skipped so accrued pool state is cleared for the collect window. | ||
| unClaimableInternal := s.processUnClaimableReward(depositResolver.TargetPoolPath(), currentTime) | ||
| if unClaimableInternal > 0 { | ||
| totalEmissionSent = safeAddInt64(totalEmissionSent, unClaimableInternal) | ||
| } | ||
|
|
||
| err := s.store.SetTotalEmissionSent(totalEmissionSent) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| deposits := s.getDeposits() | ||
| deposits.set(positionId, deposit) | ||
|
|
||
| if unClaimableInternal > 0 { | ||
| gns.Transfer(cross, communityPoolAddr, unClaimableInternal) | ||
| } | ||
|
|
||
| return rewardToUser, rewardPenalty, toUserExternalReward, toUserExternalPenalty | ||
| return "0", "0", toUserExternalReward, toUserExternalPenalty |
There was a problem hiding this comment.
Emit an event when the no-internal branch processes unclaimable rewards.
This branch can reset unclaimable reward state, update totalEmissionSent, persist the deposit, and transfer GNS to the community pool, but returns without emitting any event. Add a CollectReward-style event when unClaimableInternal > 0 so indexers can observe the state change and transfer. As per coding guidelines, “All state changes MUST emit events.”
1 participant
Description
This PR makes the staker reward cursor invariant explicit and symmetric across internal GNS rewards and external incentive rewards.
Both reward streams maintains a
lastCollectTimecursor. That cursor is expected to move monotonically forward from the effective last collection time, where an unset to zero cursor falls back to the deposit'sStakeTime. Before this change, the internal and external reward paths enforced that rule through different read paths, which made the invariant weaker for external rewards in defensive cases.Problem
CollectRewardsettles two independent reward streams:Both streams update a
lastCollectTimecursor to prevent collecting rewards over an invalid earlier interval. However the update paths were asymmetric:DepositResolver.InternalRewardLastCollectTime(), which falls back toStakeTime()when the stored cursor is zero.Deposit.GetExternalRewardLastCollectTime(...)directly, which bypassed the wrapped fallback behavior.As a result, the external path did not enforce the same lower bound in two edge cases:
0In those cases, the external path could skip or weaken the monotonic validation that the internal path already performed. The normal production flow is not expected to create those states intentionally, but the implementation still encoded two different strengths of the same invariant. That made the reward cursor logic harder to reason about and easier to regress later.
Changes
DepositResolver.ExternalRewardLastCollectTime(incentiveID), so missing or zero cursor state falls back toStakeTime()like the internal path.Deposit.SetExternalRewardLastCollectTimealready lazy-initializes the map.CollectRewardupdate flow so reward accounting, cursor updates, transfers, and event emission live under one positivehasInternalRewardbranch.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests