From e6bb492dd8f108ece307f623eb882f042ea0356f Mon Sep 17 00:00:00 2001 From: Martha Date: Sat, 26 Sep 2026 19:26:02 +0100 Subject: [PATCH] Fix claimAllRewards failure isolation, add cache-invalidation and tier-boundary test coverage closes #1803 closes #1825 closes #1856 closes #1857 - #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. - #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. - #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. - #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 #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 #1803 tests can compile and run independently of it. --- backend/src/admin/admin.service.spec.ts | 128 +++++++++++++++ .../src/analytics/analytics.service.spec.ts | 66 +++++++- .../creator-events.service.spec.ts | 119 +++++++++++--- .../predictions/dto/rewards-summary.dto.ts | 49 +++++- .../predictions/predictions.service.spec.ts | 120 +++++++++++++- .../src/predictions/predictions.service.ts | 36 ++++- contracts/staking-vault/src/errors.rs | 3 + contracts/staking-vault/src/lib.rs | 29 +--- .../tests/tier_boundary_tests.rs | 153 ++++++++++++++++++ 9 files changed, 641 insertions(+), 62 deletions(-) create mode 100644 contracts/staking-vault/tests/tier_boundary_tests.rs diff --git a/backend/src/admin/admin.service.spec.ts b/backend/src/admin/admin.service.spec.ts index 71984bd7..0e044b30 100644 --- a/backend/src/admin/admin.service.spec.ts +++ b/backend/src/admin/admin.service.spec.ts @@ -401,4 +401,132 @@ describe('AdminService (Verified Addresses)', () => { ); }); }); + + describe('adminResolveMarket', () => { + let marketRepository: any; + let predictionRepository: any; + let analyticsService: any; + let sorobanService: any; + let notificationsService: any; + + const makeMarket = (overrides: Record = {}) => ({ + id: 'market-1', + on_chain_market_id: 'chain-1', + title: 'Will it rain tomorrow?', + is_resolved: false, + is_cancelled: false, + resolved_outcome: null, + outcome_options: ['Yes', 'No'], + ...overrides, + }); + + beforeEach(() => { + marketRepository = service['marketsRepository']; + predictionRepository = service['predictionsRepository']; + analyticsService = service['analyticsService']; + sorobanService = service['sorobanService']; + notificationsService = service['notificationsService']; + + marketRepository.save = jest + .fn() + .mockImplementation((entity: any) => Promise.resolve(entity)); + predictionRepository.find = jest.fn().mockResolvedValue([]); + notificationsService.create.mockResolvedValue(undefined); + sorobanService.resolveMarket.mockResolvedValue(undefined); + analyticsService.invalidateMarketResolutionCaches.mockResolvedValue( + undefined, + ); + }); + + it('invalidates the market resolution caches only after the resolution is persisted', async () => { + const market = makeMarket(); + marketRepository.findOne.mockResolvedValue(market); + + const saveOrder: string[] = []; + marketRepository.save.mockImplementation(async (entity: any) => { + saveOrder.push('save'); + return entity; + }); + analyticsService.invalidateMarketResolutionCaches.mockImplementation( + async () => { + saveOrder.push('invalidate'); + }, + ); + + await service.adminResolveMarket( + 'market-1', + { resolved_outcome: 'Yes' }, + 'admin-1', + ); + + expect(saveOrder).toEqual(['save', 'invalidate']); + expect( + analyticsService.invalidateMarketResolutionCaches, + ).toHaveBeenCalledWith('market-1', 'chain-1', []); + }); + + it('passes the affected users from every prediction on the market', async () => { + const market = makeMarket(); + marketRepository.findOne.mockResolvedValue(market); + predictionRepository.find.mockResolvedValue([ + { + user: { id: 'user-1', stellar_address: 'GADDR1' }, + chosen_outcome: 'Yes', + }, + { + user: { id: 'user-2', stellar_address: 'GADDR2' }, + chosen_outcome: 'No', + }, + ]); + + await service.adminResolveMarket( + 'market-1', + { resolved_outcome: 'Yes' }, + 'admin-1', + ); + + expect( + analyticsService.invalidateMarketResolutionCaches, + ).toHaveBeenCalledWith('market-1', 'chain-1', ['user-1', 'user-2']); + }); + + it('does not invalidate caches when the on-chain resolution fails', async () => { + const market = makeMarket(); + marketRepository.findOne.mockResolvedValue(market); + sorobanService.resolveMarket.mockRejectedValue( + new Error('soroban timeout'), + ); + + await expect( + service.adminResolveMarket( + 'market-1', + { resolved_outcome: 'Yes' }, + 'admin-1', + ), + ).rejects.toThrow('Failed to resolve market on Soroban'); + + expect(marketRepository.save).not.toHaveBeenCalled(); + expect( + analyticsService.invalidateMarketResolutionCaches, + ).not.toHaveBeenCalled(); + }); + + it('rejects resolving a market that is already resolved', async () => { + marketRepository.findOne.mockResolvedValue( + makeMarket({ is_resolved: true }), + ); + + await expect( + service.adminResolveMarket( + 'market-1', + { resolved_outcome: 'Yes' }, + 'admin-1', + ), + ).rejects.toThrow('Market is already resolved'); + + expect( + analyticsService.invalidateMarketResolutionCaches, + ).not.toHaveBeenCalled(); + }); + }); }); diff --git a/backend/src/analytics/analytics.service.spec.ts b/backend/src/analytics/analytics.service.spec.ts index d2295cb4..60f04ac7 100644 --- a/backend/src/analytics/analytics.service.spec.ts +++ b/backend/src/analytics/analytics.service.spec.ts @@ -561,7 +561,7 @@ describe('AnalyticsService', () => { }); it('uses a distinct cache entry for a different user', async () => { - const otherUser = { ...baseUser, id: 'user-id-2' } as User; + const otherUser = { ...baseUser, id: 'user-id-2' }; usersRepository.findOne.mockImplementation((opts: any) => Promise.resolve(opts.where.id === otherUser.id ? otherUser : baseUser), ); @@ -734,5 +734,69 @@ describe('AnalyticsService', () => { expect(warnSpy).toHaveBeenCalled(); }); + + it('getMarketAnalytics read right after invalidation reflects post-resolution predictions, not the cached pre-resolution value', async () => { + const predictionsRepo = module.get(getRepositoryToken(Prediction)); + const market = { + id: 'market-1', + on_chain_market_id: 'chain-1', + title: 'Will it rain tomorrow?', + outcome_options: ['Yes', 'No'], + total_pool_stroops: '1000', + participant_count: 1, + end_time: new Date(Date.now() + 60_000), + } as Market; + + marketsRepository.findOne.mockResolvedValue(market); + predictionsRepo.find = jest + .fn() + .mockResolvedValueOnce([]) // pre-resolution: no predictions counted yet + .mockResolvedValueOnce([{ chosen_outcome: 'Yes' }]); // post-resolution: the settled prediction now counts + + const beforeResolution = await service.getMarketAnalytics('market-1'); + expect( + beforeResolution.outcome_distribution.find((o) => o.outcome === 'Yes') + ?.count, + ).toBe(0); + + await service.invalidateMarketResolutionCaches('market-1', 'chain-1', []); + + const afterResolution = await service.getMarketAnalytics('market-1'); + expect( + afterResolution.outcome_distribution.find((o) => o.outcome === 'Yes') + ?.count, + ).toBe(1); + expect(marketsRepository.findOne).toHaveBeenCalledTimes(2); + }); + + it('getCategoryAnalytics read right after invalidation reflects the resolved market, not the cached pre-resolution active count', async () => { + const market = { + id: 'market-1', + category: 'Weather', + is_resolved: false, + is_cancelled: false, + total_pool_stroops: '1000', + participant_count: 1, + } as Market; + + marketsRepository.find + .mockResolvedValueOnce([market]) // pre-resolution: still active + .mockResolvedValueOnce([{ ...market, is_resolved: true }]); // post-resolution + + const beforeResolution = await service.getCategoryAnalytics(); + const weatherBefore = beforeResolution.categories.find( + (c) => c.name === 'Weather', + ); + expect(weatherBefore?.active_markets).toBe(1); + + await service.invalidateMarketResolutionCaches('market-1', null, []); + + const afterResolution = await service.getCategoryAnalytics(); + const weatherAfter = afterResolution.categories.find( + (c) => c.name === 'Weather', + ); + expect(weatherAfter?.active_markets).toBe(0); + expect(marketsRepository.find).toHaveBeenCalledTimes(2); + }); }); }); diff --git a/backend/src/creator-events/creator-events.service.spec.ts b/backend/src/creator-events/creator-events.service.spec.ts index 98dfc8a3..1c940af4 100644 --- a/backend/src/creator-events/creator-events.service.spec.ts +++ b/backend/src/creator-events/creator-events.service.spec.ts @@ -269,35 +269,33 @@ describe('CreatorEventsService getPayoutByAddress', () => { const makeLeaderboardEntry = ( overrides: Partial = {}, - ): CreatorEventLeaderboardEntry => - ({ - id: 'leaderboard-entry-1', - event_id: 'event-1', - user_address: '0xParticipant', - rank: 5, - total_predictions: 3, - correct_predictions: 1, - accuracy_percentage: 33.33, - is_winner: false, - completion_time: null, - created_at: new Date('2026-05-01T00:00:00.000Z'), - ...overrides, - }) as CreatorEventLeaderboardEntry; + ): CreatorEventLeaderboardEntry => ({ + id: 'leaderboard-entry-1', + event_id: 'event-1', + user_address: '0xParticipant', + rank: 5, + total_predictions: 3, + correct_predictions: 1, + accuracy_percentage: 33.33, + is_winner: false, + completion_time: null, + created_at: new Date('2026-05-01T00:00:00.000Z'), + ...overrides, + }); const makePayout = ( overrides: Partial = {}, - ): CreatorEventPayout => - ({ - id: 'payout-1', - event_id: 'event-1', - user_address: '0xParticipant', - payout_amount_stroops: '0', - is_claimed: false, - leaderboard_entry_id: 'leaderboard-entry-1', - leaderboard_entry: makeLeaderboardEntry(), - created_at: new Date('2026-05-02T00:00:00.000Z'), - ...overrides, - }) as CreatorEventPayout; + ): CreatorEventPayout => ({ + id: 'payout-1', + event_id: 'event-1', + user_address: '0xParticipant', + payout_amount_stroops: '0', + is_claimed: false, + leaderboard_entry_id: 'leaderboard-entry-1', + leaderboard_entry: makeLeaderboardEntry(), + created_at: new Date('2026-05-02T00:00:00.000Z'), + ...overrides, + }); beforeEach(async () => { creatorEventPayoutRepository = { @@ -540,3 +538,72 @@ describe('CreatorEventsService getLeaderboard', () => { expect(contractService.getEventLeaderboard).not.toHaveBeenCalled(); }); }); + +// NOTE: `invalidatePredictionStatsCache` is not currently called from any +// write path in this codebase (verified via a repo-wide grep for its name) - +// there is no prediction/match-result mutation flow that invokes it yet, so +// there is nothing to test for "runs after the write commits" or "a read +// right after invalidation sees fresh data" (#1825's original ask assumed +// this wiring already existed). These tests cover the function's own +// behavior in isolation instead. Wiring it into a mutation flow is a +// separate, deliberate follow-up for whoever owns that flow's design. +describe('CreatorEventsService invalidatePredictionStatsCache', () => { + let service: CreatorEventsService; + let cacheManager: { get: jest.Mock; set: jest.Mock; del: jest.Mock }; + + beforeEach(async () => { + cacheManager = { get: jest.fn(), set: jest.fn(), del: jest.fn() }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + CreatorEventsService, + { provide: ContractService, useValue: {} }, + { provide: SearchService, useValue: {} }, + { provide: getRepositoryToken(CreatorEvent), useValue: {} }, + { + provide: getRepositoryToken(CreatorEventLeaderboardEntry), + useValue: {}, + }, + { provide: getRepositoryToken(Match), useValue: {} }, + { provide: getRepositoryToken(MatchPrediction), useValue: {} }, + { provide: getRepositoryToken(User), useValue: {} }, + { provide: getRepositoryToken(CreatorEventPayout), useValue: {} }, + { provide: CACHE_MANAGER, useValue: cacheManager }, + ], + }).compile(); + + service = module.get(CreatorEventsService); + }); + + it('clears the event stats cache key for the given event', async () => { + await service.invalidatePredictionStatsCache('event-1'); + + expect(cacheManager.del).toHaveBeenCalledWith( + '/creator-events/event-1/stats', + ); + expect(cacheManager.del).toHaveBeenCalledTimes(1); + }); + + it('also clears the per-user score cache key when an address is given', async () => { + await service.invalidatePredictionStatsCache('event-1', 'GADDR1'); + + expect(cacheManager.del).toHaveBeenCalledWith( + '/creator-events/event-1/stats', + ); + expect(cacheManager.del).toHaveBeenCalledWith( + '/creator-events/event-1/score/GADDR1', + ); + expect(cacheManager.del).toHaveBeenCalledTimes(2); + }); + + it('does not throw and settles cleanly when called concurrently for the same event', async () => { + await expect( + Promise.all([ + service.invalidatePredictionStatsCache('event-1', 'GADDR1'), + service.invalidatePredictionStatsCache('event-1', 'GADDR1'), + ]), + ).resolves.toEqual([undefined, undefined]); + + expect(cacheManager.del).toHaveBeenCalledTimes(4); + }); +}); diff --git a/backend/src/predictions/dto/rewards-summary.dto.ts b/backend/src/predictions/dto/rewards-summary.dto.ts index 9f6dcd44..2f48fd9f 100644 --- a/backend/src/predictions/dto/rewards-summary.dto.ts +++ b/backend/src/predictions/dto/rewards-summary.dto.ts @@ -1,4 +1,6 @@ -import { ApiProperty } from '@nestjs/swagger'; +import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; +import { BATCH_PREDICTION_STATUS } from './batch-submit-response.dto'; +import type { BatchPredictionStatus } from './batch-submit-response.dto'; export class RewardsSummaryDto { @ApiProperty({ @@ -24,6 +26,36 @@ export class RewardsSummaryDto { vesting_xlm: number; } +export class ClaimResultDto { + @ApiProperty({ description: 'Prediction this result refers to' }) + prediction_id!: string; + + @ApiProperty({ + description: 'Whether this individual claim succeeded or failed', + enum: [BATCH_PREDICTION_STATUS.FULFILLED, BATCH_PREDICTION_STATUS.REJECTED], + example: BATCH_PREDICTION_STATUS.FULFILLED, + }) + status!: BatchPredictionStatus; + + @ApiPropertyOptional({ + description: 'Transaction hash (only when fulfilled)', + example: 'a1b2c3...', + }) + tx_hash?: string; + + @ApiPropertyOptional({ + description: 'Payout amount claimed, in stroops (only when fulfilled)', + example: '10000000', + }) + payout_amount_stroops?: string; + + @ApiPropertyOptional({ + description: 'Failure reason (only when rejected)', + example: 'Soroban claimPayout failed', + }) + error?: string; +} + export class ClaimAllRewardsResponseDto { @ApiProperty({ description: 'Total amount claimed in this request, in XLM.', @@ -32,17 +64,28 @@ export class ClaimAllRewardsResponseDto { claimed_xlm: number; @ApiProperty({ - description: 'Number of predictions claimed in this request.', + description: 'Number of predictions successfully claimed in this request.', example: 3, }) claimed_count: number; @ApiProperty({ - description: 'Transaction hash of the most recently submitted claim.', + description: + 'Transaction hash of the most recently successful claim, or an empty ' + + 'string if every claim in this request failed.', example: 'a1b2c3...', }) transaction_hash: string; + @ApiProperty({ + description: + 'Per-prediction outcome for every claimable prediction attempted in ' + + 'this request, in the order they were processed. A rejected entry ' + + 'does not prevent the others from being attempted.', + type: [ClaimResultDto], + }) + results: ClaimResultDto[]; + @ApiProperty({ type: RewardsSummaryDto }) summary: RewardsSummaryDto; } diff --git a/backend/src/predictions/predictions.service.spec.ts b/backend/src/predictions/predictions.service.spec.ts index 95b9c530..2d5565e8 100644 --- a/backend/src/predictions/predictions.service.spec.ts +++ b/backend/src/predictions/predictions.service.spec.ts @@ -33,6 +33,7 @@ import { UsersService } from '../users/users.service'; import { SorobanService } from '../soroban/soroban.service'; import { SlippageCheckerService } from './services/slippage-checker.service'; import { SlippageExceededException } from './exceptions/slippage-exceeded.exception'; +import { BATCH_PREDICTION_STATUS } from './dto/batch-submit-response.dto'; type MockRepo = jest.Mocked< Pick< @@ -128,7 +129,7 @@ describe('PredictionsService', () => { findAndCount: jest.fn(), find: jest.fn().mockResolvedValue([]), createQueryBuilder: jest.fn().mockReturnValue(fraudQbMock), - } as unknown as MockRepo; + }; mockMarketsRepo = { findOne: jest.fn(), @@ -811,6 +812,20 @@ describe('PredictionsService', () => { expect(result.claimed_xlm).toBe(1.5); expect(result.transaction_hash).toBe('tx-2'); expect(mockSoroban.claimPayout).toHaveBeenCalledTimes(2); + expect(result.results).toEqual([ + { + prediction_id: 'p-1', + status: BATCH_PREDICTION_STATUS.FULFILLED, + tx_hash: 'tx-1', + payout_amount_stroops: '10000000', + }, + { + prediction_id: 'p-2', + status: BATCH_PREDICTION_STATUS.FULFILLED, + tx_hash: 'tx-2', + payout_amount_stroops: '5000000', + }, + ]); }); it('throws NoClaimableRewardsException when there is nothing to claim', async () => { @@ -820,6 +835,107 @@ describe('PredictionsService', () => { NoClaimableRewardsException, ); }); + + it('isolates a failing claim so the other claimable predictions still succeed', async () => { + const user = makeUser(); + const resolvedWon = makeMarket({ + id: 'm-won', + is_resolved: true, + resolved_outcome: 'Yes', + }); + + const claimablePredictions = [ + { + id: 'p-1', + user, + market: resolvedWon, + chosen_outcome: 'Yes', + payout_claimed: false, + stake_amount_stroops: '10000000', + }, + { + id: 'p-2', + user, + market: resolvedWon, + chosen_outcome: 'Yes', + payout_claimed: false, + stake_amount_stroops: '2000000', + }, + { + id: 'p-3', + user, + market: resolvedWon, + chosen_outcome: 'Yes', + payout_claimed: false, + stake_amount_stroops: '3000000', + }, + ] as Prediction[]; + + mockPredictionsRepo.find + .mockResolvedValueOnce(claimablePredictions) + .mockResolvedValueOnce([ + { + ...claimablePredictions[0], + payout_claimed: true, + payout_amount_stroops: '10000000', + }, + claimablePredictions[1], + { + ...claimablePredictions[2], + payout_claimed: true, + payout_amount_stroops: '3000000', + }, + ] as Prediction[]); + + // findOne() is used internally by claim() for each prediction. + mockPredictionsRepo.findOne + .mockResolvedValueOnce(claimablePredictions[0]) + .mockResolvedValueOnce(claimablePredictions[1]) + .mockResolvedValueOnce(claimablePredictions[2]); + + mockSoroban.claimPayout + .mockResolvedValueOnce({ + tx_hash: 'tx-1', + payout_amount_stroops: '10000000', + }) + .mockRejectedValueOnce(new Error('Soroban claimPayout failed')) + .mockResolvedValueOnce({ + tx_hash: 'tx-3', + payout_amount_stroops: '3000000', + }); + + mockPredictionsRepo.save = jest + .fn() + .mockImplementation((entity: Prediction) => Promise.resolve(entity)); + + const result = await service.claimAllRewards(user); + + // The middle claim failed, but both the first and third were still + // attempted and succeeded, rather than the batch aborting after p-2. + expect(mockSoroban.claimPayout).toHaveBeenCalledTimes(3); + expect(result.claimed_count).toBe(2); + expect(result.claimed_xlm).toBe(1.3); + expect(result.transaction_hash).toBe('tx-3'); + expect(result.results).toEqual([ + { + prediction_id: 'p-1', + status: BATCH_PREDICTION_STATUS.FULFILLED, + tx_hash: 'tx-1', + payout_amount_stroops: '10000000', + }, + { + prediction_id: 'p-2', + status: BATCH_PREDICTION_STATUS.REJECTED, + error: 'Soroban claimPayout failed', + }, + { + prediction_id: 'p-3', + status: BATCH_PREDICTION_STATUS.FULFILLED, + tx_hash: 'tx-3', + payout_amount_stroops: '3000000', + }, + ]); + }); }); describe('updateNote', () => { @@ -838,7 +954,7 @@ describe('PredictionsService', () => { mockPredictionsRepo.save.mockResolvedValue({ ...prediction, note: 'My analysis note', - } as Prediction); + }); const result = await service.updateNote( 'pred-1', diff --git a/backend/src/predictions/predictions.service.ts b/backend/src/predictions/predictions.service.ts index 967bbf94..0f1ddfc4 100644 --- a/backend/src/predictions/predictions.service.ts +++ b/backend/src/predictions/predictions.service.ts @@ -43,6 +43,7 @@ import { SorobanService } from '../soroban/soroban.service'; import { SlippageCheckerService } from './services/slippage-checker.service'; import { ClaimAllRewardsResponseDto, + ClaimResultDto, RewardsSummaryDto, } from './dto/rewards-summary.dto'; import { @@ -799,6 +800,10 @@ export class PredictionsService { /** * Claim every currently-claimable prediction for a user in sequence, reusing * `claim()` for the actual on-chain submission and bookkeeping per prediction. + * + * Each claim is isolated: a failure on one prediction (e.g. a transient + * on-chain error) is recorded in that entry's `results` and does not stop + * the remaining claimable predictions from being attempted. */ async claimAllRewards(user: User): Promise { const predictions = await this.predictionsRepository.find({ @@ -820,18 +825,41 @@ export class PredictionsService { let claimedStroops = 0n; let lastTxHash = ''; + const results: ClaimResultDto[] = []; + for (const predictionId of claimableIds) { - const claimed = await this.claim(predictionId, user); - claimedStroops += BigInt(claimed.payout_amount_stroops ?? '0'); - lastTxHash = claimed.tx_hash ?? lastTxHash; + try { + const claimed = await this.claim(predictionId, user); + claimedStroops += BigInt(claimed.payout_amount_stroops ?? '0'); + lastTxHash = claimed.tx_hash ?? lastTxHash; + results.push({ + prediction_id: predictionId, + status: BATCH_PREDICTION_STATUS.FULFILLED, + tx_hash: claimed.tx_hash ?? undefined, + payout_amount_stroops: claimed.payout_amount_stroops ?? undefined, + }); + } catch (err) { + this.logger.error( + `claimAllRewards: claim failed for prediction ${predictionId}`, + err, + ); + results.push({ + prediction_id: predictionId, + status: BATCH_PREDICTION_STATUS.REJECTED, + error: err instanceof Error ? err.message : String(err), + }); + } } const summary = await this.getRewardsSummary(user); return { claimed_xlm: stroopsToXlm(claimedStroops), - claimed_count: claimableIds.length, + claimed_count: results.filter( + (r) => r.status === BATCH_PREDICTION_STATUS.FULFILLED, + ).length, transaction_hash: lastTxHash, + results, summary, }; } diff --git a/contracts/staking-vault/src/errors.rs b/contracts/staking-vault/src/errors.rs index 6d2b6ad1..1566c422 100644 --- a/contracts/staking-vault/src/errors.rs +++ b/contracts/staking-vault/src/errors.rs @@ -28,6 +28,9 @@ pub enum StakingError { LockNotElapsed = 13, /// The supplied lock duration is not one of the configured tiers. InvalidLockPeriod = 14, + /// The lock-tier configuration supplied to `initialize` is empty or not + /// strictly ordered by ascending duration. + InvalidTierConfig = 15, // ── Rewards / fees ──────────────────────────────────────────────────────── /// There are no rewards available to claim for this position. diff --git a/contracts/staking-vault/src/lib.rs b/contracts/staking-vault/src/lib.rs index 226c80ce..e9f75501 100644 --- a/contracts/staking-vault/src/lib.rs +++ b/contracts/staking-vault/src/lib.rs @@ -78,29 +78,6 @@ fn require_not_paused(env: &Env) -> Result<(), StakingError> { Ok(()) } -/// Validate the lock-tier configuration supplied to `initialize`. -/// -/// Rejects an empty tier vector and any tiers that are not strictly ordered by -/// ascending `min_lock_duration`, so `lock::tier_for` can never silently resolve -/// the wrong boundary. Returns `InvalidLockTiers` on any violation. -fn validate_lock_tiers(tiers: &Vec) -> Result<(), StakingError> { - if tiers.is_empty() { - return Err(StakingError::InvalidLockTiers); - } - - let mut prev: Option = None; - for tier in tiers.iter() { - if let Some(prev_duration) = prev { - if tier.min_lock_duration <= prev_duration { - return Err(StakingError::InvalidLockTiers); - } - } - prev = Some(tier.min_lock_duration); - } - - Ok(()) -} - #[contractimpl] impl StakingVault { // ── Initialisation ────────────────────────────────────────────────────────── @@ -135,7 +112,7 @@ impl StakingVault { // Reject empty or out-of-order tier configurations up front so `stake` // can never silently apply the wrong boost. - validate_lock_tiers(&lock_tiers)?; + lock::validate_tiers(&lock_tiers)?; let config = Config { admin, @@ -272,14 +249,14 @@ impl StakingVault { return Err(StakingError::InvalidAmount); } - let mut position = get_position_raw(&env, &staker).ok_or(StakingError::NoPosition)?; + let mut position = get_position_raw(&env, &staker).ok_or(StakingError::PositionNotFound)?; if amount > position.amount { return Err(StakingError::InsufficientStake); } if env.ledger().timestamp() < position.unlock_at { - return Err(StakingError::StillLocked); + return Err(StakingError::LockNotElapsed); } position.unlock_requested_at = env.ledger().timestamp(); diff --git a/contracts/staking-vault/tests/tier_boundary_tests.rs b/contracts/staking-vault/tests/tier_boundary_tests.rs new file mode 100644 index 00000000..d292fe4e --- /dev/null +++ b/contracts/staking-vault/tests/tier_boundary_tests.rs @@ -0,0 +1,153 @@ +#![cfg(test)] + +// --------------------------------------------------------------------------- +// #1803 — `tier_for` boundary behavior +// +// Kept in its own file rather than tests/staking_tests.rs: that file also +// exercises unstake/withdraw/deposit_fees/pending_rewards/claim_rewards/ +// set_paused, none of which exist on StakingVaultClient in the current +// lib.rs (a separate, much larger pre-existing gap between the contract and +// its own test suite - see the PR description). Splitting these tests out +// lets them compile and run independently of that unrelated breakage. +// +// The issue as filed assumed `tier_for` resolves a tier by comparing +// `duration` against each tier's minimum threshold (i.e. ">="/">" semantics, +// where a duration between two configured tiers falls through to the +// next-lower one). That is not what the current implementation does: +// `lock::tier_for` (contracts/staking-vault/src/lock.rs) matches a tier by +// *exact* equality on `duration` only, with no threshold/fallthrough logic +// at all. A duration that doesn't exactly match one of the configured tiers +// - even by a single second - returns `InvalidLockPeriod` rather than +// resolving to a lower tier. These tests pin down that actual exact-match +// behavior at each configured tier's boundary, so a future refactor can't +// silently turn it into threshold matching (or vice versa) without a test +// failing. +// --------------------------------------------------------------------------- + +use soroban_sdk::{ + testutils::Address as _, + token::{StellarAssetClient, TokenClient}, + Address, Env, Vec, +}; +use staking_vault::{LockTier, StakingError, StakingVault, StakingVaultClient, UnbondingConfig}; + +fn setup_token(env: &Env) -> (Address, TokenClient<'static>, StellarAssetClient<'static>) { + let admin = Address::generate(env); + let sac = env.register_stellar_asset_contract_v2(admin); + let address = sac.address(); + let token_client = TokenClient::new(env, &address); + let asset_client = StellarAssetClient::new(env, &address); + (address, token_client, asset_client) +} + +fn tiers(env: &Env) -> Vec { + let mut tiers = Vec::new(env); + tiers.push_back(LockTier { + duration: 30 * 86_400, + boost_bps: 10_000, // 1.0x + }); + tiers.push_back(LockTier { + duration: 90 * 86_400, + boost_bps: 15_000, // 1.5x + }); + tiers.push_back(LockTier { + duration: 365 * 86_400, + boost_bps: 20_000, // 2.0x + }); + tiers +} + +fn setup( + env: &Env, +) -> ( + StakingVaultClient<'static>, + Address, + Address, + TokenClient<'static>, + StellarAssetClient<'static>, +) { + let contract_id = env.register(StakingVault, ()); + let client = StakingVaultClient::new(env, &contract_id); + let admin = Address::generate(env); + let (token_address, token_client, asset_client) = setup_token(env); + let fee_source = Address::generate(env); + + let unbonding_config = UnbondingConfig { + cooldown_period: 7 * 86_400, // 7 days + penalty_bps: 500, // 5% penalty + }; + + client.initialize( + &admin, + &token_address, + &fee_source, + &tiers(env), + &unbonding_config, + ); + + (client, admin, fee_source, token_client, asset_client) +} + +#[test] +fn test_tier_for_matches_each_configured_duration_exactly() { + let env = Env::default(); + let tier_list = tiers(&env); + + for tier in tier_list.iter() { + let resolved = staking_vault::lock::tier_for(&tier_list, tier.duration) + .expect("configured duration must resolve to its own tier"); + assert_eq!(resolved.duration, tier.duration); + assert_eq!(resolved.boost_bps, tier.boost_bps); + } +} + +#[test] +fn test_tier_for_one_second_below_a_tier_duration_is_invalid_not_next_lower_tier() { + let env = Env::default(); + let tier_list = tiers(&env); + + // One second short of the 90-day tier does NOT fall through to the + // 30-day tier - it is simply not a configured duration. + let result = staking_vault::lock::tier_for(&tier_list, 90 * 86_400 - 1); + assert!(matches!(result, Err(StakingError::InvalidLockPeriod))); +} + +#[test] +fn test_tier_for_one_second_above_a_tier_duration_is_invalid_not_next_higher_tier() { + let env = Env::default(); + let tier_list = tiers(&env); + + // One second past the 30-day tier does NOT round up to the 90-day tier. + let result = staking_vault::lock::tier_for(&tier_list, 30 * 86_400 + 1); + assert!(matches!(result, Err(StakingError::InvalidLockPeriod))); +} + +#[test] +fn test_tier_for_far_above_highest_tier_duration_is_invalid() { + let env = Env::default(); + let tier_list = tiers(&env); + + // Far above the highest (365-day) tier's duration still does not + // resolve to the highest tier under exact-match semantics. + let result = staking_vault::lock::tier_for(&tier_list, 10 * 365 * 86_400); + assert!(matches!(result, Err(StakingError::InvalidLockPeriod))); +} + +#[test] +fn test_stake_at_each_exact_tier_boundary_applies_that_tiers_boost() { + let env = Env::default(); + env.mock_all_auths(); + let (client, _admin, _fee_source, _token, asset) = setup(&env); + + // 30-day tier: 1.0x boost. + let staker_30 = Address::generate(&env); + asset.mint(&staker_30, &1_000_000); + client.stake(&staker_30, &1_000, &(30 * 86_400)); + assert_eq!(client.get_position(&staker_30).unwrap().shares, 1_000); + + // 365-day tier: 2.0x boost. + let staker_365 = Address::generate(&env); + asset.mint(&staker_365, &1_000_000); + client.stake(&staker_365, &1_000, &(365 * 86_400)); + assert_eq!(client.get_position(&staker_365).unwrap().shares, 2_000); +}