Fix claimAllRewards failure isolation, add cache-invalidation and tier-boundary test coverage - #1908
Merged
Olowodarey merged 2 commits intoSep 26, 2026
Conversation
…r-boundary test coverage closes Arena1X#1803 closes Arena1X#1825 closes Arena1X#1856 closes Arena1X#1857 - Arena1X#1857: claimAllRewards aborted the entire batch if any single claim failed, silently skipping the user's other eligible rewards for that request. Isolated each claim in its own try/catch so one failure no longer blocks the rest, and added a `results` field to ClaimAllRewardsResponseDto reporting per-prediction success/failure (reusing the existing BATCH_PREDICTION_STATUS convention from submitBatchPredictions for consistency). Added a regression test proving a failing middle claim no longer prevents the other claims in the batch. - Arena1X#1856: invalidateMarketResolutionCaches was already correctly wired into adminResolveMarket and correctly ordered after the write commits, but had no test coverage of that flow (admin.service.spec.ts stubbed it as a bare jest.fn() with no assertions on when/how it's called). Added tests covering: invalidation only firing after market.save() succeeds, affected-user ids being collected from the market's predictions, invalidation never firing when the on-chain resolution fails, and end-to-end getMarketAnalytics/getCategoryAnalytics reads immediately after invalidation reflecting the resolved state instead of stale cached values. - Arena1X#1825: invalidatePredictionStatsCache turned out to be dead code, never called from any write path in the codebase (verified via a repo-wide grep). The issue's premise assumed it was already wired into a prediction mutation flow. Added tests for the function's own behavior in isolation (key construction, concurrent-call safety) and documented that it has no caller yet; wiring it into a mutation flow is a separate, deliberate decision for whoever owns that flow's design. - Arena1X#1803: the issue assumed tier_for resolves a lock tier by comparing duration against a threshold (">="/">" semantics with fallthrough to a lower tier). The actual implementation matches a tier by *exact* equality on duration only, with no threshold logic at all - a duration one second off from any configured tier already reverts with InvalidLockPeriod rather than falling through, as the pre-existing test_stake_with_invalid_lock_period_reverts test demonstrates. Added tests pinning down the real exact-match behavior at each configured tier's boundary, in a new tests/tier_boundary_tests.rs file. Also fixed, as a necessary prerequisite for Arena1X#1803 (the crate did not compile at all on main): lib.rs referenced LockTier.min_lock_duration (the actual field is `duration`) and StakingError::InvalidLockTiers/ NoPosition/StillLocked, none of which exist in errors.rs. Restored these to the existing, semantically-matching variants (PositionNotFound, LockNotElapsed, matching what the pre-existing test suite already asserts) and added the one genuinely-missing InvalidTierConfig variant, then removed lib.rs's duplicate, buggy validate_lock_tiers in favor of the already-correct lock::validate_tiers. Disclosure: tests/staking_tests.rs still does not compile as a whole file after this fix - it also calls unstake/withdraw/deposit_fees/ pending_rewards/claim_rewards/set_paused, none of which exist anywhere in lib.rs. That is a much larger, separate pre-existing gap (an entire feature surface missing from the contract, not a naming mismatch) that is out of scope for this PR; tests/tier_boundary_tests.rs was split out as its own file so the Arena1X#1803 tests can compile and run independently of it.
|
@Martha-code-dev Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch had an error being deployed
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
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.
Summary
closes #1803
closes #1825
closes #1856
closes #1857
resultsfield toClaimAllRewardsResponseDtoreporting per-prediction success/failure (reusing the existingBATCH_PREDICTION_STATUSconvention fromsubmitBatchPredictionsfor consistency). Added a regression test proving a failing middle claim no longer prevents the other claims in the batch from succeeding.tier_forBoundary Equality Not Covered #1803 (staking-vault tier_for boundary equality): the issue assumed tier_for resolves a lock tier by comparing duration against a threshold (">="/">" semantics with fallthrough to a lower tier). The actual implementation matches a tier by exact equality on duration only, with no threshold logic at all: a duration one second off from any configured tier already reverts with InvalidLockPeriod rather than falling through, as the pre-existing test_stake_with_invalid_lock_period_reverts test already demonstrates. Added tests pinning down the real exact-match behavior at each configured tier's boundary in a new tests/tier_boundary_tests.rs file.Also included
contracts/staking-vault did not compile at all on main prior to this PR (unrelated to any of the four issues above, but a necessary prerequisite to get #1803's tests running): lib.rs referenced LockTier.min_lock_duration (the actual field is
duration) and StakingError::InvalidLockTiers/NoPosition/StillLocked, none of which exist in errors.rs. This looks like leftover drift from an earlier commit (bebd2d0, #1806's fix) that never got matching struct/enum changes. Restored these references to the existing, semantically-matching variants (PositionNotFound, LockNotElapsed, matching what the pre-existing test suite already asserts at those call sites) and added the one genuinely-missing InvalidTierConfig variant. Also removed lib.rs's duplicate, buggy validate_lock_tiers function in favor of the already-correct lock::validate_tiers it was a near-exact copy of.Disclosure: tests/staking_tests.rs still does not compile as a whole file after this fix. It also calls unstake, withdraw, deposit_fees, pending_rewards, claim_rewards, and set_paused, none of which exist anywhere in lib.rs (only initialize, stake, request_unlock, and 3 getters are implemented). This is a much larger, separate pre-existing gap, an entire feature surface missing from the contract, not a naming mismatch, and is out of scope for this PR. tests/tier_boundary_tests.rs was split out as its own file specifically so the #1803 tests can compile and run independently of that unrelated breakage. Whoever picks up implementing those methods will need staking_tests.rs's existing assertions as the spec for their intended behavior.
Test plan