fix: validate passing score in reward claim retry path#57
Conversation
DeFiVC
left a comment
There was a problem hiding this comment.
PR Review: fix: validate passing score in reward claim retry path
Author: Danielodingz | Closes: #31 | CI: ❌ Test FAILURE | Conflicts: ✅ None
Blocking Issues
❌ CI Test Failure — The new if (score < 70) { return true; } guard at src/modules/rewards/reward.service.ts:67 causes existing tests to fail. The test suite in tests/unit/services/process-reward-claim.test.ts uses score: 5 throughout (lines 89, 106, 126, 148, 186, 224). With the new early return, these tests now short-circuit before reaching the code paths they're meant to test.
Failing tests:
- "should successfully process claim and update DB in transaction" (line 152) — expects
mockDb.transactionto be called, but the function now returns early at the score check - "should throw when on-chain transaction fails" (line 197) — expects an error to be thrown, but the function now returns
trueearly
Fix required: Update test data to use passing scores (>= 70) for tests that expect the function to proceed past the threshold check. Add a new test specifically for the score < 70 early return.
Code Issues
1. Hardcoded magic number — src/modules/rewards/reward.service.ts:67
score < 70 uses a magic number. The codebase already defines PASSING_PERCENTAGE = 70 in src/modules/quizzes/quiz.service.ts:20. Extract this to a shared constant (e.g., in src/config/index.ts or a shared thresholds.ts) to keep the threshold in one place.
Minor Nits
- The
scoreparameter inprocessRewardClaimis the quiz score passed from the retry job. Consider adding a JSDoc note clarifying that this is validated against the passing threshold, since the function signature doesn't make this constraint obvious.
What's Good
- The fix correctly addresses #31 — prevents non-passing submissions from being processed via the retry path
- Early return is the right approach (fail fast before DB queries)
- Placed at the top of the function, before any expensive operations
- The logic is correct:
return truemeans "claim succeeded / no retry needed" which is appropriate for dropping invalid jobs
The core fix is sound but the tests must be updated to match the new behavior.
DeFiVC
left a comment
There was a problem hiding this comment.
This PR adds an early return for scores below 70% in processRewardClaim, but the check is placed before the database lookup, breaking existing tests.
Blocking Issues
- CI Test Failure (2/81 tests fail) — The early return at
src/modules/rewards/reward.service.ts:67-69fires before the DB lookup, causing two existing tests to fail:should successfully process claim and update DB in transaction(line 196):mockDb.transactionis never called because the function short-circuitsshould throw when on-chain transaction fails(line 232): resolvestrueinstead of rejecting
Code Issues
-
Placement of the check is wrong (
reward.service.ts:67-69). Theif (score < 70) return trueis placed before the submission DB lookup on line 71. This validates the caller-providedscoreparameter, not the actual submission score stored in the database. It also skips the existingsubmission.rewardClaimedguard at line 72-74. -
The check should use
submission.scorefrom the database after the submission lookup, not thescoreparameter. The issue says: "It uses thescoreparameter directly from the retry job without validating that the quiz was actually passed." The fix should validate the stored score, not the parameter score. -
No tests added for the new validation path. The existing tests all pass
score: 5, so the new branch is untested.
Minor Nits
- Extract the hardcoded
70threshold to a constant (e.g.,PASSING_THRESHOLD = 70).
What's Good
- The intent is correct — the retry path lacked a passing-score guard.
- The change is minimal and focused on the right file.
- Lint and typecheck passed in CI.
Please move the check after the submission DB lookup, validate against submission.score, and update/add tests to cover the new logic.
Closes #31
Fix: Validate passing threshold for reward claims
Description
This PR addresses an issue where the
processRewardClaimlogic did not validate if a quiz submission actually met the passing threshold. Prior to this fix, if a retry job for a non-passing submission was pushed to the queue (e.g., due to a bug or manual queue manipulation), the reward could be incorrectly processed.This update introduces an early return check to ensure that if the score is below the passing threshold (70%), the reward claim process halts immediately without initiating the payout.
Changes Made
src/modules/rewards/reward.service.ts: Added an early return conditionif (score < 70)in theprocessRewardClaimfunction to drop any invalid retry jobs safely.Impact
Secures the reward claim retry path by strictly enforcing the passing requirement, preventing any accidental bypassing of the threshold requirement for on-chain rewards.