feat: implement donations API with campaign title support#47
Open
Skinny001 wants to merge 2 commits into
Open
Conversation
- Add DonationEntity, service, controller, routes, validation, DTO - Add campaign title column to CampaignEntity and donation.campaign_title - Support filtering (date range, amount, status, confirmed, campaign, donor) - Support sorting (created_at, amount, status, confirmed_at, campaign_ref, campaign_title, donor_address) - Support search across donorAddress, donorName, campaignRef, campaignTitle - Add pagination with stats aggregation endpoint - Add admin guard middleware for GET /api/v1/donations/ - Register spec-compliant routes under /campaigns/:id/donations and /users/:userId/donations - Add 2 migrations for donations table and title columns - 33 tests covering service, validation, edge cases
Contributor
📝 WalkthroughWalkthroughAdds a donation API stack with validation schemas, entity and migration support, service methods for create/list/filter/stats, controller and route wiring for authenticated/admin endpoints, campaign title support in the campaign module, and unit tests covering donation service and schema behavior. ChangesDonation API and campaign title changes
Sequence Diagram(s)Donation request flow sequenceDiagram
participant Routes as donation.routes
participant Controller as donation.controller
participant Service as DonationService
participant Repo as donationRepository
Routes->>Controller: validated donation request
Controller->>Service: create/list/get/stats
Service->>Repo: save / findOne / createQueryBuilder
Repo-->>Service: entities / raw aggregates
Service-->>Controller: DTOs / paginated data / stats
Controller-->>Routes: sendSuccess / sendError
Campaign title create flow sequenceDiagram
participant Controller as campaign.controller
participant Service as CampaignService
participant CairoClient as cairoClient
participant Repo as campaignRepository
Controller->>Service: createCampaign(parsed.title)
Service->>CairoClient: createCampaign(...)
CairoClient-->>Service: transaction details
Service->>Repo: save campaign with title
Repo-->>Service: saved campaign
Service-->>Controller: campaign response with title
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
@Skinny001 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! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@src/__tests__/donation.service.test.ts`:
- Around line 8-10: The query-builder mock in donation.service tests is only
parsing “=” clauses via extractColName, so `!=`, `>=`, `<=`, and `ILIKE` are
being simulated incorrectly and can produce false positives. Update the mocked
`where`/filter handling in `donation.service.test.ts` to evaluate each clause as
a real predicate over the in-memory rows instead of inferring a column name, and
make sure the tests around `confirmed`, date range, amount range, and search
paths use the same predicate-based behavior.
In `@src/components/v1/campaign/campaign.service.ts`:
- Around line 102-110: The campaignCount increment in CampaignService is
currently read-modify-write and can lose updates under concurrent requests.
Update the logic in the campaign.service.ts flow to perform the increment
atomically in the database, using the userRepository update path instead of
first calling findOne and computing nextCount in memory. Keep the fix localized
around the userRepository.update call so it applies COALESCE(campaign_count, 0)
+ 1 in one SQL statement.
- Around line 37-86: The campaign creation flow in campaign.service.ts is racy
because `findOne()` is only a precheck and `cairoClient.createCampaign()` can
run before `campaign_ref` is durably reserved. Change the `createCampaign` path
to reserve the ref/idempotency key in the database first, then call
`cairoClient.createCampaign`, and keep the final save guarded by the unique
`campaign_ref` constraint. In the duplicate path, translate the unique-violation
handling in `CampaignService` to the duplicate 409 instead of letting it bubble
as a generic error, and preserve the existing error codes for the other early
validation branches.
In `@src/components/v1/campaign/campaign.validation.ts`:
- Around line 34-38: The campaign title validation currently allows
whitespace-only input because `campaign.validation.ts` uses `title` with
`z.string().trim().max(255).optional()`, which can normalize `" "` into an
empty string. Update the `title` schema in the validation object to reject empty
strings after trimming, or convert trimmed empty strings to `undefined`, so the
field is represented consistently and does not persist as `""` in addition to
missing/null.
In `@src/components/v1/Donation/donation.controller.ts`:
- Around line 28-35: The createDonation handler is trusting client-provided
donor ownership, so update the createDonation flow in donation.controller to use
IRequest and derive donorId from req.auth.userId instead of forwarding donorId
from req.body. Keep the request body for donation fields only, and if a
wallet/address is still accepted, verify it against the authenticated user
before using it. Make sure the createDonation call receives the authenticated
donor identity consistently so /users/:userId/donations and /users/me/donations
can’t be polluted by spoofed IDs.
In `@src/components/v1/Donation/donation.entity.ts`:
- Around line 44-45: The DonationEntity.blockNumber field is typed as
number|null even though the bigint column hydrates as a string at runtime, so
update the end-to-end typing to match the actual value or add a transformer.
Adjust DonationEntity.blockNumber and DonationResponseDto.blockNumber together,
and if you choose normalization, apply it in the entity mapping/transformer so
callers receive a consistent value from the Donation flow.
In `@src/components/v1/Donation/donation.routes.ts`:
- Line 33: The /stats route in the donation router is currently the only global
read endpoint that bypasses auth, so protect it consistently with the other
admin-only donation routes. Update the routing setup in donation.routes.ts
around router.get('/stats', getDonationStats) to apply the same
requireJwtAuthApi and requireAdminApi guards used for GET /donations, or move
public access to a separately reviewed campaign-scoped stats handler.
In `@src/components/v1/Donation/donation.service.ts`:
- Around line 269-294: The DonationService.formatResponse method is exposing
donor identity fields even when isAnonymous is true, so public donation
endpoints are still deanonymizing anonymous donations. Update the response
shaping in DonationService/formatResponse so donorAddress, donorName, and
message are redacted or omitted for public callers when the donation is
anonymous, and only included for the donor or admin views. Consider splitting
the DTO mapping into public and admin variants, or gating the sensitive fields
on the caller’s role/context while keeping the rest of DonationResponseDto
unchanged.
In `@src/components/v1/Donation/donation.validation.ts`:
- Around line 51-63: The Donation validation schema currently only checks for
decimal strings, but amount, usdAmount, and gasFee must also respect the
database’s decimal(65,30) limits. Update the zod rules in donation.validation.ts
for these fields to enforce max precision and scale (and any required
sign/format constraints) so values accepted by the request schema cannot exceed
what the persistence layer can store. Keep the change localized to the donation
schema definitions for amount, usdAmount, and gasFee.
In `@src/components/v1/routes.api.v1.ts`:
- Around line 33-38: The user donations endpoints are missing an
ownership/access check, so any authenticated user can query another user’s
donation history. Update the route wiring in routes.api.v1.ts for both the
users/:userId/donations and donations/users/:userId paths to enforce that
req.params.userId matches req.auth.userId unless the caller has admin
privileges, ideally via a shared authorization middleware before
getUserDonations. Keep policyMiddleware and requireJwtAuthApi, but add the same
self-or-admin guard to both route definitions so the restriction is consistent.
In `@src/migrations/CreateDonationsTable1760000000002.js`:
- Around line 19-43: The donations migration currently adds only a normal index
for transaction_hash, which does not prevent duplicate on-chain donations from
being inserted on retries. Update CreateDonationsTable1760000000002 so
transaction_hash is enforced as unique in the donations table, and adjust the
index/constraint definition accordingly using the existing migration/queryRunner
setup to preserve idempotency for donation creation and aggregate stats.
- Around line 39-43: The donation table migration is missing indexes for the new
primary filter columns, so update CreateDonationsTable1760000000002 to add
indexes for donor_id and donation_token alongside the existing create index
calls in the migration’s up path. Make sure the corresponding down path in the
same migration removes those indexes as well, and keep the changes aligned with
the existing queryRunner.query patterns used for donations_campaign_id_idx and
donations_donor_address_idx.
🪄 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: f1b160b7-adb6-478c-9a17-728e8c824a9f
📒 Files selected for processing (19)
src/__tests__/donation.service.test.tssrc/__tests__/donation.validation.test.tssrc/appMiddlewares/jwtAuth.api.tssrc/components/v1/Donation/donation.controller.tssrc/components/v1/Donation/donation.dto.tssrc/components/v1/Donation/donation.entity.tssrc/components/v1/Donation/donation.routes.tssrc/components/v1/Donation/donation.service.tssrc/components/v1/Donation/donation.validation.tssrc/components/v1/campaign/campaign.controller.tssrc/components/v1/campaign/campaign.entity.tssrc/components/v1/campaign/campaign.routes.tssrc/components/v1/campaign/campaign.service.tssrc/components/v1/campaign/campaign.validation.tssrc/components/v1/routes.api.v1.tssrc/config/persistence/data-source.tssrc/migrations/AddCampaignTitleColumns1760000000003.jssrc/migrations/CreateDonationsTable1760000000002.jssrc/types/enums.ts
Comment on lines
+8
to
+10
| const extractColName = (clause: string): string | null => { | ||
| const match = clause.match(/\.(\w+)\s*=/); | ||
| return match ? match[1] : null; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The mock query builder only simulates = filters.
extractColName() does not recognize !=, >=, <=, or ILIKE, so those clauses degrade into fake key checks like confirmedStatus and the tests pass for the wrong reason. The current confirmed=false assertion is a false positive, and the date/amount/search paths are not being exercised realistically. Model each where as a predicate instead of guessing a column name from the SQL string.
Also applies to: 54-63, 76-148
🤖 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/__tests__/donation.service.test.ts` around lines 8 - 10, The
query-builder mock in donation.service tests is only parsing “=” clauses via
extractColName, so `!=`, `>=`, `<=`, and `ILIKE` are being simulated incorrectly
and can produce false positives. Update the mocked `where`/filter handling in
`donation.service.test.ts` to evaluate each clause as a real predicate over the
in-memory rows instead of inferring a column name, and make sure the tests
around `confirmed`, date range, amount range, and search paths use the same
predicate-based behavior.
Comment on lines
+37
to
+86
| const existing = await this.campaignRepository.findOne({ | ||
| where: { campaignRef }, | ||
| }); | ||
| if (existing) { | ||
| const error = new Error('Duplicate campaign_ref'); | ||
| (error as any).code = 'DUPLICATE_CAMPAIGN_REF'; | ||
| throw error; | ||
| } | ||
|
|
||
| if (!walletAddress) { | ||
| const error = new Error("Missing wallet address in token claims") | ||
| ;(error as any).code = "MISSING_WALLET_ADDRESS" | ||
| throw error | ||
| } | ||
| if (!walletAddress) { | ||
| const error = new Error('Missing wallet address in token claims'); | ||
| (error as any).code = 'MISSING_WALLET_ADDRESS'; | ||
| throw error; | ||
| } | ||
|
|
||
| const wallet = await this.walletRepository.findOne({ where: { address: walletAddress } }) | ||
| const wallet = await this.walletRepository.findOne({ | ||
| where: { address: walletAddress }, | ||
| }); | ||
|
|
||
| if (!wallet) { | ||
| const error = new Error("Wallet not found") | ||
| ;(error as any).code = "WALLET_NOT_FOUND" | ||
| throw error | ||
| } | ||
| if (!wallet) { | ||
| const error = new Error('Wallet not found'); | ||
| (error as any).code = 'WALLET_NOT_FOUND'; | ||
| throw error; | ||
| } | ||
|
|
||
| const walletBalance = Number.parseFloat(wallet.balance ?? "0") | ||
| if (!Number.isFinite(walletBalance) || walletBalance <= 0) { | ||
| const error = new Error("Insufficient wallet balance for fees") | ||
| ;(error as any).code = "INSUFFICIENT_BALANCE" | ||
| throw error | ||
| } | ||
| const walletBalance = Number.parseFloat(wallet.balance ?? '0'); | ||
| if (!Number.isFinite(walletBalance) || walletBalance <= 0) { | ||
| const error = new Error('Insufficient wallet balance for fees'); | ||
| (error as any).code = 'INSUFFICIENT_BALANCE'; | ||
| throw error; | ||
| } | ||
|
|
||
| const { transactionHash, campaignId } = await this.cairoClient.createCampaign({ | ||
| campaignRef, | ||
| targetAmount, | ||
| donationToken, | ||
| }) | ||
| const { transactionHash, campaignId } = | ||
| await this.cairoClient.createCampaign({ | ||
| campaignRef, | ||
| targetAmount, | ||
| donationToken, | ||
| }); | ||
|
|
||
| const saved = await this.campaignRepository.save( | ||
| this.campaignRepository.create({ | ||
| campaignId, | ||
| userId, | ||
| campaignRef, | ||
| targetAmount, | ||
| donationToken, | ||
| transactionHash, | ||
| }) | ||
| ) | ||
| const saved = await this.campaignRepository.save( | ||
| this.campaignRepository.create({ | ||
| campaignId, | ||
| userId, | ||
| campaignRef, | ||
| targetAmount, | ||
| donationToken, | ||
| transactionHash, | ||
| title: title ?? null, | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Don't call Cairo before campaign_ref is durably reserved.
This findOne() check is racy. Two concurrent requests can both miss here and both reach cairoClient.createCampaign() before the unique campaign_ref constraint rejects one save. The losing request then falls into the generic 500 path in campaign.controller.ts, and any successful Cairo side effect from that request is no longer represented in the database.
Reserve the ref/idempotency key in the database first, then call Cairo, and translate the eventual unique-violation path to the duplicate 409.
🤖 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/campaign/campaign.service.ts` around lines 37 - 86, The
campaign creation flow in campaign.service.ts is racy because `findOne()` is
only a precheck and `cairoClient.createCampaign()` can run before `campaign_ref`
is durably reserved. Change the `createCampaign` path to reserve the
ref/idempotency key in the database first, then call
`cairoClient.createCampaign`, and keep the final save guarded by the unique
`campaign_ref` constraint. In the duplicate path, translate the unique-violation
handling in `CampaignService` to the duplicate 409 instead of letting it bubble
as a generic error, and preserve the existing error codes for the other early
validation branches.
Comment on lines
+102
to
+110
| const user = await this.userRepository.findOne({ | ||
| where: { id: userId }, | ||
| }); | ||
| if (user) { | ||
| const nextCount = (user.campaignCount ?? 0) + 1; | ||
| await this.userRepository.update( | ||
| { id: userId }, | ||
| { campaignCount: nextCount } | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use an atomic update for campaignCount.
This read-then-write drops increments when the same user creates campaigns concurrently: both requests can read the same value and both write back the same nextCount. Update the column in one SQL statement (COALESCE(campaign_count, 0) + 1) instead of loading it first.
🤖 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/campaign/campaign.service.ts` around lines 102 - 110, The
campaignCount increment in CampaignService is currently read-modify-write and
can lose updates under concurrent requests. Update the logic in the
campaign.service.ts flow to perform the increment atomically in the database,
using the userRepository update path instead of first calling findOne and
computing nextCount in memory. Keep the fix localized around the
userRepository.update call so it applies COALESCE(campaign_count, 0) + 1 in one
SQL statement.
Comment on lines
+34
to
+38
| title: z | ||
| .string() | ||
| .trim() | ||
| .max(255, 'title must be at most 255 characters') | ||
| .optional(), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject whitespace-only titles.
trim().max(255).optional() still accepts " " and turns it into "". That then gets persisted as an empty string, so the new field represents “missing title” two different ways (null and ""). Add a non-empty check after trim() or normalize "" to undefined.
Suggested fix
title: z
.string()
.trim()
+ .min(1, 'title cannot be empty')
.max(255, 'title must be at most 255 characters')
.optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| title: z | |
| .string() | |
| .trim() | |
| .max(255, 'title must be at most 255 characters') | |
| .optional(), | |
| title: z | |
| .string() | |
| .trim() | |
| .min(1, 'title cannot be empty') | |
| .max(255, 'title must be at most 255 characters') | |
| .optional(), |
🤖 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/campaign/campaign.validation.ts` around lines 34 - 38, The
campaign title validation currently allows whitespace-only input because
`campaign.validation.ts` uses `title` with
`z.string().trim().max(255).optional()`, which can normalize `" "` into an
empty string. Update the `title` schema in the validation object to reject empty
strings after trimming, or convert trimmed empty strings to `undefined`, so the
field is represented consistently and does not persist as `""` in addition to
missing/null.
Comment on lines
+28
to
+35
| export const createDonation = async ( | ||
| req: Request, | ||
| res: Response | ||
| ): Promise<void> => { | ||
| try { | ||
| const service = getService(); | ||
| const data = req.body as CreateDonationInput; | ||
| const donation = await service.createDonation(data); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t trust client-supplied donor ownership on an authenticated create route.
POST /donations is JWT-protected, but this handler ignores req.auth and forwards donorId unchanged from the body. A caller can create donations under another user's id and pollute /users/:userId/donations and /users/me/donations. Make this an IRequest, derive donorId from req.auth.userId, and only accept a caller-supplied wallet/address if you can verify it.
🤖 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/Donation/donation.controller.ts` around lines 28 - 35, The
createDonation handler is trusting client-provided donor ownership, so update
the createDonation flow in donation.controller to use IRequest and derive
donorId from req.auth.userId instead of forwarding donorId from req.body. Keep
the request body for donation fields only, and if a wallet/address is still
accepted, verify it against the authenticated user before using it. Make sure
the createDonation call receives the authenticated donor identity consistently
so /users/:userId/donations and /users/me/donations can’t be polluted by spoofed
IDs.
Comment on lines
+269
to
+294
| private formatResponse(entity: DonationEntity): DonationResponseDto { | ||
| return { | ||
| id: entity.id, | ||
| campaignId: entity.campaignId, | ||
| campaignRef: entity.campaignRef, | ||
| campaignTitle: entity.campaignTitle, | ||
| donorId: entity.donorId, | ||
| donorAddress: entity.donorAddress, | ||
| donorName: entity.donorName, | ||
| transactionHash: entity.transactionHash, | ||
| blockNumber: entity.blockNumber, | ||
| blockTimestamp: entity.blockTimestamp, | ||
| gasFee: entity.gasFee, | ||
| amount: entity.amount, | ||
| usdAmount: entity.usdAmount, | ||
| tokenAddress: entity.tokenAddress, | ||
| tokenSymbol: entity.tokenSymbol, | ||
| tokenDecimals: entity.tokenDecimals, | ||
| status: entity.status, | ||
| confirmedAt: entity.confirmedAt, | ||
| isAnonymous: entity.isAnonymous, | ||
| message: entity.message, | ||
| network: entity.network, | ||
| createdAt: entity.createdAt, | ||
| updatedAt: entity.updatedAt, | ||
| }; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Anonymous donations are still fully deanonymized.
This formatter always returns donorAddress, donorName, and message, but the campaign donation endpoints are public. For isAnonymous = true, public callers still get the donor identity, so the anonymity flag currently has no effect. Split public/admin response shaping or redact identity fields unless the caller is the donor or an admin.
🤖 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/Donation/donation.service.ts` around lines 269 - 294, The
DonationService.formatResponse method is exposing donor identity fields even
when isAnonymous is true, so public donation endpoints are still deanonymizing
anonymous donations. Update the response shaping in
DonationService/formatResponse so donorAddress, donorName, and message are
redacted or omitted for public callers when the donation is anonymous, and only
included for the donor or admin views. Consider splitting the DTO mapping into
public and admin variants, or gating the sensitive fields on the caller’s
role/context while keeping the rest of DonationResponseDto unchanged.
Comment on lines
+51
to
+63
| amount: z | ||
| .string() | ||
| .regex(decimalStringRegex, 'amount must be a valid decimal string'), | ||
|
|
||
| usdAmount: z | ||
| .string() | ||
| .regex(decimalStringRegex, 'usdAmount must be a valid decimal string') | ||
| .optional(), | ||
|
|
||
| gasFee: z | ||
| .string() | ||
| .regex(decimalStringRegex, 'gasFee must be a valid decimal string') | ||
| .optional(), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the DB decimal bounds in the request schema.
amount, usdAmount, and gasFee currently accept arbitrarily large/precise decimal strings, but the migration persists them as decimal(65,30). Values that pass this schema can still fail at the database boundary or get truncated unexpectedly.
Suggested fix
const decimalStringRegex = /^\d+(\.\d+)?$/;
+const fitsDecimal6530 = (value: string) => {
+ if (!decimalStringRegex.test(value)) return false;
+ const [whole, fraction = ''] = value.split('.');
+ return whole.length + fraction.length <= 65 && fraction.length <= 30;
+};
+
+const decimal6530 = (field: string) =>
+ z
+ .string()
+ .refine(fitsDecimal6530, `${field} must fit in DECIMAL(65,30)`);
export const createDonationSchema = z.object({
@@
- amount: z
- .string()
- .regex(decimalStringRegex, 'amount must be a valid decimal string'),
+ amount: decimal6530('amount'),
- usdAmount: z
- .string()
- .regex(decimalStringRegex, 'usdAmount must be a valid decimal string')
- .optional(),
+ usdAmount: decimal6530('usdAmount').optional(),
- gasFee: z
- .string()
- .regex(decimalStringRegex, 'gasFee must be a valid decimal string')
- .optional(),
+ gasFee: decimal6530('gasFee').optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| amount: z | |
| .string() | |
| .regex(decimalStringRegex, 'amount must be a valid decimal string'), | |
| usdAmount: z | |
| .string() | |
| .regex(decimalStringRegex, 'usdAmount must be a valid decimal string') | |
| .optional(), | |
| gasFee: z | |
| .string() | |
| .regex(decimalStringRegex, 'gasFee must be a valid decimal string') | |
| .optional(), | |
| const decimalStringRegex = /^\d+(\.\d+)?$/; | |
| const fitsDecimal6530 = (value: string) => { | |
| if (!decimalStringRegex.test(value)) return false; | |
| const [whole, fraction = ''] = value.split('.'); | |
| return whole.length + fraction.length <= 65 && fraction.length <= 30; | |
| }; | |
| const decimal6530 = (field: string) => | |
| z | |
| .string() | |
| .refine(fitsDecimal6530, `${field} must fit in DECIMAL(65,30)`); | |
| export const createDonationSchema = z.object({ | |
| amount: decimal6530('amount'), | |
| usdAmount: decimal6530('usdAmount').optional(), | |
| gasFee: decimal6530('gasFee').optional(), |
🤖 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/Donation/donation.validation.ts` around lines 51 - 63, The
Donation validation schema currently only checks for decimal strings, but
amount, usdAmount, and gasFee must also respect the database’s decimal(65,30)
limits. Update the zod rules in donation.validation.ts for these fields to
enforce max precision and scale (and any required sign/format constraints) so
values accepted by the request schema cannot exceed what the persistence layer
can store. Keep the change localized to the donation schema definitions for
amount, usdAmount, and gasFee.
Comment on lines
+33
to
+38
| router.get( | ||
| '/users/:userId/donations', | ||
| requireJwtAuthApi, | ||
| policyMiddleware(listUserDonationsQuerySchema, 'query'), | ||
| getUserDonations | ||
| ); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
This route leaks any user’s donation history to any logged-in user.
requireJwtAuthApi only authenticates; it does not ensure req.params.userId === req.auth.userId. As written, one user can call /users/{otherUserId}/donations and read that account’s donations. Restrict this to self/admin access, and keep the same rule on the duplicate /donations/users/:userId route.
🤖 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/routes.api.v1.ts` around lines 33 - 38, The user donations
endpoints are missing an ownership/access check, so any authenticated user can
query another user’s donation history. Update the route wiring in
routes.api.v1.ts for both the users/:userId/donations and
donations/users/:userId paths to enforce that req.params.userId matches
req.auth.userId unless the caller has admin privileges, ideally via a shared
authorization middleware before getUserDonations. Keep policyMiddleware and
requireJwtAuthApi, but add the same self-or-admin guard to both route
definitions so the restriction is consistent.
Comment on lines
+19
to
+43
| "transaction_hash" text, | ||
| "block_number" bigint, | ||
| "block_timestamp" TIMESTAMP(3), | ||
| "gas_fee" decimal(65,30) DEFAULT '0', | ||
| "amount" decimal(65,30) NOT NULL, | ||
| "usd_amount" decimal(65,30) DEFAULT '0', | ||
| "token_address" text NOT NULL, | ||
| "token_symbol" text NOT NULL, | ||
| "token_decimals" integer NOT NULL, | ||
| "status" "donation_status" NOT NULL DEFAULT 'pending', | ||
| "confirmed_at" TIMESTAMP(3), | ||
| "is_anonymous" boolean NOT NULL DEFAULT false, | ||
| "message" text, | ||
| "network" "network" NOT NULL DEFAULT 'mainnet', | ||
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "updated_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| CONSTRAINT "PK_donations_id" PRIMARY KEY ("id") | ||
| ) | ||
| `) | ||
|
|
||
| await queryRunner.query(`CREATE INDEX "donations_campaign_id_idx" ON "donations" ("campaign_id")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_donor_address_idx" ON "donations" ("donor_address")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_status_idx" ON "donations" ("status")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_created_at_idx" ON "donations" ("created_at")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_transaction_hash_idx" ON "donations" ("transaction_hash")`) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make transaction_hash enforce idempotency.
A plain index here does not stop the same on-chain donation from being inserted twice. Since this PR also adds donation creation plus aggregate stats, duplicate rows will overcount totals and donor metrics on request retries.
Suggested fix
- await queryRunner.query(`CREATE INDEX "donations_transaction_hash_idx" ON "donations" ("transaction_hash")`)
+ await queryRunner.query(`
+ CREATE UNIQUE INDEX "donations_transaction_hash_uidx"
+ ON "donations" ("transaction_hash")
+ WHERE "transaction_hash" IS NOT NULL
+ `)
@@
- await queryRunner.query(`DROP INDEX IF EXISTS "donations_transaction_hash_idx"`)
+ await queryRunner.query(`DROP INDEX IF EXISTS "donations_transaction_hash_uidx"`)🤖 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/migrations/CreateDonationsTable1760000000002.js` around lines 19 - 43,
The donations migration currently adds only a normal index for transaction_hash,
which does not prevent duplicate on-chain donations from being inserted on
retries. Update CreateDonationsTable1760000000002 so transaction_hash is
enforced as unique in the donations table, and adjust the index/constraint
definition accordingly using the existing migration/queryRunner setup to
preserve idempotency for donation creation and aggregate stats.
Comment on lines
+39
to
+43
| await queryRunner.query(`CREATE INDEX "donations_campaign_id_idx" ON "donations" ("campaign_id")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_donor_address_idx" ON "donations" ("donor_address")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_status_idx" ON "donations" ("status")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_created_at_idx" ON "donations" ("created_at")`) | ||
| await queryRunner.query(`CREATE INDEX "donations_transaction_hash_idx" ON "donations" ("transaction_hash")`) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Index the new primary filter columns.
donor_id and donation_token are first-class list filters in this PR, but the migration only indexes donor_address. The user-scoped and token-scoped donation endpoints will degrade to table scans once this table grows.
Suggested fix
await queryRunner.query(`CREATE INDEX "donations_campaign_id_idx" ON "donations" ("campaign_id")`)
+ await queryRunner.query(`CREATE INDEX "donations_donor_id_idx" ON "donations" ("donor_id")`)
await queryRunner.query(`CREATE INDEX "donations_donor_address_idx" ON "donations" ("donor_address")`)
+ await queryRunner.query(`CREATE INDEX "donations_token_address_idx" ON "donations" ("token_address")`)
await queryRunner.query(`CREATE INDEX "donations_status_idx" ON "donations" ("status")`)
@@
+ await queryRunner.query(`DROP INDEX IF EXISTS "donations_token_address_idx"`)
await queryRunner.query(`DROP INDEX IF EXISTS "donations_status_idx"`)
await queryRunner.query(`DROP INDEX IF EXISTS "donations_donor_address_idx"`)
+ await queryRunner.query(`DROP INDEX IF EXISTS "donations_donor_id_idx"`)
await queryRunner.query(`DROP INDEX IF EXISTS "donations_campaign_id_idx"`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await queryRunner.query(`CREATE INDEX "donations_campaign_id_idx" ON "donations" ("campaign_id")`) | |
| await queryRunner.query(`CREATE INDEX "donations_donor_address_idx" ON "donations" ("donor_address")`) | |
| await queryRunner.query(`CREATE INDEX "donations_status_idx" ON "donations" ("status")`) | |
| await queryRunner.query(`CREATE INDEX "donations_created_at_idx" ON "donations" ("created_at")`) | |
| await queryRunner.query(`CREATE INDEX "donations_transaction_hash_idx" ON "donations" ("transaction_hash")`) | |
| await queryRunner.query(`CREATE INDEX "donations_campaign_id_idx" ON "donations" ("campaign_id")`) | |
| await queryRunner.query(`CREATE INDEX "donations_donor_id_idx" ON "donations" ("donor_id")`) | |
| await queryRunner.query(`CREATE INDEX "donations_donor_address_idx" ON "donations" ("donor_address")`) | |
| await queryRunner.query(`CREATE INDEX "donations_token_address_idx" ON "donations" ("token_address")`) | |
| await queryRunner.query(`CREATE INDEX "donations_status_idx" ON "donations" ("status")`) | |
| await queryRunner.query(`CREATE INDEX "donations_created_at_idx" ON "donations" ("created_at")`) | |
| await queryRunner.query(`CREATE INDEX "donations_transaction_hash_idx" ON "donations" ("transaction_hash")`) |
🤖 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/migrations/CreateDonationsTable1760000000002.js` around lines 39 - 43,
The donation table migration is missing indexes for the new primary filter
columns, so update CreateDonationsTable1760000000002 to add indexes for donor_id
and donation_token alongside the existing create index calls in the migration’s
up path. Make sure the corresponding down path in the same migration removes
those indexes as well, and keep the changes aligned with the existing
queryRunner.query patterns used for donations_campaign_id_idx and
donations_donor_address_idx.
Contributor
|
Hi @Skinny001 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. |
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
2 participants
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.
This pull request introduces a comprehensive implementation of the donation API, including route definitions, controller logic, validation, and data access layers. It also adds robust authentication and authorization middleware, as well as thorough test coverage for validation logic.
Key changes include:
Donation API Implementation
DonationEntitymodel with full schema, including new fields, database indexes, and enum support for status and network. (src/components/v1/Donation/donation.entity.ts)DonationResponseDto,PaginatedResponse, andDonationStatsDtotypes to standardize API responses. (src/components/v1/Donation/donation.dto.ts)src/components/v1/Donation/donation.controller.ts)src/components/v1/Donation/donation.routes.ts)Authentication & Authorization
requireAdminApimiddleware for admin-only endpoints. (src/appMiddlewares/jwtAuth.api.ts)Validation & Testing
src/__tests__/donation.validation.test.ts)- Add DonationEntity, service, controller, routes, validation, DTOSummary
Describe what changed and why.
Area
src/)indexer/common/)indexer/streams/)indexer/distributions/)Scope
Verification
bun run type-checkbun run testbun run lintbun run indexer:type-checkif indexer files changedbun run indexer:testif indexer files changedbun run indexer:lintif indexer files changedIndexer Safety
Notes
Closes #9
Summary by CodeRabbit
New Features
Bug Fixes