Wire distribution event handlers to persistence and add API tests - #80
Conversation
…ests Indexer (#53): Wire distribution event handlers to durable persistence - Add DistributionBatch and ClaimAction entities plus an initial migration - Add DistributionRepository with explicit createBatch/recordClaim/setStatus APIs - Convert distribution handlers to repository-backed factories that: - persist created batches after validating required payload fields - record claims without double-counting via a unique claim event identity - apply paused/resumed status changes - coordinate indexed event identity through the shared EventRepository so replayed events are idempotent - Replace the persistence TODOs with real writes - Add repository-backed handler tests and a repository unit test covering created, claimed, paused, resumed, invalid payload, and duplicate replay API (#55): Distribution endpoint tests and consistent error responses - Align controller responses with shared sendSuccess/sendError helpers - Return 503 DB_NOT_READY and 404 DISTRIBUTION_NOT_FOUND instead of raw 500s - Replace console.* with the project logger; add a coded not-found error - Make the controller service resolver injectable for testing - Add controller, service, and validation tests covering success, validation, not-found, db-not-ready, and internal-error paths Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bjxdi78eokeEiy3MG9EuVy
…ence-hq3gzb feat(distributions): wire event handlers to persistence and add API tests
|
@vickydve 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! 🚀 |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds a full TypeORM persistence layer (entities, migration, repository) for distribution batches and claim actions in the indexer; rewires all four distribution event handlers into dependency-injected factories with idempotency gating; refactors the distribution API controller to use resolver injection and centralized error mapping; adds ChangesIndexer Distribution Persistence and Handler Wiring
Distribution API Controller/Service Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/v1/distribution/distribution.service.ts (1)
73-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize unexpected update failures to the same service contract.
createDistributionandlistDistributionstranslate repository failures into service-level errors, butupdateDistributionnow rethrows raw repository exceptions. That makes update the odd one out and undercuts the error-handling standardization this PR is adding. PreserveDistributionNotFoundError, then wrap everything else in a single"Failed to update distribution"error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/v1/distribution/distribution.service.ts` around lines 73 - 77, In updateDistribution, keep the existing DistributionNotFoundError passthrough, but normalize every other unexpected failure to the same service-level contract used by createDistribution and listDistributions. Update the error handling around the logger.error path so it logs the original exception details, then always throws a new Error with the message "Failed to update distribution" instead of rethrowing raw repository errors.src/components/v1/distribution/distribution.controller.ts (1)
23-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefer DB wiring so resolver injection actually decouples the controller.
src/__tests__/distribution.controller.test.ts, Lines 4-13 still have to seed DB env vars before this module can even be imported. That means the new resolver injection removes the live-connection requirement, but not the data-source bootstrap coupling. Moving theAppDataSource/entity lookup behind the default resolver path would make injected-controller tests fully standalone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/v1/distribution/distribution.controller.ts` around lines 23 - 33, The default resolver in distribution.controller still pulls in AppDataSource and DistributionEntity at module load, so controller tests cannot import the module without DB env setup. Move the data-source and repository lookup fully inside defaultDistributionServiceResolver so only the default path touches AppDataSource.getRepository(DistributionEntity), while injected resolver paths remain free of bootstrap coupling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indexer/distributions/src/db/migrations/00001_InitialDistributionsSchema.ts`:
- Around line 7-50: The migration for 00001_InitialDistributionsSchema uses
uuid_generate_v4() in distribution_claim_action without ensuring the uuid-ossp
extension exists, so a clean PostgreSQL setup will fail. Update the migration to
create/enable the UUID extension before the CREATE TABLE for
distribution_claim_action, ideally alongside the existing enum setup in the same
queryRunner.query block, and keep the table creation for distribution_batch and
distribution_claim_action unchanged otherwise.
In `@indexer/distributions/src/db/repository.ts`:
- Around line 29-34: `SetStatusInput.status` in `SetStatusInput` is too broad
for what `setStatus()` can actually persist, because the non-`PAUSED` branch in
the status mapping treats every other value as a resume event. Narrow the
accepted status type to only the states `setStatus()` can handle correctly, and
update the logic in `setStatus()` so `COMPLETED` and `CANCELLED` are either
rejected or mapped to their own persisted fields instead of `resumedAt`; also
make sure the `src/index.ts` re-export reflects the corrected contract.
In `@indexer/distributions/src/handlers/distribution-created.handler.ts`:
- Around line 40-46: The missing-total validation in distribution handling is
ineffective because parseDistributionCreated() still defaults a missing
total_amount to "0", so the handler can proceed with a fabricated value. Update
the parsing/validation flow in parseDistributionCreated() and the
distributionCreated handler so a raw missing total_amount is rejected before
createBatch is called, and ensure the payload.totalAmount check reflects the
actual presence of the field rather than a defaulted value.
In `@indexer/distributions/src/handlers/distribution-pause.handler.ts`:
- Around line 50-61: The pause/resume handlers in distribution-pause.handler.ts
and the matching resume path update status before calling
recordEventProcessed(), which can let a retried older event overwrite a newer
state after partial failure. Refactor the flow around the
setStatus/deps.distributions call and deps.events.recordEventProcessed so the
status change and event-processing mark happen atomically in one repository
operation, or extend SetStatusInput/setStatus to include ledger/event identity
and reject out-of-order updates using that metadata.
---
Nitpick comments:
In `@src/components/v1/distribution/distribution.controller.ts`:
- Around line 23-33: The default resolver in distribution.controller still pulls
in AppDataSource and DistributionEntity at module load, so controller tests
cannot import the module without DB env setup. Move the data-source and
repository lookup fully inside defaultDistributionServiceResolver so only the
default path touches AppDataSource.getRepository(DistributionEntity), while
injected resolver paths remain free of bootstrap coupling.
In `@src/components/v1/distribution/distribution.service.ts`:
- Around line 73-77: In updateDistribution, keep the existing
DistributionNotFoundError passthrough, but normalize every other unexpected
failure to the same service-level contract used by createDistribution and
listDistributions. Update the error handling around the logger.error path so it
logs the original exception details, then always throws a new Error with the
message "Failed to update distribution" instead of rethrowing raw repository
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9615cccd-81d5-475f-a4d3-7119496b0b74
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
indexer/common/src/handlers/registry.test.tsindexer/common/src/handlers/registry.tsindexer/common/src/handlers/types.tsindexer/distributions/package.jsonindexer/distributions/src/db/entity/ClaimAction.tsindexer/distributions/src/db/entity/DistributionBatch.tsindexer/distributions/src/db/migrations/00001_InitialDistributionsSchema.tsindexer/distributions/src/db/repository.test.tsindexer/distributions/src/db/repository.tsindexer/distributions/src/handlers/distribution-created.handler.tsindexer/distributions/src/handlers/distribution-handlers.test.tsindexer/distributions/src/handlers/distribution-pause.handler.tsindexer/distributions/src/handlers/index.tsindexer/distributions/src/handlers/persistence.tsindexer/distributions/src/handlers/tokens-claimed.handler.tsindexer/distributions/src/handlers/types.tsindexer/distributions/src/index.tsindexer/streams/src/handlers/stream-cancel.handler.tsindexer/streams/src/handlers/stream-funded.handler.tsindexer/streams/src/handlers/stream-withdrawal.handler.tssrc/__tests__/distribution.controller.test.tssrc/__tests__/distribution.service.test.tssrc/__tests__/distribution.validation.test.tssrc/components/v1/distribution/distribution.controller.tssrc/components/v1/distribution/distribution.service.tssrc/components/v1/distribution/distrubtion.routes.ts
|
Hi @vickydve Thank you for your awesome contribution, however after analyzing your implementation, there are some minor fixes to be done. Kindly fix them to merge your PR asap. Also do not forget to use fundable.finance to offramp. |
- migration: create the uuid-ossp extension before tables that use uuid_generate_v4(), so the schema applies on a clean database - repository: narrow setStatus to ACTIVE/PAUSED so COMPLETED/CANCELLED can no longer be passed as type-valid resume writes - repository: guard status updates with a statusLedger column so a stale (out-of-order) pause/resume cannot overwrite newer state after a retry - handlers: drop the total_amount "0" parser default so a missing amount is rejected instead of persisting a fabricated total - pass the event ledger into setStatus and cover the new paths in tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bjxdi78eokeEiy3MG9EuVy
…ence-hq3gzb fix(distributions): address review feedback on persistence layer
Summary
Two scoped changes for the distributions domain:
Indexer (#53) — wire distribution event handlers to persistence
DistributionBatchandClaimActionTypeORM entities plus an initial migration (indexer/distributions/src/db/).DistributionRepositorywith explicitcreateBatch/recordClaim/setStatusAPIs.EventRepositoryso replayed events are idempotent.TODOs.Backend API (#55) — distribution endpoint tests and consistent error responses
sendSuccess/sendErrorhelpers.503 DB_NOT_READYand404 DISTRIBUTION_NOT_FOUNDinstead of raw 500s; add a coded not-found error in the service.console.*with the projectlogger.Area
src/)indexer/common/)indexer/streams/)indexer/distributions/)Scope
Verification
bun run type-checkbun run test— all 58 tests pass; see note on the pre-existing coverage gatebun run lintbun run indexer:type-checkbun run indexer:testbun run indexer:lintIndexer Safety
Idempotency is enforced via the shared
indexed_eventidentity gate plus a unique(txHash, ledgerNumber, eventIndex)constraint on claims. Handler tests cover created, claimed, paused, resumed, invalid payload, and duplicate replay paths.Notes
Closes #53
Closes #55
Test coverage caveat: all 58 backend tests pass, but
bun run testenforces a global 90% coverage threshold that is already red ondev(~74%) due to low coverage in unrelatedcampaignfiles (campaign.entity.ts,campaign.service.ts). The new distribution service/controller/validation tests are thorough (distribution code sits at ~94% when measured), and consistent with the donation domain precedent they were not added to the sharedc8.include. Raising the global threshold to green would require campaign-domain work outside the scope of these two issues.Summary by CodeRabbit
New Features
Bug Fixes
Tests