Skip to content

fix: validate passing score in reward claim retry path#57

Open
Danielodingz wants to merge 3 commits into
ChainLearnOfficial:mainfrom
Danielodingz:fix/reward-claim-passing-threshold
Open

fix: validate passing score in reward claim retry path#57
Danielodingz wants to merge 3 commits into
ChainLearnOfficial:mainfrom
Danielodingz:fix/reward-claim-passing-threshold

Conversation

@Danielodingz

Copy link
Copy Markdown
Contributor

Closes #31

Fix: Validate passing threshold for reward claims

Description

This PR addresses an issue where the processRewardClaim logic 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 condition if (score < 70) in the processRewardClaim function 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.

@DeFiVC DeFiVC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.transaction to 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 true early

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 numbersrc/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 score parameter in processRewardClaim is 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 true means "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 DeFiVC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-69 fires before the DB lookup, causing two existing tests to fail:
    • should successfully process claim and update DB in transaction (line 196): mockDb.transaction is never called because the function short-circuits
    • should throw when on-chain transaction fails (line 232): resolves true instead of rejecting

Code Issues

  1. Placement of the check is wrong (reward.service.ts:67-69). The if (score < 70) return true is placed before the submission DB lookup on line 71. This validates the caller-provided score parameter, not the actual submission score stored in the database. It also skips the existing submission.rewardClaimed guard at line 72-74.

  2. The check should use submission.score from the database after the submission lookup, not the score parameter. The issue says: "It uses the score parameter directly from the retry job without validating that the quiz was actually passed." The fix should validate the stored score, not the parameter score.

  3. 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 70 threshold 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[HIGH] processRewardClaim retry path doesn't validate passing score

2 participants